diff mbox series

[FFmpeg-devel] avutil/imgutils: don't add offsets to NULL pointers

Message ID 20210503040743.54346-1-jamrial@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel] avutil/imgutils: don't add offsets to NULL pointers
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

James Almer May 3, 2021, 4:07 a.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/imgutils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Rheinhardt May 4, 2021, 8:13 p.m. UTC | #1
James Almer:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavutil/imgutils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> index 53faad889a..aaee0dfb7a 100644
> --- a/libavutil/imgutils.c
> +++ b/libavutil/imgutils.c
> @@ -166,7 +166,7 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei
>      }
>  
>      data[0] = ptr;
> -    for (i = 1; i < 4 && sizes[i]; i++)
> +    for (i = 1; i < 4 && data[i - 1] && sizes[i]; i++)
>          data[i] = data[i - 1] + sizes[i - 1];
>  
>      return ret;
> I see two ways to make this a NULL + offset: First, if ptr == NULL; and
second if data[i - 1] + sizes[i - 1] no longer fits into the allocated
buffer and happens to yield NULL (very unlikely, but possible) in which
case data[i] + sizes[i] would be NULL + offset. In the second case, the
first addition is already undefined behaviour against which we cannot
guard at all: We don't know the size of the buffer. The only thing we
can guard against is ptr being NULL; we can even error out in this
scenario, but I don't know how disruptive that would be.
Notice that in C the result of pointer + offset can never be NULL, so a
compiler could optimize the check for data[i - 1] to just a check for ptr.

- Andreas
James Almer May 4, 2021, 8:50 p.m. UTC | #2
On 5/4/2021 5:13 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavutil/imgutils.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
>> index 53faad889a..aaee0dfb7a 100644
>> --- a/libavutil/imgutils.c
>> +++ b/libavutil/imgutils.c
>> @@ -166,7 +166,7 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei
>>       }
>>   
>>       data[0] = ptr;
>> -    for (i = 1; i < 4 && sizes[i]; i++)
>> +    for (i = 1; i < 4 && data[i - 1] && sizes[i]; i++)
>>           data[i] = data[i - 1] + sizes[i - 1];
>>   
>>       return ret;
>> I see two ways to make this a NULL + offset: First, if ptr == NULL; and
> second if data[i - 1] + sizes[i - 1] no longer fits into the allocated
> buffer and happens to yield NULL (very unlikely, but possible) in which
> case data[i] + sizes[i] would be NULL + offset. In the second case, the
> first addition is already undefined behaviour against which we cannot
> guard at all: We don't know the size of the buffer. The only thing we
> can guard against is ptr being NULL; we can even error out in this
> scenario, but I don't know how disruptive that would be.

That'd be an undesirable breakage, yes. Aside from filling data[], the 
function also returns the size of the buffer that should be allocated, 
so that functionality should remain even when ptr == NULL.

> Notice that in C the result of pointer + offset can never be NULL, so a
> compiler could optimize the check for data[i - 1] to just a check for ptr.

If you say there's no warranty that an scenario where data[i-1] + 
size[i-1] == NULL will break the for loop in the next iteration, and no 
way to guard against it at all, then we can just return right before 
attempting to set data[] when ptr == NULL, and at least simplify that 
scenario.

> 
> - Andreas
> _______________________________________________
> 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 May 12, 2021, 8:23 p.m. UTC | #3
On 5/4/2021 5:50 PM, James Almer wrote:
> On 5/4/2021 5:13 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>   libavutil/imgutils.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
>>> index 53faad889a..aaee0dfb7a 100644
>>> --- a/libavutil/imgutils.c
>>> +++ b/libavutil/imgutils.c
>>> @@ -166,7 +166,7 @@ int av_image_fill_pointers(uint8_t *data[4], enum 
>>> AVPixelFormat pix_fmt, int hei
>>>       }
>>>       data[0] = ptr;
>>> -    for (i = 1; i < 4 && sizes[i]; i++)
>>> +    for (i = 1; i < 4 && data[i - 1] && sizes[i]; i++)
>>>           data[i] = data[i - 1] + sizes[i - 1];
>>>       return ret;
>>> I see two ways to make this a NULL + offset: First, if ptr == NULL; and
>> second if data[i - 1] + sizes[i - 1] no longer fits into the allocated
>> buffer and happens to yield NULL (very unlikely, but possible) in which
>> case data[i] + sizes[i] would be NULL + offset. In the second case, the
>> first addition is already undefined behaviour against which we cannot
>> guard at all: We don't know the size of the buffer. The only thing we
>> can guard against is ptr being NULL; we can even error out in this
>> scenario, but I don't know how disruptive that would be.
> 
> That'd be an undesirable breakage, yes. Aside from filling data[], the 
> function also returns the size of the buffer that should be allocated, 
> so that functionality should remain even when ptr == NULL.
> 
>> Notice that in C the result of pointer + offset can never be NULL, so a
>> compiler could optimize the check for data[i - 1] to just a check for 
>> ptr.
> 
> If you say there's no warranty that an scenario where data[i-1] + 
> size[i-1] == NULL will break the for loop in the next iteration, and no 
> way to guard against it at all, then we can just return right before 
> attempting to set data[] when ptr == NULL, and at least simplify that 
> scenario.

Made that change and pushed.
diff mbox series

Patch

diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
index 53faad889a..aaee0dfb7a 100644
--- a/libavutil/imgutils.c
+++ b/libavutil/imgutils.c
@@ -166,7 +166,7 @@  int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei
     }
 
     data[0] = ptr;
-    for (i = 1; i < 4 && sizes[i]; i++)
+    for (i = 1; i < 4 && data[i - 1] && sizes[i]; i++)
         data[i] = data[i - 1] + sizes[i - 1];
 
     return ret;