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 <bk...@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
>
_______________________________________________
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".
