| Message ID | 20190829225421.97898-1-unique.will.martin@gmail.com |
|---|---|
| State | New |
| Headers | show |
hello. I didn't see any comments about this patch - does that mean it is ready to be merged? On Thu, Aug 29, 2019 at 3:54 PM William Martin <unique.will.martin@gmail.com> wrote: > From: Will Martin <will.martin@verizondigitalmedia.com> > > Some rtmp streamers (i.e. AWS Elemental Encoder, Wirecast) send C0 and C1 > together and expect S0 and S1 returned together. When sent in different > packets, this results in a C2 handshake. This patch fixes that error. > Note that the patch is based off of a fix proposed by rubensanchez in > https://trac.ffmpeg.org/ticket/6453. > The only difference between that propsed fix and this patch is that > dummy_unit is declared as a uint32_t instead of unit8_8 (this addresses a > crash in debug builds). > This patch being submitted in a [v2] so that these commit messages could > be added for clarity. > --- > libavformat/rtmpproto.c | 103 +++++++++++++++++++++++----------------- > 1 file changed, 59 insertions(+), 44 deletions(-) > > diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c > index b741e421af..24070ba0f5 100644 > --- a/libavformat/rtmpproto.c > +++ b/libavformat/rtmpproto.c > @@ -1416,71 +1416,86 @@ static int rtmp_send_hs_packet(RTMPContext* rt, > uint32_t first_int, > */ > static int rtmp_server_handshake(URLContext *s, RTMPContext *rt) > { > - uint8_t buffer[RTMP_HANDSHAKE_PACKET_SIZE]; > - uint32_t hs_epoch; > + uint8_t hs_s0s1[RTMP_HANDSHAKE_PACKET_SIZE + 1]; > + uint8_t hs_c0c1[RTMP_HANDSHAKE_PACKET_SIZE + 1]; > + uint8_t hs_c2[RTMP_HANDSHAKE_PACKET_SIZE + 1]; > + uint8_t hs_s2[RTMP_HANDSHAKE_PACKET_SIZE]; > + uint32_t dummy_uint; > uint32_t hs_my_epoch; > - uint8_t hs_c1[RTMP_HANDSHAKE_PACKET_SIZE]; > - uint8_t hs_s1[RTMP_HANDSHAKE_PACKET_SIZE]; > - uint32_t zeroes; > uint32_t temp = 0; > int randomidx = 0; > int inoutsize = 0; > int ret; > > - inoutsize = ffurl_read_complete(rt->stream, buffer, 1); // > Receive C0 > - if (inoutsize <= 0) { > - av_log(s, AV_LOG_ERROR, "Unable to read handshake\n"); > - return AVERROR(EIO); > + /**************** > + * Receive C0+C1 > + ***************/ > + ret = rtmp_receive_hs_packet(rt, &dummy_uint, &dummy_uint, hs_c0c1, > + RTMP_HANDSHAKE_PACKET_SIZE + 1); > + if (ret) { > + av_log(s, AV_LOG_ERROR, "RTMP Handshake C1 Error %d\n", ret); > + return ret; > } > // Check Version > - if (buffer[0] != 3) { > - av_log(s, AV_LOG_ERROR, "RTMP protocol version mismatch\n"); > + if (hs_c0c1[0] != 3) { > + av_log(s, AV_LOG_ERROR, "RTMP protocol version mismatch. Expected > 0x03 received %02x\n", hs_c0c1[0]); > return AVERROR(EIO); > } > - if (ffurl_write(rt->stream, buffer, 1) <= 0) { // > Send S0 > - av_log(s, AV_LOG_ERROR, > - "Unable to write answer - RTMP S0\n"); > + // Get client epoch and set our with the same value > + hs_my_epoch = AV_RB32(hs_c0c1 + 1); > + > + /************* > + * Send S0+S1 > + ************/ > + // Generate random data to send it on S0+S1 > + for (randomidx = 9; randomidx < (RTMP_HANDSHAKE_PACKET_SIZE + 1); > + randomidx += 4) > + AV_WB32(hs_s0s1 + randomidx, av_get_random_seed()); > + // Set the RTMP protocol code on S0+S1 (First byte) > + hs_s0s1[0] = 0x03; > + // Copy the random data from C1 to S1 > + memcpy(hs_s0s1 + 1, hs_c0c1 + 1, RTMP_HANDSHAKE_PACKET_SIZE); > + AV_WB32(hs_s0s1 + 1, hs_my_epoch); > + AV_WB32(hs_s0s1 + 5, 0); > + inoutsize = ffurl_write(rt->stream, hs_s0s1, > + RTMP_HANDSHAKE_PACKET_SIZE + 1); > + if (inoutsize != RTMP_HANDSHAKE_PACKET_SIZE + 1) { > + av_log(s, AV_LOG_ERROR, "RTMP Handshake S1 Error %d\n", ret); > return AVERROR(EIO); > } > - /* Receive C1 */ > - ret = rtmp_receive_hs_packet(rt, &hs_epoch, &zeroes, hs_c1, > - RTMP_HANDSHAKE_PACKET_SIZE); > - if (ret) { > - av_log(s, AV_LOG_ERROR, "RTMP Handshake C1 Error\n"); > - return ret; > - } > - /* Send S1 */ > - /* By now same epoch will be sent */ > - hs_my_epoch = hs_epoch; > - /* Generate random */ > - for (randomidx = 8; randomidx < (RTMP_HANDSHAKE_PACKET_SIZE); > - randomidx += 4) > - AV_WB32(hs_s1 + randomidx, av_get_random_seed()); > > - ret = rtmp_send_hs_packet(rt, hs_my_epoch, 0, hs_s1, > - RTMP_HANDSHAKE_PACKET_SIZE); > - if (ret) { > - av_log(s, AV_LOG_ERROR, "RTMP Handshake S1 Error\n"); > - return ret; > - } > - /* Send S2 */ > - ret = rtmp_send_hs_packet(rt, hs_epoch, 0, hs_c1, > + /*********** > + * Send S2 > + **********/ > + // Get the S2 random data from C0+C1 > + memcpy(hs_s2, hs_c0c1, RTMP_HANDSHAKE_PACKET_SIZE); > + ret = rtmp_send_hs_packet(rt, hs_my_epoch, 0, hs_s2, > RTMP_HANDSHAKE_PACKET_SIZE); > if (ret) { > av_log(s, AV_LOG_ERROR, "RTMP Handshake S2 Error\n"); > return ret; > } > - /* Receive C2 */ > - ret = rtmp_receive_hs_packet(rt, &temp, &zeroes, buffer, > - RTMP_HANDSHAKE_PACKET_SIZE); > - if (ret) { > - av_log(s, AV_LOG_ERROR, "RTMP Handshake C2 Error\n"); > - return ret; > + > + /************* > + * Receive C2 > + ************/ > + > + ret = ffurl_read_complete(rt->stream, hs_c2, > + RTMP_HANDSHAKE_PACKET_SIZE + 1); > + if (ret <= 0) > + return AVERROR(EIO); > + if (ret != RTMP_HANDSHAKE_PACKET_SIZE + 1) { > + av_log(rt, AV_LOG_ERROR, "Erroneous Message size %d" > + " not following standard\n", (int)inoutsize); > + return AVERROR(EINVAL); > } > + > + // Check timestamp and random data from C2 > + temp = AV_RB32(hs_c2 + 1); > if (temp != hs_my_epoch) > av_log(s, AV_LOG_WARNING, > - "Erroneous C2 Message epoch does not match up with C1 > epoch\n"); > - if (memcmp(buffer + 8, hs_s1 + 8, > + "Erroneous C2 Message epoch does not match up with C1 > epoch"); > + if (memcmp(hs_c2 + 9, hs_c0c1 + 9, > RTMP_HANDSHAKE_PACKET_SIZE - 8)) > av_log(s, AV_LOG_WARNING, > "Erroneous C2 Message random does not match up\n"); > -- > 2.20.1 (Apple Git-117) > >
On Thu, Aug 29, 2019 at 03:54:21PM -0700, William Martin wrote: > From: Will Martin <will.martin@verizondigitalmedia.com> > > Some rtmp streamers (i.e. AWS Elemental Encoder, Wirecast) send C0 and C1 together and expect S0 and S1 returned together. When sent in different packets, this results in a C2 handshake. This patch fixes that error. > Note that the patch is based off of a fix proposed by rubensanchez in https://trac.ffmpeg.org/ticket/6453. > The only difference between that propsed fix and this patch is that dummy_unit is declared as a uint32_t instead of unit8_8 (this addresses a crash in debug builds). > This patch being submitted in a [v2] so that these commit messages could be added for clarity. > --- > libavformat/rtmpproto.c | 103 +++++++++++++++++++++++----------------- > 1 file changed, 59 insertions(+), 44 deletions(-) > > diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c > index b741e421af..24070ba0f5 100644 > --- a/libavformat/rtmpproto.c > +++ b/libavformat/rtmpproto.c > @@ -1416,71 +1416,86 @@ static int rtmp_send_hs_packet(RTMPContext* rt, uint32_t first_int, > */ > static int rtmp_server_handshake(URLContext *s, RTMPContext *rt) > { > - uint8_t buffer[RTMP_HANDSHAKE_PACKET_SIZE]; > - uint32_t hs_epoch; > + uint8_t hs_s0s1[RTMP_HANDSHAKE_PACKET_SIZE + 1]; > + uint8_t hs_c0c1[RTMP_HANDSHAKE_PACKET_SIZE + 1]; > + uint8_t hs_c2[RTMP_HANDSHAKE_PACKET_SIZE + 1]; > + uint8_t hs_s2[RTMP_HANDSHAKE_PACKET_SIZE]; > + uint32_t dummy_uint; > uint32_t hs_my_epoch; > - uint8_t hs_c1[RTMP_HANDSHAKE_PACKET_SIZE]; > - uint8_t hs_s1[RTMP_HANDSHAKE_PACKET_SIZE]; > - uint32_t zeroes; > uint32_t temp = 0; > int randomidx = 0; > int inoutsize = 0; > int ret; > > - inoutsize = ffurl_read_complete(rt->stream, buffer, 1); // Receive C0 > - if (inoutsize <= 0) { > - av_log(s, AV_LOG_ERROR, "Unable to read handshake\n"); > - return AVERROR(EIO); > + /**************** > + * Receive C0+C1 > + ***************/ > + ret = rtmp_receive_hs_packet(rt, &dummy_uint, &dummy_uint, hs_c0c1, > + RTMP_HANDSHAKE_PACKET_SIZE + 1); > + if (ret) { > + av_log(s, AV_LOG_ERROR, "RTMP Handshake C1 Error %d\n", ret); > + return ret; > } > // Check Version > - if (buffer[0] != 3) { > - av_log(s, AV_LOG_ERROR, "RTMP protocol version mismatch\n"); > + if (hs_c0c1[0] != 3) { > + av_log(s, AV_LOG_ERROR, "RTMP protocol version mismatch. Expected 0x03 received %02x\n", hs_c0c1[0]); > return AVERROR(EIO); > } > - if (ffurl_write(rt->stream, buffer, 1) <= 0) { // Send S0 > - av_log(s, AV_LOG_ERROR, > - "Unable to write answer - RTMP S0\n"); > + // Get client epoch and set our with the same value > + hs_my_epoch = AV_RB32(hs_c0c1 + 1); > + > + /************* > + * Send S0+S1 > + ************/ > + // Generate random data to send it on S0+S1 > + for (randomidx = 9; randomidx < (RTMP_HANDSHAKE_PACKET_SIZE + 1); > + randomidx += 4) > + AV_WB32(hs_s0s1 + randomidx, av_get_random_seed()); > + // Set the RTMP protocol code on S0+S1 (First byte) > + hs_s0s1[0] = 0x03; > + // Copy the random data from C1 to S1 > + memcpy(hs_s0s1 + 1, hs_c0c1 + 1, RTMP_HANDSHAKE_PACKET_SIZE); > + AV_WB32(hs_s0s1 + 1, hs_my_epoch); > + AV_WB32(hs_s0s1 + 5, 0); > + inoutsize = ffurl_write(rt->stream, hs_s0s1, > + RTMP_HANDSHAKE_PACKET_SIZE + 1); > + if (inoutsize != RTMP_HANDSHAKE_PACKET_SIZE + 1) { > + av_log(s, AV_LOG_ERROR, "RTMP Handshake S1 Error %d\n", ret); > return AVERROR(EIO); > } > - /* Receive C1 */ > - ret = rtmp_receive_hs_packet(rt, &hs_epoch, &zeroes, hs_c1, > - RTMP_HANDSHAKE_PACKET_SIZE); > - if (ret) { > - av_log(s, AV_LOG_ERROR, "RTMP Handshake C1 Error\n"); > - return ret; > - } > - /* Send S1 */ > - /* By now same epoch will be sent */ > - hs_my_epoch = hs_epoch; > - /* Generate random */ > - for (randomidx = 8; randomidx < (RTMP_HANDSHAKE_PACKET_SIZE); > - randomidx += 4) > - AV_WB32(hs_s1 + randomidx, av_get_random_seed()); > > - ret = rtmp_send_hs_packet(rt, hs_my_epoch, 0, hs_s1, > - RTMP_HANDSHAKE_PACKET_SIZE); > - if (ret) { > - av_log(s, AV_LOG_ERROR, "RTMP Handshake S1 Error\n"); > - return ret; > - } > - /* Send S2 */ > + /*********** > + * Send S2 > + **********/ > + // Get the S2 random data from C0+C1 > + memcpy(hs_s2, hs_c0c1, RTMP_HANDSHAKE_PACKET_SIZE); > - ret = rtmp_send_hs_packet(rt, hs_epoch, 0, hs_c1, > + ret = rtmp_send_hs_packet(rt, hs_my_epoch, 0, hs_s2, > RTMP_HANDSHAKE_PACKET_SIZE); > if (ret) { > av_log(s, AV_LOG_ERROR, "RTMP Handshake S2 Error\n"); > return ret; > } > - /* Receive C2 */ > + > + /************* > + * Receive C2 > + ************/ this patch is mixing cosmetics and functional changes and is quite messy, its unlikely that this will be applied and its not easy to review because of this as well If you could try to make it look more tidy overall so its easy to review then its more likely to receice usefull review comments generally, if you need to rename, move or reformat something do it in a seperate patch. This should make this more readable Thanks [...]
diff --git a/libavformat/rtmpproto.c b/libavformat/rtmpproto.c index b741e421af..24070ba0f5 100644 --- a/libavformat/rtmpproto.c +++ b/libavformat/rtmpproto.c @@ -1416,71 +1416,86 @@ static int rtmp_send_hs_packet(RTMPContext* rt, uint32_t first_int, */ static int rtmp_server_handshake(URLContext *s, RTMPContext *rt) { - uint8_t buffer[RTMP_HANDSHAKE_PACKET_SIZE]; - uint32_t hs_epoch; + uint8_t hs_s0s1[RTMP_HANDSHAKE_PACKET_SIZE + 1]; + uint8_t hs_c0c1[RTMP_HANDSHAKE_PACKET_SIZE + 1]; + uint8_t hs_c2[RTMP_HANDSHAKE_PACKET_SIZE + 1]; + uint8_t hs_s2[RTMP_HANDSHAKE_PACKET_SIZE]; + uint32_t dummy_uint; uint32_t hs_my_epoch; - uint8_t hs_c1[RTMP_HANDSHAKE_PACKET_SIZE]; - uint8_t hs_s1[RTMP_HANDSHAKE_PACKET_SIZE]; - uint32_t zeroes; uint32_t temp = 0; int randomidx = 0; int inoutsize = 0; int ret; - inoutsize = ffurl_read_complete(rt->stream, buffer, 1); // Receive C0 - if (inoutsize <= 0) { - av_log(s, AV_LOG_ERROR, "Unable to read handshake\n"); - return AVERROR(EIO); + /**************** + * Receive C0+C1 + ***************/ + ret = rtmp_receive_hs_packet(rt, &dummy_uint, &dummy_uint, hs_c0c1, + RTMP_HANDSHAKE_PACKET_SIZE + 1); + if (ret) { + av_log(s, AV_LOG_ERROR, "RTMP Handshake C1 Error %d\n", ret); + return ret; } // Check Version - if (buffer[0] != 3) { - av_log(s, AV_LOG_ERROR, "RTMP protocol version mismatch\n"); + if (hs_c0c1[0] != 3) { + av_log(s, AV_LOG_ERROR, "RTMP protocol version mismatch. Expected 0x03 received %02x\n", hs_c0c1[0]); return AVERROR(EIO); } - if (ffurl_write(rt->stream, buffer, 1) <= 0) { // Send S0 - av_log(s, AV_LOG_ERROR, - "Unable to write answer - RTMP S0\n"); + // Get client epoch and set our with the same value + hs_my_epoch = AV_RB32(hs_c0c1 + 1); + + /************* + * Send S0+S1 + ************/ + // Generate random data to send it on S0+S1 + for (randomidx = 9; randomidx < (RTMP_HANDSHAKE_PACKET_SIZE + 1); + randomidx += 4) + AV_WB32(hs_s0s1 + randomidx, av_get_random_seed()); + // Set the RTMP protocol code on S0+S1 (First byte) + hs_s0s1[0] = 0x03; + // Copy the random data from C1 to S1 + memcpy(hs_s0s1 + 1, hs_c0c1 + 1, RTMP_HANDSHAKE_PACKET_SIZE); + AV_WB32(hs_s0s1 + 1, hs_my_epoch); + AV_WB32(hs_s0s1 + 5, 0); + inoutsize = ffurl_write(rt->stream, hs_s0s1, + RTMP_HANDSHAKE_PACKET_SIZE + 1); + if (inoutsize != RTMP_HANDSHAKE_PACKET_SIZE + 1) { + av_log(s, AV_LOG_ERROR, "RTMP Handshake S1 Error %d\n", ret); return AVERROR(EIO); } - /* Receive C1 */ - ret = rtmp_receive_hs_packet(rt, &hs_epoch, &zeroes, hs_c1, - RTMP_HANDSHAKE_PACKET_SIZE); - if (ret) { - av_log(s, AV_LOG_ERROR, "RTMP Handshake C1 Error\n"); - return ret; - } - /* Send S1 */ - /* By now same epoch will be sent */ - hs_my_epoch = hs_epoch; - /* Generate random */ - for (randomidx = 8; randomidx < (RTMP_HANDSHAKE_PACKET_SIZE); - randomidx += 4) - AV_WB32(hs_s1 + randomidx, av_get_random_seed()); - ret = rtmp_send_hs_packet(rt, hs_my_epoch, 0, hs_s1, - RTMP_HANDSHAKE_PACKET_SIZE); - if (ret) { - av_log(s, AV_LOG_ERROR, "RTMP Handshake S1 Error\n"); - return ret; - } - /* Send S2 */ - ret = rtmp_send_hs_packet(rt, hs_epoch, 0, hs_c1, + /*********** + * Send S2 + **********/ + // Get the S2 random data from C0+C1 + memcpy(hs_s2, hs_c0c1, RTMP_HANDSHAKE_PACKET_SIZE); + ret = rtmp_send_hs_packet(rt, hs_my_epoch, 0, hs_s2, RTMP_HANDSHAKE_PACKET_SIZE); if (ret) { av_log(s, AV_LOG_ERROR, "RTMP Handshake S2 Error\n"); return ret; } - /* Receive C2 */ - ret = rtmp_receive_hs_packet(rt, &temp, &zeroes, buffer, - RTMP_HANDSHAKE_PACKET_SIZE); - if (ret) { - av_log(s, AV_LOG_ERROR, "RTMP Handshake C2 Error\n"); - return ret; + + /************* + * Receive C2 + ************/ + + ret = ffurl_read_complete(rt->stream, hs_c2, + RTMP_HANDSHAKE_PACKET_SIZE + 1); + if (ret <= 0) + return AVERROR(EIO); + if (ret != RTMP_HANDSHAKE_PACKET_SIZE + 1) { + av_log(rt, AV_LOG_ERROR, "Erroneous Message size %d" + " not following standard\n", (int)inoutsize); + return AVERROR(EINVAL); } + + // Check timestamp and random data from C2 + temp = AV_RB32(hs_c2 + 1); if (temp != hs_my_epoch) av_log(s, AV_LOG_WARNING, - "Erroneous C2 Message epoch does not match up with C1 epoch\n"); - if (memcmp(buffer + 8, hs_s1 + 8, + "Erroneous C2 Message epoch does not match up with C1 epoch"); + if (memcmp(hs_c2 + 9, hs_c0c1 + 9, RTMP_HANDSHAKE_PACKET_SIZE - 8)) av_log(s, AV_LOG_WARNING, "Erroneous C2 Message random does not match up\n");
From: Will Martin <will.martin@verizondigitalmedia.com> Some rtmp streamers (i.e. AWS Elemental Encoder, Wirecast) send C0 and C1 together and expect S0 and S1 returned together. When sent in different packets, this results in a C2 handshake. This patch fixes that error. Note that the patch is based off of a fix proposed by rubensanchez in https://trac.ffmpeg.org/ticket/6453. The only difference between that propsed fix and this patch is that dummy_unit is declared as a uint32_t instead of unit8_8 (this addresses a crash in debug builds). This patch being submitted in a [v2] so that these commit messages could be added for clarity. --- libavformat/rtmpproto.c | 103 +++++++++++++++++++++++----------------- 1 file changed, 59 insertions(+), 44 deletions(-)