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

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@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(-)

On Mon, Nov 18, 2019 at 8:48 AM Andreas Rheinhardt <
andreas.rheinhardt@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@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

On 18/11/2019 07:47, Andreas Rheinhardt 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@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;
> 

Yep, applied.

Thanks,

- Mark