[FFmpeg-devel] [PATCH 1/2] avcodec/cbs: Fix potential double-free when adding unit fails

[Andreas Rheinhardt]

On Mon, Nov 18, 2019 at 8:48 AM Andreas Rheinhardt <
andreas.rheinhardt at gmail.com> wrote:

> ff_cbs_insert_unit_data() has two modes of operation: It can insert a
> unit with a newly created reference to an already existing AVBuffer; or
> it can take a buffer and create an AVBuffer for it. Said buffer will
> then become owned by the unit lateron.
>
> A potential memleak/double-free exists in the second case, because if
> creating the AVBuffer fails, the function immediately returns, but when
> it fails lateron, the supplied buffer will be freed. The caller has no
> way to distinguish between these two outcomes. The only such caller
> (cbs_jpeg_split_fragment() in cbs_jpeg.c) opted for a potential
> double-free.
>
> This commit changes this by explicitly stating that a non-refcounted
> buffer will be freed on error. The aforementioned caller has been
> brought in line with this.
>
> Fixes CID 1452623.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> Actually CID 1452623 is a false positive: Coverity thinks that the
> frsgment's data buffer is NULL, which it never is (or we wouldn't be
> here).
>
>  libavcodec/cbs.c      | 5 ++++-
>  libavcodec/cbs.h      | 3 ++-
>  libavcodec/cbs_jpeg.c | 5 +----
>  3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 0badb192d9..0bd5e1ac5d 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -775,8 +775,11 @@ int ff_cbs_insert_unit_data(CodedBitstreamContext
> *ctx,
>          data_ref = av_buffer_ref(data_buf);
>      else
>          data_ref = av_buffer_create(data, data_size, NULL, NULL, 0);
> -    if (!data_ref)
> +    if (!data_ref) {
> +        if (!data_buf)
> +            av_free(data);
>          return AVERROR(ENOMEM);
> +    }
>
>      err = cbs_insert_unit(ctx, frag, position);
>      if (err < 0) {
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index cdb777d111..9ca1fbd609 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -376,7 +376,8 @@ int ff_cbs_insert_unit_content(CodedBitstreamContext
> *ctx,
>   * Insert a new unit into a fragment with the given data bitstream.
>   *
>   * If data_buf is not supplied then data must have been allocated with
> - * av_malloc() and will become owned by the unit after this call.
> + * av_malloc() and will on success become owned by the unit after this
> + * call or freed on error.
>   */
>  int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx,
>                              CodedBitstreamFragment *frag,
> diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c
> index b189cbd9b7..b52e5c5823 100644
> --- a/libavcodec/cbs_jpeg.c
> +++ b/libavcodec/cbs_jpeg.c
> @@ -225,11 +225,8 @@ static int
> cbs_jpeg_split_fragment(CodedBitstreamContext *ctx,
>
>          err = ff_cbs_insert_unit_data(ctx, frag, unit, marker,
>                                        data, data_size, data_ref);
> -        if (err < 0) {
> -            if (!data_ref)
> -                av_freep(&data);
> +        if (err < 0)
>              return err;
> -        }
>
>          if (next_marker == -1)
>              break;
> --
>

Ping.

- Andreas