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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200707/1a29ea12/attachment.sig>