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

James Almer jamrial at gmail.com
Wed May 12 23:23:04 EEST 2021


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 at 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.


More information about the ffmpeg-devel mailing list