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 |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
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 [...]
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".
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 [...]
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 >
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?
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? [...]
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.
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 [...]
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 [...]
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 --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.