diff mbox series

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

Message ID CAO4ZO8fb4-1EsufKtE8mHszKp7JZoJzJaaYRKYaM15XJyTkQjw@mail.gmail.com
State New
Headers show
Series [FFmpeg-devel] libavutil/imgutils: UBSan nullptr-with-offset in av_image_fill_pointer | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Brian Kim July 1, 2020, 6:14 p.m. UTC 
While running under Clang's UndefinedBehaviorSanitizer, I found a few
places where av_image_fill_pointers is called before buffers for the image
are allocated, so ptr is passed in as NULL.

This leads to (currently harmless) UB when the plane sizes are added to the
null pointer, so I was wondering if there was interest in avoiding it?

I've attached a patch to expose some extra utilities and avoid passing in
the null pointer, but is this an appropriate way to work around it?

Thank you,
Brian
Subject: [PATCH] libavutil/imgutils: add utility to get plane sizes

This utility helps avoid undefined behavior when doing things like
checking how much memory we need to allocate for an image before we have
allocated a buffer.
---
 libavcodec/decode.c  | 11 +++-------
 libavutil/frame.c    |  9 ++++----
 libavutil/imgutils.c | 49 ++++++++++++++++++++++++++++++++------------
 libavutil/imgutils.h | 22 ++++++++++++++++++++
 4 files changed, 65 insertions(+), 26 deletions(-)

Comments

Michael Niedermayer July 2, 2020, 3:06 p.m. UTC | #1 
On Wed, Jul 01, 2020 at 11:14:13AM -0700, Brian Kim wrote:
> While running under Clang's UndefinedBehaviorSanitizer, I found a few
> places where av_image_fill_pointers is called before buffers for the image
> are allocated, so ptr is passed in as NULL.
> 
> This leads to (currently harmless) UB when the plane sizes are added to the
> null pointer, so I was wondering if there was interest in avoiding it?
> 
> I've attached a patch to expose some extra utilities and avoid passing in
> the null pointer, but is this an appropriate way to work around it?

how can these cases be reproduced ?

thx

