[FFmpeg-devel] [PATCH v2 1/3] libavutil/imgutils: add utility to get plane sizes

[James Almer]

On 7/8/2020 10:50 PM, Brian Kim wrote:
> Patch attached.
> 
> Main changes from v1 are switching to from int to size_t/ptrdiff_t
> where relevant and removing av_image_fill_pointers_from_sizes()
> 
> Some things to note:
>  - The av_image_fill_planes_sizes uses size_t/ptrdiff_t for buffer
> sizes, but as mentioned during the v1 review, this leads to some
> inconvenient conversions and range checks when using it with existing
> functions. We could keep the return type as int to alleviate this, but
> that seems like it would somewhat defeat the purpose of moving to
> these types.
>  - SIZE_MAX is used in several places in libavutil, so I used
> PTRDIFF_MAX, but I could not see any mention of these being allowed.

[...]

> From a47b3f3b5c2ed4a1caf9f0a3429dd425ce708bb1 Mon Sep 17 00:00:00 2001
> From: Brian Kim <bkkim at google.com>
> Date: Tue, 7 Jul 2020 11:36:19 -0700
> Subject: [PATCH 1/3] 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.
> 
> Signed-off-by: Brian Kim <bkkim at google.com>
> ---
>  doc/APIchanges       |  3 ++
>  libavutil/frame.c    | 15 ++++++---
>  libavutil/imgutils.c | 74 ++++++++++++++++++++++++++++++++------------
>  libavutil/imgutils.h | 12 +++++++
>  libavutil/version.h  |  2 +-
>  5 files changed, 82 insertions(+), 24 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 1d6cc36b8c..44defd9ca8 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2020-07-xx - xxxxxxxxxx - lavu 56.56.100 - imgutils.h
> +  Add av_image_fill_plane_sizes().
> +
>  2020-06-12 - b09fb030c1 - lavu 56.55.100 - pixdesc.h
>    Add AV_PIX_FMT_X2RGB10.
>  
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 9884eae054..b664dcfbe8 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c

Changes to frame.c should be in a separate commit as well.

> @@ -214,6 +214,8 @@ static int get_video_buffer(AVFrame *frame, int align)
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(frame->format);
>      int ret, i, padded_height;
>      int plane_padding = FFMAX(16 + 16/*STRIDE_ALIGN*/, align);
> +    ptrdiff_t total_size, linesizes[4];
> +    size_t size[4];
>  
>      if (!desc)
>          return AVERROR(EINVAL);
> @@ -238,12 +240,17 @@ static int get_video_buffer(AVFrame *frame, int align)
>              frame->linesize[i] = FFALIGN(frame->linesize[i], align);
>      }
>  
> +    for (i = 0; i < 4; i++)
> +        linesizes[i] = frame->linesize[i];
> +
>      padded_height = FFALIGN(frame->height, 32);
> -    if ((ret = av_image_fill_pointers(frame->data, frame->format, padded_height,
> -                                      NULL, frame->linesize)) < 0)
> -        return ret;
> +    if ((total_size = av_image_fill_plane_sizes(size, frame->format, padded_height,
> +                                                linesizes)) < 0)
> +        return total_size;
> +    if (total_size > INT_MAX - 4*plane_padding)
> +        return AVERROR(EINVAL);
>  
> -    frame->buf[0] = av_buffer_alloc(ret + 4*plane_padding);
> +    frame->buf[0] = av_buffer_alloc(total_size + 4*plane_padding);
>      if (!frame->buf[0]) {
>          ret = AVERROR(ENOMEM);
>          goto fail;
> diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> index 7f9c1b632c..082229cfaf 100644
> --- a/libavutil/imgutils.c
> +++ b/libavutil/imgutils.c
> @@ -108,26 +108,26 @@ 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])
> +ptrdiff_t av_image_fill_plane_sizes(size_t size[4], enum AVPixelFormat pix_fmt,

The sum of all sizes should be size_t if each size is a size_t. But if
you do that you can't return error values, so i'm not sure what to
suggest other than just stick to ints for both (ptrdiff_t linesizes
should be ok).
I'd like to hear Michael's opinion about it.

For that matter, as it is right now i think this would be the first
function that returns ptrdiff_t to signal AVERROR values.

> +                                    int height, const ptrdiff_t linesizes[4])
>  {
> -    int i, total_size, size[4] = { 0 }, has_plane[4] = { 0 };
> +    int i, has_plane[4] = { 0 };
> +    ptrdiff_t total_size;
>  
>      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)
> +    if (linesizes[0] > (PTRDIFF_MAX - 1024) / height)

This check would need to ensure the calculation below fits in a size_t
instead.

Same for other similar calculations in the function.

>          return AVERROR(EINVAL);
>      size[0] = linesizes[0] * height;