diff mbox series

[FFmpeg-devel] avformat/utils: fix undefined behaviour

Message ID 20210214210923.12099-1-onemda@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/utils: fix undefined behaviour
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Paul B Mahol Feb. 14, 2021, 9:09 p.m. UTC
Fixes following report:
libavformat/utils.c:1429:14: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavformat/utils.c:1429:14

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavformat/utils.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

James Almer Feb. 14, 2021, 9:21 p.m. UTC | #1
On 2/14/2021 6:09 PM, Paul B Mahol wrote:
> Fixes following report:
> libavformat/utils.c:1429:14: runtime error: applying zero offset to null pointer

How is data NULL here? That's the input packet's data pointer, and this 
loop is accessed only if size is > 0. data == NULL and size != 0 doesn't 
sound valid. Or am i missing something?

Try compiling with assert level set to 1, see if you get an assertion 
failure on avpacket helpers.

> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavformat/utils.c:1429:14
> 
> Signed-off-by: Paul B Mahol <onemda@gmail.com>
> ---
>   libavformat/utils.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 3e955b85bc..e4f100fda2 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -1426,8 +1426,10 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt,
>           pkt->pts = pkt->dts = AV_NOPTS_VALUE;
>           pkt->pos = -1;
>           /* increment read pointer */
> -        data += len;
> -        size -= len;
> +        if (len > 0) {
> +            data += len;
> +            size -= len;
> +        }
>   
>           got_output = !!out_pkt.size;
>   
>
Andreas Rheinhardt Feb. 14, 2021, 9:23 p.m. UTC | #2
James Almer:
> On 2/14/2021 6:09 PM, Paul B Mahol wrote:
>> Fixes following report:
>> libavformat/utils.c:1429:14: runtime error: applying zero offset to
>> null pointer
> 
> How is data NULL here? That's the input packet's data pointer, and this
> loop is accessed only if size is > 0. data == NULL and size != 0 doesn't
> sound valid. Or am i missing something?

Flushing.