[...]
Brian Kim July 6, 2020, 7:12 p.m. UTC | #2 
I'm using clang 10.0.0 and configuring with with `configure --enable-gpl
--enable-libass --enable-libfdk-aac --enable-libmp3lame
--enable-libopencore-amrnb --enable-libopencore-amrwb --enable-librtmp
--enable-libtheora --enable-libvorbis --enable-libopus --enable-libx264
--enable-libvpx --enable-nonfree --enable-version3 --disable-optimizations
--disable-stripping --enable-debug=3 --toolchain=clang-usan` (plus
`--extra-cflags=-fno-sanitize-recover=pointer-overflow
--extra-ldflags=-fno-sanitize-recover=pointer-overflow` to crash when we
run into the UB)

When I run `make fate -j 12 SAMPLES=fate-suite`, several tests (e.g.
api-mjpeg-codec-param) fail with something like the following in the error
logs:

libavutil/imgutils.c:139:29: runtime error: applying non-zero offset 131072
to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
libavutil/imgutils.c:139:29 in

 It looks like this check was added in clang 10 (
https://reviews.llvm.org/D67122)

On Thu, Jul 2, 2020 at 8:06 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Jul 01, 2020 at 11:14:13AM -0700, Brian Kim wrote:
> > While running under Clang's UndefinedBehaviorSanitizer, I found a few
> > places where av_image_fill_pointers is called before buffers for the
> image
> > are allocated, so ptr is passed in as NULL.
> >
> > This leads to (currently harmless) UB when the plane sizes are added to
> the
> > null pointer, so I was wondering if there was interest in avoiding it?
> >
> > I've attached a patch to expose some extra utilities and avoid passing in
> > the null pointer, but is this an appropriate way to work around it?
>
> how can these cases be reproduced ?
>
> thx
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> The smallest minority on earth is the individual. Those who deny
> individual rights cannot claim to be defenders of minorities. - Ayn Rand
> _______________________________________________
> 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".
Michael Niedermayer July 7, 2020, 11:34 a.m. UTC | #3 
Hi

On Wed, Jul 01, 2020 at 11:14:13AM -0700, Brian Kim wrote:
> While running under Clang's UndefinedBehaviorSanitizer, I found a few
> places where av_image_fill_pointers is called before buffers for the image
> are allocated, so ptr is passed in as NULL.
> 
> This leads to (currently harmless) UB when the plane sizes are added to the
> null pointer, so I was wondering if there was interest in avoiding it?

There is certainly interrest in avoiding this


> 
> I've attached a patch to expose some extra utilities and avoid passing in
> the null pointer, but is this an appropriate way to work around it?
> 
> Thank you,
> Brian

>  libavcodec/decode.c  |   11 +++--------
>  libavutil/frame.c    |    9 ++++-----
>  libavutil/imgutils.c |   49 ++++++++++++++++++++++++++++++++++++-------------
>  libavutil/imgutils.h |   22 ++++++++++++++++++++++
>  4 files changed, 65 insertions(+), 26 deletions(-)
> 1807613b16b290ccac0574586b42e13230dc824b  0001-libavutil-imgutils-add-utility-to-get-plane-sizes.patch
> From 9db97425b2b3ca0913b7ce8f6f7c096a8aa5c964 Mon Sep 17 00:00:00 2001
> From: Brian Kim <bkkim@google.com>
> Date: Tue, 30 Jun 2020 17:59:53 -0700
> Subject: [PATCH] libavutil/imgutils: add utility to get plane sizes
> 
> This utility helps avoid undefined behavior when doing things like
> checking how much memory we need to allocate for an image before we have
> allocated a buffer.
> ---

>  libavcodec/decode.c  | 11 +++-------
>  libavutil/frame.c    |  9 ++++----
>  libavutil/imgutils.c | 49 ++++++++++++++++++++++++++++++++------------
>  libavutil/imgutils.h | 22 ++++++++++++++++++++

the changes to libavcodec and libavutil need to be in seperate patches
API additions in libavutil have to also bump the minor version and add
an entry in doc/APIchanges


[...]

> diff --git a/libavutil/imgutils.h b/libavutil/imgutils.h
> index 5b790ecf0a..cbdef12928 100644
> --- a/libavutil/imgutils.h
> +++ b/libavutil/imgutils.h
> @@ -67,6 +67,28 @@ int av_image_get_linesize(enum AVPixelFormat pix_fmt, int width, int plane);
>   */
>  int av_image_fill_linesizes(int linesizes[4], enum AVPixelFormat pix_fmt, int width);
>  
> +/**
> + * Fill plane sizes for an image with pixel format pix_fmt and height height.
> + *
> + * @param size the array to be filled with the size of each image plane
> + * @param linesizes the array containing the linesize for each
> + * plane, should be filled by av_image_fill_linesizes()
> + * @return the size in bytes required for the image buffer, a negative
> + * error code in case of failure
> + */
> +int av_image_fill_plane_sizes(int size[4], enum AVPixelFormat pix_fmt, int height,
> +                           const int linesizes[4]);
> +
> +/**
> + * Fill plane data pointers for an image with plane sizes size.
> + *
> + * @param data pointers array to be filled with the pointer for each image plane
> + * @param size the array containing the size of each plane, should be filled
> + * by av_image_fill_plane_sizes()
> + * @param ptr the pointer to a buffer which will contain the image
> + */
> +void av_image_fill_pointers_from_sizes(uint8_t *data[4], int size[4], uint8_t *ptr);

I wonder if linesizes for newly added functions should be ptrdiff_t
this would add some type converting loops though

And size probably should be ptrdiff_t or int64_t to similarly be more future
proof 

thx

[...]
James Almer July 7, 2020, 1:34 p.m. UTC | #4 
On 7/1/2020 3:14 PM, Brian Kim wrote:
> While running under Clang's UndefinedBehaviorSanitizer, I found a few
> places where av_image_fill_pointers is called before buffers for the image
> are allocated, so ptr is passed in as NULL.
> 
> This leads to (currently harmless) UB when the plane sizes are added to the
> null pointer, so I was wondering if there was interest in avoiding it?
> 
> I've attached a patch to expose some extra utilities and avoid passing in
> the null pointer, but is this an appropriate way to work around it?

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);

But i like the new av_image_fill_plane_sizes() function you introduced.
av_image_fill_pointers_from_sizes() however not so much. Its only
purpose is to be called after av_image_fill_plane_sizes(), and there's
basically no other scenario where you could use it.

More comments below.

[...]

> From 9db97425b2b3ca0913b7ce8f6f7c096a8aa5c964 Mon Sep 17 00:00:00 2001
> From: Brian Kim <bkkim@google.com>
> Date: Tue, 30 Jun 2020 17:59:53 -0700
> Subject: [PATCH] libavutil/imgutils: add utility to get plane sizes
> 
> This utility helps avoid undefined behavior when doing things like
> checking how much memory we need to allocate for an image before we have
> allocated a buffer.
> ---
>  libavcodec/decode.c  | 11 +++-------
>  libavutil/frame.c    |  9 ++++----
>  libavutil/imgutils.c | 49 ++++++++++++++++++++++++++++++++------------
>  libavutil/imgutils.h | 22 ++++++++++++++++++++
>  4 files changed, 65 insertions(+), 26 deletions(-)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index de9c079f9d..cd0424b467 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1471,9 +1471,8 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
>  
>      switch (avctx->codec_type) {
>      case AVMEDIA_TYPE_VIDEO: {
> -        uint8_t *data[4];
>          int linesize[4];
> -        int size[4] = { 0 };
> +        int size[4];
>          int w = frame->width;
>          int h = frame->height;
>          int tmpsize, unaligned;
> @@ -1494,17 +1493,13 @@ static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
>                  unaligned |= linesize[i] % pool->stride_align[i];
>          } while (unaligned);
>  
> -        tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h,
> -                                         NULL, linesize);
> +        tmpsize = av_image_fill_plane_sizes(size, avctx->pix_fmt, h,
> +                                         linesize);
>          if (tmpsize < 0) {
>              ret = tmpsize;
>              goto fail;
>          }
>  
> -        for (i = 0; i < 3 && data[i + 1]; i++)
> -            size[i] = data[i + 1] - data[i];
> -        size[i] = tmpsize - (data[i] - data[0]);
> -
>          for (i = 0; i < 4; i++) {
>              pool->linesize[i] = linesize[i];
>              if (size[i]) {
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 9884eae054..3abb1f12d5 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -212,6 +212,7 @@ void av_frame_free(AVFrame **frame)
>  static int get_video_buffer(AVFrame *frame, int align)
>  {
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
> +    int size[4];
>      int ret, i, padded_height;
>      int plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align);
>  
> @@ -239,8 +240,8 @@ static int get_video_buffer(AVFrame *frame, int align)
>      }
>  
>      padded_height = FFALIGN(frame->height, 32);
> -    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
> -                                      NULL, frame->linesize)) < 0)
> +    if ((ret = av_image_fill_plane_sizes(size, frame->format, padded_height,
> +                                      frame->linesize)) < 0)
>          return ret;
>  
>      frame->buf[0] = av_buffer_alloc(ret + 4*plane_padding);
> @@ -249,9 +250,7 @@ static int get_video_buffer(AVFrame *frame, int align)
>          goto fail;
>      }
>  
> -    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
> -                                      frame->buf[0]->data, frame->linesize)) < 0)
> -        goto fail;
> +    av_image_fill_pointers_from_sizes(frame->data, size, frame->buf[0]->data);
>  
>      for (i = 1; i < 4; i++) {
>          if (frame->data[i])
> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> index 7f9c1b632c..7045a9df65 100644
> --- a/libavutil/imgutils.c
> +++ b/libavutil/imgutils.c
> @@ -108,26 +108,25 @@ int av_image_fill_linesizes(int linesizes[4], enum AVPixelFormat pix_fmt, int wi
>      return 0;
>  }
>  
> -int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int height,
> -                           uint8_t *ptr, const int linesizes[4])
> +int av_image_fill_plane_sizes(int size[4], enum AVPixelFormat pix_fmt, int height,
> +                           const int linesizes[4])
>  {
> -    int i, total_size, size[4] = { 0 }, has_plane[4] = { 0 };
> +    int i, total_size, has_plane[4] = { 0 };
>  
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt);
> -    memset(data     , 0, sizeof(data[0])*4);
> +    memset(size     , 0, sizeof(size[0])*4);
>  
>      if (!desc || desc->flags & AV_PIX_FMT_FLAG_HWACCEL)
>          return AVERROR(EINVAL);
>  
> -    data[0] = ptr;
>      if (linesizes[0] > (INT_MAX - 1024) / height)
>          return AVERROR(EINVAL);
>      size[0] = linesizes[0] * height;
>  
>      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 */
> -        return size[0] + 256 * 4;
> +        size[1] = 256 * 4; /* palette is stored here as 256 32 bits words */
> +        return size[0] + size[1];
>      }
>  
>      for (i = 0; i < 4; i++)
> @@ -136,7 +135,6 @@ 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];
>          h = (height + (1 << s) - 1) >> s;
>          if (linesizes[i] > INT_MAX / h)
>              return AVERROR(EINVAL);
> @@ -149,6 +147,31 @@ int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei
>      return total_size;
>  }
>  
> +void av_image_fill_pointers_from_sizes(uint8_t *data[4], int size[4], uint8_t *ptr)
> +{
> +    int i;
> +
> +    memset(data , 0, sizeof(data[0])*4);
> +
> +    data[0] = ptr;
> +    for (i = 1; i < 4 && size[i - 1] > 0; i++)
> +        data[i] = data[i - 1] + size[i - 1];

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.

> +}
> +
> +int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int height,
> +                           uint8_t *ptr, const int linesizes[4]) {
> +    int size[4];
> +    int ret;
> +
> +    ret = av_image_fill_plane_sizes(size, pix_fmt, height, linesizes);
> +    if (ret < 0)
> +        return ret;
> +
> +    av_image_fill_pointers_from_sizes(data, size, ptr);

I'd rather not add this function and instead just put the relevant code
here.

> +
> +    return ret;
> +}
> +
>  int avpriv_set_systematic_pal2(uint32_t pal[256], enum AVPixelFormat pix_fmt)
>  {
>      int i;
> @@ -194,6 +217,7 @@ int av_image_alloc(uint8_t *pointers[4], int linesizes[4],
>  {
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt);
>      int i, ret;
> +    int size[4];
>      uint8_t *buf;
>  
>      if (!desc)
> @@ -207,19 +231,18 @@ int av_image_alloc(uint8_t *pointers[4], int linesizes[4],
>      for (i = 0; i < 4; i++)
>          linesizes[i] = FFALIGN(linesizes[i], align);
>  
> -    if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, NULL, linesizes)) < 0)
> +    if ((ret = av_image_fill_plane_sizes(size, pix_fmt, h, linesizes)) < 0)
>          return ret;
>      buf = av_malloc(ret + align);
>      if (!buf)
>          return AVERROR(ENOMEM);
> -    if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, buf, linesizes)) < 0) {
> -        av_free(buf);
> -        return ret;
> -    }
> +    av_image_fill_pointers_from_sizes(pointers, size, buf);
> +
>      if (desc->flags & AV_PIX_FMT_FLAG_PAL || (desc->flags & FF_PSEUDOPAL && pointers[1])) {
>          avpriv_set_systematic_pal2((uint32_t*)pointers[1], pix_fmt);
>          if (align < 4) {
>              av_log(NULL, AV_LOG_ERROR, "Formats with a palette require a minimum alignment of 4\n");
> +            av_free(buf);
>              return AVERROR(EINVAL);
>          }
>      }
> diff --git a/libavutil/imgutils.h b/libavutil/imgutils.h
> index 5b790ecf0a..cbdef12928 100644
> --- a/libavutil/imgutils.h
> +++ b/libavutil/imgutils.h
> @@ -67,6 +67,28 @@ int av_image_get_linesize(enum AVPixelFormat pix_fmt, int width, int plane);
>   */
>  int av_image_fill_linesizes(int linesizes[4], enum AVPixelFormat pix_fmt, int width);
>  
> +/**
> + * Fill plane sizes for an image with pixel format pix_fmt and height height.
> + *
> + * @param size the array to be filled with the size of each image plane
> + * @param linesizes the array containing the linesize for each
> + * plane, should be filled by av_image_fill_linesizes()
> + * @return the size in bytes required for the image buffer, a negative
> + * error code in case of failure
> + */
> +int av_image_fill_plane_sizes(int size[4], enum AVPixelFormat pix_fmt, int height,
> +                           const int linesizes[4]);
> +
> +/**
> + * Fill plane data pointers for an image with plane sizes size.
> + *
> + * @param data pointers array to be filled with the pointer for each image plane
> + * @param size the array containing the size of each plane, should be filled
> + * by av_image_fill_plane_sizes()
> + * @param ptr the pointer to a buffer which will contain the image
> + */
> +void av_image_fill_pointers_from_sizes(uint8_t *data[4], int size[4], uint8_t *ptr);
> +
>  /**
>   * Fill plane data pointers for an image with pixel format pix_fmt and
>   * height height.
> -- 
> 2.27.0.212.ge8ba1cc988-goog
>
Brian Kim July 7, 2020, 8:07 p.m. UTC | #5 
On Tue, Jul 7, 2020 at 6:34 AM James Almer <jamrial@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?
Brian Kim July 7, 2020, 8:14 p.m. UTC | #6 
On Tue, Jul 7, 2020 at 4:35 AM Michael Niedermayer
<michael@niedermayer.cc> wrote:
[...]
> I wonder if linesizes for newly added functions should be ptrdiff_t
> this would add some type converting loops though
>
> And size probably should be ptrdiff_t or int64_t to similarly be more future
> proof

Can these values be negative? Or is there some other reason not to use size_t?

[...]
James Almer July 7, 2020, 8:41 p.m. UTC | #7 
On 7/7/2020 5:07 PM, Brian Kim wrote:
> On Tue, Jul 7, 2020 at 6:34 AM James Almer <jamrial@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.
Michael Niedermayer July 8, 2020, 7:37 p.m. UTC | #8 
On Tue, Jul 07, 2020 at 01:14:41PM -0700, Brian Kim wrote:
> On Tue, Jul 7, 2020 at 4:35 AM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> [...]
> > I wonder if linesizes for newly added functions should be ptrdiff_t
> > this would add some type converting loops though
> >
> > And size probably should be ptrdiff_t or int64_t to similarly be more future
> > proof
> 
> Can these values be negative? Or is there some other reason not to use size_t?

size should not be negative, size_t is a option for that
linesize in principle can be negative

thx

[...]
Michael Niedermayer July 8, 2020, 7:44 p.m. UTC | #9 
On Tue, Jul 07, 2020 at 05:41:11PM -0300, James Almer wrote:
> On 7/7/2020 5:07 PM, Brian Kim wrote:
> > On Tue, Jul 7, 2020 at 6:34 AM James Almer <jamrial@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.

If you break the size = data0 - data1 usecase then
you must bump the major abi version because if you dont then
distros will have things break and this is our own libs that break
because they use this before we fix it.

I hope iam wrong, of course because this is a mess 

thx

[...]
James Almer July 8, 2020, 8:07 p.m. UTC | #10 
On 7/8/2020 4:44 PM, Michael Niedermayer wrote:
> On Tue, Jul 07, 2020 at 05:41:11PM -0300, James Almer wrote:
>> On 7/7/2020 5:07 PM, Brian Kim wrote:
>>> On Tue, Jul 7, 2020 at 6:34 AM James Almer <jamrial@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.
> 
> If you break the size = data0 - data1 usecase then
> you must bump the major abi version because if you dont then
> distros will have things break and this is our own libs that break
> because they use this before we fix it.

Leaving *data[4] zeroed when ptr == NULL would only break size = data1 -
data0 for prt == NULL. And if this is UB, then we can't even be sure the
above is guaranteed with every compiler.

In any case, I'm fine with postponing that change until after a bump. As
i said it should be a matter of adding a simple check.
The addition of av_image_fill_plane_sizes() should be enough for now.

> 
> I hope iam wrong, of course because this is a mess 
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
diff mbox series

Patch

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index de9c079f9d..cd0424b467 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -1471,9 +1471,8 @@  static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
 
     switch (avctx->codec_type) {
     case AVMEDIA_TYPE_VIDEO: {
-        uint8_t *data[4];
         int linesize[4];
-        int size[4] = { 0 };
+        int size[4];
         int w = frame->width;
         int h = frame->height;
         int tmpsize, unaligned;
@@ -1494,17 +1493,13 @@  static int update_frame_pool(AVCodecContext *avctx, AVFrame *frame)
                 unaligned |= linesize[i] % pool->stride_align[i];
         } while (unaligned);
 
-        tmpsize = av_image_fill_pointers(data, avctx->pix_fmt, h,
-                                         NULL, linesize);
+        tmpsize = av_image_fill_plane_sizes(size, avctx->pix_fmt, h,
+                                         linesize);
         if (tmpsize < 0) {
             ret = tmpsize;
             goto fail;
         }
 
-        for (i = 0; i < 3 && data[i + 1]; i++)
-            size[i] = data[i + 1] - data[i];
-        size[i] = tmpsize - (data[i] - data[0]);
-
         for (i = 0; i < 4; i++) {
             pool->linesize[i] = linesize[i];
             if (size[i]) {
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 9884eae054..3abb1f12d5 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -212,6 +212,7 @@  void av_frame_free(AVFrame **frame)
 static int get_video_buffer(AVFrame *frame, int align)
 {
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
+    int size[4];
     int ret, i, padded_height;
     int plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align);
 
@@ -239,8 +240,8 @@  static int get_video_buffer(AVFrame *frame, int align)
     }
 
     padded_height = FFALIGN(frame->height, 32);
-    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
-                                      NULL, frame->linesize)) < 0)
+    if ((ret = av_image_fill_plane_sizes(size, frame->format, padded_height,
+                                      frame->linesize)) < 0)
         return ret;
 
     frame->buf[0] = av_buffer_alloc(ret + 4*plane_padding);
@@ -249,9 +250,7 @@  static int get_video_buffer(AVFrame *frame, int align)
         goto fail;
     }
 
-    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
-                                      frame->buf[0]->data, frame->linesize)) < 0)
-        goto fail;
+    av_image_fill_pointers_from_sizes(frame->data, size, frame->buf[0]->data);
 
     for (i = 1; i < 4; i++) {
         if (frame->data[i])
diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
index 7f9c1b632c..7045a9df65 100644
--- a/libavutil/imgutils.c
+++ b/libavutil/imgutils.c
@@ -108,26 +108,25 @@  int av_image_fill_linesizes(int linesizes[4], enum AVPixelFormat pix_fmt, int wi
     return 0;
 }
 
-int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int height,
-                           uint8_t *ptr, const int linesizes[4])
+int av_image_fill_plane_sizes(int size[4], enum AVPixelFormat pix_fmt, int height,
+                           const int linesizes[4])
 {
-    int i, total_size, size[4] = { 0 }, has_plane[4] = { 0 };
+    int i, total_size, has_plane[4] = { 0 };
 
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt);
-    memset(data     , 0, sizeof(data[0])*4);
+    memset(size     , 0, sizeof(size[0])*4);
 
     if (!desc || desc->flags & AV_PIX_FMT_FLAG_HWACCEL)
         return AVERROR(EINVAL);
 
-    data[0] = ptr;
     if (linesizes[0] > (INT_MAX - 1024) / height)
         return AVERROR(EINVAL);
     size[0] = linesizes[0] * height;
 
     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 */
-        return size[0] + 256 * 4;
+        size[1] = 256 * 4; /* palette is stored here as 256 32 bits words */
+        return size[0] + size[1];
     }
 
     for (i = 0; i < 4; i++)
@@ -136,7 +135,6 @@  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];
         h = (height + (1 << s) - 1) >> s;
         if (linesizes[i] > INT_MAX / h)
             return AVERROR(EINVAL);
@@ -149,6 +147,31 @@  int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int hei
     return total_size;
 }
 
+void av_image_fill_pointers_from_sizes(uint8_t *data[4], int size[4], uint8_t *ptr)
+{
+    int i;
+
+    memset(data , 0, sizeof(data[0])*4);
+
+    data[0] = ptr;
+    for (i = 1; i < 4 && size[i - 1] > 0; i++)
+        data[i] = data[i - 1] + size[i - 1];
+}
+
+int av_image_fill_pointers(uint8_t *data[4], enum AVPixelFormat pix_fmt, int height,
+                           uint8_t *ptr, const int linesizes[4]) {
+    int size[4];
+    int ret;
+
+    ret = av_image_fill_plane_sizes(size, pix_fmt, height, linesizes);
+    if (ret < 0)
+        return ret;
+
+    av_image_fill_pointers_from_sizes(data, size, ptr);
+
+    return ret;
+}
+
 int avpriv_set_systematic_pal2(uint32_t pal[256], enum AVPixelFormat pix_fmt)
 {
     int i;
@@ -194,6 +217,7 @@  int av_image_alloc(uint8_t *pointers[4], int linesizes[4],
 {
     const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(pix_fmt);
     int i, ret;
+    int size[4];
     uint8_t *buf;
 
     if (!desc)
@@ -207,19 +231,18 @@  int av_image_alloc(uint8_t *pointers[4], int linesizes[4],
     for (i = 0; i < 4; i++)
         linesizes[i] = FFALIGN(linesizes[i], align);
 
-    if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, NULL, linesizes)) < 0)
+    if ((ret = av_image_fill_plane_sizes(size, pix_fmt, h, linesizes)) < 0)
         return ret;
     buf = av_malloc(ret + align);
     if (!buf)
         return AVERROR(ENOMEM);
-    if ((ret = av_image_fill_pointers(pointers, pix_fmt, h, buf, linesizes)) < 0) {
-        av_free(buf);
-        return ret;
-    }
+    av_image_fill_pointers_from_sizes(pointers, size, buf);
+
     if (desc->flags & AV_PIX_FMT_FLAG_PAL || (desc->flags & FF_PSEUDOPAL && pointers[1])) {
         avpriv_set_systematic_pal2((uint32_t*)pointers[1], pix_fmt);
         if (align < 4) {
             av_log(NULL, AV_LOG_ERROR, "Formats with a palette require a minimum alignment of 4\n");
+            av_free(buf);
             return AVERROR(EINVAL);
         }
     }
diff --git a/libavutil/imgutils.h b/libavutil/imgutils.h
index 5b790ecf0a..cbdef12928 100644
--- a/libavutil/imgutils.h
+++ b/libavutil/imgutils.h
@@ -67,6 +67,28 @@  int av_image_get_linesize(enum AVPixelFormat pix_fmt, int width, int plane);
  */
 int av_image_fill_linesizes(int linesizes[4], enum AVPixelFormat pix_fmt, int width);
 
+/**
+ * Fill plane sizes for an image with pixel format pix_fmt and height height.
+ *
+ * @param size the array to be filled with the size of each image plane
+ * @param linesizes the array containing the linesize for each
+ * plane, should be filled by av_image_fill_linesizes()
+ * @return the size in bytes required for the image buffer, a negative
+ * error code in case of failure
+ */
+int av_image_fill_plane_sizes(int size[4], enum AVPixelFormat pix_fmt, int height,
+                           const int linesizes[4]);
+
+/**
+ * Fill plane data pointers for an image with plane sizes size.
+ *
+ * @param data pointers array to be filled with the pointer for each image plane
+ * @param size the array containing the size of each plane, should be filled
+ * by av_image_fill_plane_sizes()
+ * @param ptr the pointer to a buffer which will contain the image
+ */
+void av_image_fill_pointers_from_sizes(uint8_t *data[4], int size[4], uint8_t *ptr);
+
 /**
  * Fill plane data pointers for an image with pixel format pix_fmt and
  * height height.