[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