> 
> Try compiling with assert level set to 1, see if you get an assertion
> failure on avpacket helpers.
> 
>> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
>> libavformat/utils.c:1429:14
>>
>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>> ---
>>   libavformat/utils.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>> index 3e955b85bc..e4f100fda2 100644
>> --- a/libavformat/utils.c
>> +++ b/libavformat/utils.c
>> @@ -1426,8 +1426,10 @@ static int parse_packet(AVFormatContext *s,
>> AVPacket *pkt,
>>           pkt->pts = pkt->dts = AV_NOPTS_VALUE;
>>           pkt->pos = -1;
>>           /* increment read pointer */
>> -        data += len;
>> -        size -= len;
>> +        if (len > 0) {
>> +            data += len;
>> +            size -= len;
>> +        }
>>             got_output = !!out_pkt.size;
>>  
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Feb. 14, 2021, 9:28 p.m. UTC | #3
On 2/14/2021 6:23 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 2/14/2021 6:09 PM, Paul B Mahol wrote:
>>> Fixes following report:
>>> libavformat/utils.c:1429:14: runtime error: applying zero offset to
>>> null pointer
>>
>> How is data NULL here? That's the input packet's data pointer, and this
>> loop is accessed only if size is > 0. data == NULL and size != 0 doesn't
>> sound valid. Or am i missing something?
> 
> Flushing.

A flush packet with data == NULL and size != 0? ff_read_packet(), called 
before the flush attempt, initializes the packet to defaults. So if it 
returns < 1, shouldn't the packet remain clean?

> 
>>
>> Try compiling with assert level set to 1, see if you get an assertion
>> failure on avpacket helpers.
>>
>>> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
>>> libavformat/utils.c:1429:14
>>>
>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>> ---
>>>    libavformat/utils.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>> index 3e955b85bc..e4f100fda2 100644
>>> --- a/libavformat/utils.c
>>> +++ b/libavformat/utils.c
>>> @@ -1426,8 +1426,10 @@ static int parse_packet(AVFormatContext *s,
>>> AVPacket *pkt,
>>>            pkt->pts = pkt->dts = AV_NOPTS_VALUE;
>>>            pkt->pos = -1;
>>>            /* increment read pointer */
>>> -        data += len;
>>> -        size -= len;
>>> +        if (len > 0) {
>>> +            data += len;
>>> +            size -= len;
>>> +        }
>>>              got_output = !!out_pkt.size;
>>>   
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Andreas Rheinhardt Feb. 14, 2021, 9:36 p.m. UTC | #4
James Almer:
> On 2/14/2021 6:23 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 2/14/2021 6:09 PM, Paul B Mahol wrote:
>>>> Fixes following report:
>>>> libavformat/utils.c:1429:14: runtime error: applying zero offset to
>>>> null pointer
>>>
>>> How is data NULL here? That's the input packet's data pointer, and this
>>> loop is accessed only if size is > 0. data == NULL and size != 0 doesn't
>>> sound valid. Or am i missing something?
>>
>> Flushing.
> 
> A flush packet with data == NULL and size != 0? ff_read_packet(), called
> before the flush attempt, initializes the packet to defaults. So if it
> returns < 1, shouldn't the packet remain clean?

A flush packet with data == NULL and size == 0. And the parser of course
returns 0 in this case, which leads to NULL + 0, which is UB.

> 
>>
>>>
>>> Try compiling with assert level set to 1, see if you get an assertion
>>> failure on avpacket helpers.
>>>
>>>> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
>>>> libavformat/utils.c:1429:14
>>>>
>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>> ---
>>>>    libavformat/utils.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>>> index 3e955b85bc..e4f100fda2 100644
>>>> --- a/libavformat/utils.c
>>>> +++ b/libavformat/utils.c
>>>> @@ -1426,8 +1426,10 @@ static int parse_packet(AVFormatContext *s,
>>>> AVPacket *pkt,
>>>>            pkt->pts = pkt->dts = AV_NOPTS_VALUE;
>>>>            pkt->pos = -1;
>>>>            /* increment read pointer */
>>>> -        data += len;
>>>> -        size -= len;
>>>> +        if (len > 0) {
>>>> +            data += len;
>>>> +            size -= len;
>>>> +        }
>>>>              got_output = !!out_pkt.size;
>>>>   
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer Feb. 14, 2021, 9:38 p.m. UTC | #5
On 2/14/2021 6:36 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 2/14/2021 6:23 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 2/14/2021 6:09 PM, Paul B Mahol wrote:
>>>>> Fixes following report:
>>>>> libavformat/utils.c:1429:14: runtime error: applying zero offset to
>>>>> null pointer
>>>>
>>>> How is data NULL here? That's the input packet's data pointer, and this
>>>> loop is accessed only if size is > 0. data == NULL and size != 0 doesn't
>>>> sound valid. Or am i missing something?
>>>
>>> Flushing.
>>
>> A flush packet with data == NULL and size != 0? ff_read_packet(), called
>> before the flush attempt, initializes the packet to defaults. So if it
>> returns < 1, shouldn't the packet remain clean?
> 
> A flush packet with data == NULL and size == 0. And the parser of course
> returns 0 in this case, which leads to NULL + 0, which is UB.

Oh, i missed the flush && got_output check that bypasses the size one.
Nevermind then.

> 
>>
>>>
>>>>
>>>> Try compiling with assert level set to 1, see if you get an assertion
>>>> failure on avpacket helpers.
>>>>
>>>>> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
>>>>> libavformat/utils.c:1429:14
>>>>>
>>>>> Signed-off-by: Paul B Mahol <onemda@gmail.com>
>>>>> ---
>>>>>     libavformat/utils.c | 6 ++++--
>>>>>     1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>>>> index 3e955b85bc..e4f100fda2 100644
>>>>> --- a/libavformat/utils.c
>>>>> +++ b/libavformat/utils.c
>>>>> @@ -1426,8 +1426,10 @@ static int parse_packet(AVFormatContext *s,
>>>>> AVPacket *pkt,
>>>>>             pkt->pts = pkt->dts = AV_NOPTS_VALUE;
>>>>>             pkt->pos = -1;
>>>>>             /* increment read pointer */
>>>>> -        data += len;
>>>>> -        size -= len;
>>>>> +        if (len > 0) {
>>>>> +            data += len;
>>>>> +            size -= len;
>>>>> +        }
>>>>>               got_output = !!out_pkt.size;
>>>>>    
>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>> To unsubscribe, visit link above, or email
>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox series

Patch

diff --git a/libavformat/utils.c b/libavformat/utils.c
index 3e955b85bc..e4f100fda2 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -1426,8 +1426,10 @@  static int parse_packet(AVFormatContext *s, AVPacket *pkt,
         pkt->pts = pkt->dts = AV_NOPTS_VALUE;
         pkt->pos = -1;
         /* increment read pointer */
-        data += len;
-        size -= len;
+        if (len > 0) {
+            data += len;
+            size -= len;
+        }
 
         got_output = !!out_pkt.size;