On Mon, Nov 18, 2019 at 8:48 AM Andreas Rheinhardt <
andreas.rheinha...@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.rheinha...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
