From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.ffmpeg.org (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTPS id 0F08E44B51 for ; Sun, 17 Aug 2025 02:07:10 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTP id 4BE4268D2FE; Sun, 17 Aug 2025 05:07:05 +0300 (EEST) Received: from c1ad6a1ecdc3 (code.ffmpeg.org [188.245.149.3]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTPS id 080EE68CF0A for ; Sun, 17 Aug 2025 05:07:03 +0300 (EEST) MIME-Version: 1.0 To: ffmpeg-devel@ffmpeg.org Subject: [FFmpeg-devel] =?utf-8?q?=5BPATCH=5D_aacdec=5Fusac=3A_use_RefStr?= =?utf-8?q?uct_to_track_unfinished_extension_buffers_=5BLynnes_version_+_2?= =?utf-8?q?_bugfixes=5D_=28PR_=2320261=29?= X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: michaelni via ffmpeg-devel Reply-To: FFmpeg development discussions and patches Cc: michaelni Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Message-Id: <20250817020705.4BE4268D2FE@ffbox0-bg.ffmpeg.org> Date: Sun, 17 Aug 2025 05:07:05 +0300 (EEST) Archived-At: List-Archive: List-Post: PR #20261 opened by michaelni URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20261 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20261.patch Extensions in AAC USAC can be stored across multiple frames (mainly to keep CBR compliance). This means that we need to reallocate a buffer when new data is received, accumulate the bitstream data, and so on until the end of extension flag is signalled and the extension can be decoded. This is made more complicated by the way in which the AAC channel layout switching is performed. After decades of evolution, our AAC decoder evolved to double-buffer its entire configuration. All changes are buffered, verified, and applied, on a per-frame basis if required, in often random order. Since we allocate the extension data on heap, this means that if configuration is applied, in order to avoid double-freeing, we have to keep track of what we've allocated. It should be noted that extensions which are spread in multiple frames are generally rare, so an optimization to introduce av_refstruct_realloc() wouldn't generally be useful across the codebase. Therefore, a copy is good enough for now. 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 >From 364fd9e815a33ed2d55271cda252106889bdae3f Mon Sep 17 00:00:00 2001 From: Lynne Date: Mon, 11 Aug 2025 22:26:35 +0900 Subject: [PATCH] aacdec_usac: use RefStruct to track unfinished extension buffers Extensions in AAC USAC can be stored across multiple frames (mainly to keep CBR compliance). This means that we need to reallocate a buffer when new data is received, accumulate the bitstream data, and so on until the end of extension flag is signalled and the extension can be decoded. This is made more complicated by the way in which the AAC channel layout switching is performed. After decades of evolution, our AAC decoder evolved to double-buffer its entire configuration. All changes are buffered, verified, and applied, on a per-frame basis if required, in often random order. Since we allocate the extension data on heap, this means that if configuration is applied, in order to avoid double-freeing, we have to keep track of what we've allocated. It should be noted that extensions which are spread in multiple frames are generally rare, so an optimization to introduce av_refstruct_realloc() wouldn't generally be useful across the codebase. Therefore, a copy is good enough for now. 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 --- libavcodec/aac/aacdec.c | 28 +++++++++++++++++++++++++--- libavcodec/aac/aacdec.h | 4 ++-- libavcodec/aac/aacdec_usac.c | 31 +++++++++++++++++++------------ 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/libavcodec/aac/aacdec.c b/libavcodec/aac/aacdec.c index 6a2aa9dc8e..90a8468898 100644 --- a/libavcodec/aac/aacdec.c +++ b/libavcodec/aac/aacdec.c @@ -62,6 +62,7 @@ #include "libavutil/opt.h" #include "libavutil/tx.h" #include "libavutil/version.h" +#include "libavutil/refstruct.h" /* * supported tools @@ -421,6 +422,26 @@ static uint64_t sniff_channel_order(uint8_t (*layout_map)[3], int tags) return layout; } +static void copy_oc(OutputConfiguration *dst, OutputConfiguration *src) +{ + int i; + + for (i = 0; i < src->usac.nb_elems; i++) { + AACUsacElemConfig *src_e = &src->usac.elems[i]; + AACUsacElemConfig *dst_e = &dst->usac.elems[i]; + + av_refstruct_replace(&dst_e->ext.pl_buf, src_e->ext.pl_buf); + av_assert0(dst_e->ext.pl_buf == src_e->ext.pl_buf); + } + for (; i < dst->usac.nb_elems; i++) { + AACUsacElemConfig *dst_e = &dst->usac.elems[i]; + + av_refstruct_replace(&dst_e->ext.pl_buf, NULL); + } + + *dst = *src; +} + /** * Save current output configuration if and only if it has been locked. */ @@ -429,7 +450,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 +464,8 @@ 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); @@ -1107,7 +1129,7 @@ static av_cold int decode_close(AVCodecContext *avctx) AACUSACConfig *usac = &oc->usac; for (int j = 0; j < usac->nb_elems; j++) { AACUsacElemConfig *ec = &usac->elems[j]; - av_freep(&ec->ext.pl_data); + av_refstruct_unref(&ec->ext.pl_buf); } av_channel_layout_uninit(&ac->oc[i].ch_layout); diff --git a/libavcodec/aac/aacdec.h b/libavcodec/aac/aacdec.h index e5a79a7139..b3763fdccc 100644 --- a/libavcodec/aac/aacdec.h +++ b/libavcodec/aac/aacdec.h @@ -344,7 +344,7 @@ typedef struct AACUsacElemConfig { uint8_t payload_frag; uint32_t default_len; uint32_t pl_data_offset; - uint8_t *pl_data; + uint8_t *pl_buf; } ext; } AACUsacElemConfig; @@ -353,7 +353,7 @@ typedef struct AACUSACConfig { uint16_t core_frame_len; uint16_t stream_identifier; - AACUsacElemConfig elems[64]; + AACUsacElemConfig elems[MAX_ELEM_ID]; int nb_elems; struct { diff --git a/libavcodec/aac/aacdec_usac.c b/libavcodec/aac/aacdec_usac.c index e03e6e015f..3223ffdb9d 100644 --- a/libavcodec/aac/aacdec_usac.c +++ b/libavcodec/aac/aacdec_usac.c @@ -24,12 +24,13 @@ #include "aacdec_ac.h" #include "libavcodec/aacsbr.h" - #include "libavcodec/aactab.h" -#include "libavutil/mem.h" #include "libavcodec/mpeg4audio.h" #include "libavcodec/unary.h" +#include "libavutil/mem.h" +#include "libavutil/refstruct.h" + /* Number of scalefactor bands per complex prediction band, equal to 2. */ #define SFB_PER_PRED_BAND 2 @@ -1574,7 +1575,6 @@ static int parse_audio_preroll(AACDecContext *ac, GetBitContext *gb) static int parse_ext_ele(AACDecContext *ac, AACUsacElemConfig *e, GetBitContext *gb) { - uint8_t *tmp; uint8_t pl_frag_start = 1; uint8_t pl_frag_end = 1; uint32_t len; @@ -1601,18 +1601,25 @@ static int parse_ext_ele(AACDecContext *ac, AACUsacElemConfig *e, if (pl_frag_start) e->ext.pl_data_offset = 0; - /* If an extension starts and ends this packet, we can directly use it */ + /* If an extension starts and ends this packet, we can directly use it below. + * Otherwise, we have to copy it to a buffer and accumulate it. */ 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); + /* Reallocate the data */ + uint8_t *tmp_buf = av_refstruct_alloc_ext(e->ext.pl_data_offset + len, + AV_REFSTRUCT_FLAG_NO_ZEROING, + NULL, NULL); + if (!tmp_buf) return AVERROR(ENOMEM); - } - e->ext.pl_data = tmp; + + if (e->ext.pl_buf) + memcpy(tmp_buf, e->ext.pl_buf, e->ext.pl_data_offset); + + av_refstruct_unref(&e->ext.pl_buf); + e->ext.pl_buf = tmp_buf; /* Readout data to a buffer */ for (int i = 0; i < len; i++) - e->ext.pl_data[e->ext.pl_data_offset + i] = get_bits(gb, 8); + e->ext.pl_buf[e->ext.pl_data_offset + i] = get_bits(gb, 8); } e->ext.pl_data_offset += len; @@ -1624,7 +1631,7 @@ static int parse_ext_ele(AACDecContext *ac, AACUsacElemConfig *e, GetBitContext *gb2 = gb; GetBitContext gbc; if (!(pl_frag_start && pl_frag_end)) { - ret = init_get_bits8(&gbc, e->ext.pl_data, pl_len); + ret = init_get_bits8(&gbc, e->ext.pl_buf, pl_len); if (ret < 0) return ret; @@ -1642,7 +1649,7 @@ static int parse_ext_ele(AACDecContext *ac, AACUsacElemConfig *e, /* This should never happen */ av_assert0(0); } - av_freep(&e->ext.pl_data); + av_refstruct_unref(&e->ext.pl_buf); if (ret < 0) return ret; -- 2.49.1 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".