[FFmpeg-devel] [PATCH 1/7] avcodec/cbs: Check for overflow when reading

[Andriy Gelman]

On Mon, 09. Dec 23:25, Andreas Rheinhardt wrote:
> While CBS itself uses size_t for sizes, it relies on other APIs that use
> int for their sizes; in particular, AVBuffer uses int for their size
> parameters and so does GetBitContext with their number of bits. While
> CBS aims to be a safe API, the checks it employed were not sufficient to
> prevent overflows: E.g. if the size of a unit was > UINT_MAX / 8, 8 *
> said size may be truncated to a positive integer before being passed to
> init_get_bits() in which case its return value would not indicate an
> error.
> 
> These checks have been improved to really capture these kinds of errors;
> furthermore, although the sizes are still size_t, they are now de-facto
> restricted to 0..INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> The check in cbs_insert_unit() can currently not be triggered, because
> av_malloc_array makes sure that it doesn't allocate more than INT_MAX;
> so the allocation will fail long before we get even close to INT_MAX.
> 
> av1 and H.26x have not been changed, because their split functions
> already check the size (in case of H.264 and H.265 this happens in
> ff_h2645_packet_split()).
> 
> It would btw be possible to open the GetBitContext generically. The
> read_unit function would then get the already opened GetBitContext
> as a parameter.
> 
>  libavcodec/cbs.c       | 6 ++++++
>  libavcodec/cbs_jpeg.c  | 2 +-
>  libavcodec/cbs_mpeg2.c | 2 +-
>  libavcodec/cbs_vp9.c   | 2 +-
>  4 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 0badb192d9..805049404b 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -206,6 +206,9 @@ static int cbs_fill_fragment_data(CodedBitstreamContext *ctx,
>  {
>      av_assert0(!frag->data && !frag->data_ref);
>  
> +    if (size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
> +        return AVERROR(ERANGE);
> +
>      frag->data_ref =
>          av_buffer_alloc(size + AV_INPUT_BUFFER_PADDING_SIZE);
>      if (!frag->data_ref)
> @@ -693,6 +696,9 @@ static int cbs_insert_unit(CodedBitstreamContext *ctx,
>              memmove(units + position + 1, units + position,
>                      (frag->nb_units - position) * sizeof(*units));
>      } else {

> +        if (frag->nb_units == INT_MAX)
> +            return AVERROR(ERANGE);
> +

Why did you decide to include this? As you say in your comments it cannot be
triggered.

I looked over Patch 1-5 and they look good to me. 
Also, thanks for answering some clarifying questions I had on IRC. 

-- 
Andriy