On 7/7/2020 5:07 PM, Brian Kim wrote:
> On Tue, Jul 7, 2020 at 6:34 AM James Almer <jamr...@gmail.com> wrote:
>>
>> If i understand this right, you could easily solve it with just the
>> following changes:
>>
>>> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
>>> index 7f9c1b632c..48a373db01 100644
>>> --- a/libavutil/imgutils.c
>>> +++ b/libavutil/imgutils.c
>>> @@ -126,7 +126,8 @@ int av_image_fill_pointers(uint8_t *data[4], enum
>>> AVPixelFormat pix_fmt, int hei
>>>
>>> if (desc->flags & AV_PIX_FMT_FLAG_PAL ||
>>> desc->flags & FF_PSEUDOPAL) {
>>> - data[1] = ptr + size[0]; /* palette is stored here as 256 32 bits
>>> words */
>>> + if (ptr)
>>> + data[1] = ptr + size[0]; /* palette is stored here as 256 32
>>> bits words */
>>> return size[0] + 256 * 4;
>>> }
>>>
>>> @@ -136,7 +137,8 @@ int av_image_fill_pointers(uint8_t *data[4], enum
>>> AVPixelFormat pix_fmt, int hei
>>> total_size = size[0];
>>> for (i = 1; i < 4 && has_plane[i]; i++) {
>>> int h, s = (i == 1 || i == 2) ? desc->log2_chroma_h : 0;
>>> - data[i] = data[i-1] + size[i-1];
>>> + if (data[i - 1])
>>> + data[i] = data[i - 1] + size[i - 1];
>>> h = (height + (1 << s) - 1) >> s;
>>> if (linesizes[i] > INT_MAX / h)
>>> return AVERROR(EINVAL);
>
> Do we have to worry about backwards compatibility here? Some places
> (e.g. libavcodec/decode.c:1497) have been using data to calculate the
> sizes.
That code depended on undefined behavior, for both C and the
av_image_fill_pointers() function. So IMO no, we don't need to worry
about it.
>
>> You fixed the issue in decode.c and frame.c by replacing calls to
>> av_image_fill_pointers() with av_image_fill_plane_sizes(), but any other
>> existing av_image_fill_pointers() call with prt == NULL (Think library
>> users) will still trigger this UB even after this change.
>
> Same as above. Do we need to consider compatibility if we add a null
> check at the beginning or when we fill data?
Again, IMO no.
Ideally, an UB fixing change in imgutils.c would nonetheless be
committed after the addition of av_image_fill_plane_sizes(). After that
change it should be a matter of adding a simple check in
av_image_fill_pointers() right before filling the pointers, so if other
devs prefer to keep the current behavior, then such change is simply not
committed.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
