[FFmpeg-devel] libavutil/imgutils: UBSan nullptr-with-offset in av_image_fill_pointer

[Brian Kim]

On Tue, Jul 7, 2020 at 6:34 AM James Almer <jamrial at 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.

> 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?