rtmppkt: Check for packet size mismatches

Message ID 20161215081225.72594-1-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Dec. 15, 2016, 8:12 a.m.
From: Michael Niedermayer <michael@niedermayer.cc>

Fixes out of array access.

Found-by: Paul Cher <paulcher@icloud.com>
Reviewed-by: Paul Cher <paulcher@icloud.com>

CC: libav-stable@libav.org
---
 libavformat/rtmppkt.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Luca Barbato Dec. 15, 2016, 12:43 p.m. | #1
On 15/12/2016 09:12, Martin Storsjö wrote:
> From: Michael Niedermayer <michael@niedermayer.cc>
> 
> Fixes out of array access.
> 
> Found-by: Paul Cher <paulcher@icloud.com>
> Reviewed-by: Paul Cher <paulcher@icloud.com>
> 
> CC: libav-stable@libav.org
> ---
>  libavformat/rtmppkt.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
> index f8c51d0..373c3ea 100644
> --- a/libavformat/rtmppkt.c
> +++ b/libavformat/rtmppkt.c
> @@ -235,6 +235,13 @@ static int rtmp_packet_read_one_chunk(URLContext *h, RTMPPacket *p,
>      if (hdr != RTMP_PS_TWELVEBYTES)
>          timestamp += prev_pkt[channel_id].timestamp;
>  
> +    if (prev_pkt[channel_id].read && size != prev_pkt[channel_id].size) {
> +        av_log(h, AV_LOG_ERROR, "RTMP packet size mismatch %d != %d\n",
> +                                size, prev_pkt[channel_id].size);
> +        ff_rtmp_packet_destroy(&prev_pkt[channel_id]);
> +        prev_pkt[channel_id].read = 0;
> +    }
> +
>      if (!prev_pkt[channel_id].read) {
>          if ((ret = ff_rtmp_packet_create(p, channel_id, type, timestamp,
>                                           size)) < 0)
> 

Why it happens?

If it is ok not to forward an error the log message could be demoted to
verbose.

lu
Martin Storsjö Dec. 15, 2016, 12:53 p.m. | #2
On Thu, 15 Dec 2016, Luca Barbato wrote:

> On 15/12/2016 09:12, Martin Storsjö wrote:
>> From: Michael Niedermayer <michael@niedermayer.cc>
>>
>> Fixes out of array access.
>>
>> Found-by: Paul Cher <paulcher@icloud.com>
>> Reviewed-by: Paul Cher <paulcher@icloud.com>
>>
>> CC: libav-stable@libav.org
>> ---
>>  libavformat/rtmppkt.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
>> index f8c51d0..373c3ea 100644
>> --- a/libavformat/rtmppkt.c
>> +++ b/libavformat/rtmppkt.c
>> @@ -235,6 +235,13 @@ static int rtmp_packet_read_one_chunk(URLContext *h, RTMPPacket *p,
>>      if (hdr != RTMP_PS_TWELVEBYTES)
>>          timestamp += prev_pkt[channel_id].timestamp;
>>
>> +    if (prev_pkt[channel_id].read && size != prev_pkt[channel_id].size) {
>> +        av_log(h, AV_LOG_ERROR, "RTMP packet size mismatch %d != %d\n",
>> +                                size, prev_pkt[channel_id].size);
>> +        ff_rtmp_packet_destroy(&prev_pkt[channel_id]);
>> +        prev_pkt[channel_id].read = 0;
>> +    }
>> +
>>      if (!prev_pkt[channel_id].read) {
>>          if ((ret = ff_rtmp_packet_create(p, channel_id, type, timestamp,
>>                                           size)) < 0)
>>
>
> Why it happens?

When you have fragmented packets, the first packet declares the size and 
the later ones (normally) are small follow-on packets that don't repeat 
the size and all that. But technically the later fragments also can have a 
full header, declaring a different size than the previous packet. In those 
cases we didn't use to check that the partial packet that was allocated 
earlier actually matches the size in the current packet.

> If it is ok not to forward an error the log message could be demoted to
> verbose.

I guess we can and should return an error here directly as well.

// Martin
Luca Barbato Dec. 15, 2016, 1 p.m. | #3
On 15/12/2016 13:53, Martin Storsjö wrote:
> I guess we can and should return an error here directly as well.

I agree =)

lu

Patch

diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c
index f8c51d0..373c3ea 100644
--- a/libavformat/rtmppkt.c
+++ b/libavformat/rtmppkt.c
@@ -235,6 +235,13 @@  static int rtmp_packet_read_one_chunk(URLContext *h, RTMPPacket *p,
     if (hdr != RTMP_PS_TWELVEBYTES)
         timestamp += prev_pkt[channel_id].timestamp;
 
+    if (prev_pkt[channel_id].read && size != prev_pkt[channel_id].size) {
+        av_log(h, AV_LOG_ERROR, "RTMP packet size mismatch %d != %d\n",
+                                size, prev_pkt[channel_id].size);
+        ff_rtmp_packet_destroy(&prev_pkt[channel_id]);
+        prev_pkt[channel_id].read = 0;
+    }
+
     if (!prev_pkt[channel_id].read) {
         if ((ret = ff_rtmp_packet_create(p, channel_id, type, timestamp,
                                          size)) < 0)