[FFmpeg-devel] [PATCH 3/6] avcodec/aac/aacdec_usac: Fix memory deallocation of pl_data

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: double free
> Fixes: 393523547/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AAC_LATM_fuzzer-6740617236905984
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libavcodec/aac/aacdec.c      | 19 +++++++++++++++++--
>  libavcodec/aac/aacdec_usac.c |  3 ++-
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/aac/aacdec.c b/libavcodec/aac/aacdec.c
> index 8d50ad6d095..16259b5ada9 100644
> --- a/libavcodec/aac/aacdec.c
> +++ b/libavcodec/aac/aacdec.c
> @@ -421,6 +421,21 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags)
>      return layout;
>  }
>  
> +static void copy_oc(OutputConfiguration *dst, OutputConfiguration *src)
> +{
> +    int err = 0;

Seems unused.

> +
> +    for(int i = 0; i < dst->usac.nb_elems; i++)
> +        av_freep(&dst->usac.elems[i].ext.pl_data);
> +
> +    *dst = *src;
> +
> +    for(int i = 0; i < dst->usac.nb_elems; i++) {
> +        AACUsacElemConfig *e = &dst->usac.elems[i];
> +        e->ext.pl_data = av_memdup(e->ext.pl_data, e->ext.pl_data_offset);

Unchecked allocation. Furthermore, the *dst = *src makes cleanup on
error here a PITA. Would making pl_data reference-counted (via
RefStruct) work instead?

> +    }
> +}
> +
>  /**
>   * Save current output configuration if and only if it has been locked.
>   */
> @@ -429,7 +444,7 @@ static int push_output_configuration(AACDecContext *ac)
>      int pushed = 0;
>  
>      if (ac->oc[1].status == OC_LOCKED || ac->oc[0].status == OC_NONE) {
> -        ac->oc[0] = ac->oc[1];
> +        copy_oc(&ac->oc[0], &ac->oc[1]);
>          pushed = 1;
>      }
>      ac->oc[1].status = OC_NONE;
> @@ -443,7 +458,7 @@ static int push_output_configuration(AACDecContext *ac)
>  static void pop_output_configuration(AACDecContext *ac)
>  {
>      if (ac->oc[1].status != OC_LOCKED && ac->oc[0].status != OC_NONE) {
> -        ac->oc[1] = ac->oc[0];
> +        copy_oc(&ac->oc[1], &ac->oc[0]);
>          ac->avctx->ch_layout = ac->oc[1].ch_layout;
>          ff_aac_output_configure(ac, ac->oc[1].layout_map, ac->oc[1].layout_map_tags,
>                                  ac->oc[1].status, 0);
> diff --git a/libavcodec/aac/aacdec_usac.c b/libavcodec/aac/aacdec_usac.c
> index ccdf58bc8e2..e6f86b4a677 100644
> --- a/libavcodec/aac/aacdec_usac.c
> +++ b/libavcodec/aac/aacdec_usac.c
> @@ -1604,7 +1604,8 @@ static int parse_ext_ele(AACDecContext *ac, AACUsacElemConfig *e,
>      if (!(pl_frag_start && pl_frag_end)) {
>          tmp = av_realloc(e->ext.pl_data, e->ext.pl_data_offset + len);
>          if (!tmp) {
> -            av_free(e->ext.pl_data);
> +            av_freep(&e->ext.pl_data);
> +            e->ext.pl_data_offset = 0;
>              return AVERROR(ENOMEM);
>          }
>          e->ext.pl_data = tmp;