PR #24992 opened by 5ym URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24992 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24992.patch
Follow-ups to #24067 in `libaribcaption.c`, as noted in the Fairy review there. All three are pre-existing bugs: 1. **Bitmap mode leaks every rendered caption.** `aribcc_renderer_render()` [zeroes `out_result` before filling it](https://github.com/xqq/libaribcaption/blob/0a3a209d3f0b3650b92941248b9701bf69584a1f/src/renderer/renderer_capi.cpp#L212-L249), and the bitmap path only called `aribcc_render_result_cleanup()` on the unchanged, no-image and unknown-status returns. The result is now released once its images have been converted, on both the success and error paths. Nothing is kept between calls, so `close` needs no change. 2. **Text mode writes past a zero-sized allocation.** `rects` was allocated with `region_count` entries, but exactly one rect is always written. For a caption without regions (CS, or TIME only) that is `av_calloc(0, ...)`. 3. **Text and ass mode clear on TIME-only statements.** Both treated every regionless caption as a clear. They now check `ARIBCC_CAPTIONFLAGS_CLEARSCREEN`, as the bitmap path does since #24067. ## Tests New `fate-sub-aribcaption-clear-text` and `-ass` run the existing `sub/aribcaption_clear.ts` sample (caption at 2s, TIME only at 3s, CS only at 4s). Neither mode renders, so the tests do not depend on fonts. Minimal `--toolchain=gcc-asan` build, libaribcaption 1.1.1, `ffprobe -show_frames` on that sample (`pts_time:num_rects`): | mode | master | this PR | |---|---|---| | bitmap | `2:1 4:0`, LeakSanitizer: 38440 bytes in 2 allocations | `2:1 4:0`, clean | | text | `2:1`, then **heap-buffer-overflow** at the 3s statement | `2:1 4:1`, clean | | ass | `2:1 3:1 4:1` (erroneous clear at 3s) | `2:1 4:1`, clean | `fate-sub-aribcaption-clear`, `-text` and `-ass` pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01DdDiDRusmi6eD7qSrKBjHb From 889807c6709f5ea0d866731f53028a7fabef6a7f Mon Sep 17 00:00:00 2001 From: Ruk Doe <[email protected]> Date: Sat, 10 Oct 2026 12:04:27 +0900 Subject: [PATCH 1/3] avcodec/libaribcaption: free the render result after converting it aribcc_renderer_render() fills ctx->render_result with images that are owned by the caller and must be released with aribcc_render_result_cleanup(). The bitmap path did this only for the "unchanged", "no image" and unknown-status returns. After converting the images into AVSubtitleRects, and on the error path, the result was left allocated, and the next call overwrote it without freeing it. Every rendered caption therefore leaked its bitmaps. The images are not used after conversion, so release them before returning from both paths. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01DdDiDRusmi6eD7qSrKBjHb --- libavcodec/libaribcaption.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavcodec/libaribcaption.c b/libavcodec/libaribcaption.c index e8f7cada73..b68f1a6453 100644 --- a/libavcodec/libaribcaption.c +++ b/libavcodec/libaribcaption.c @@ -493,6 +493,7 @@ static int aribcaption_trans_bitmap_subtitle(ARIBCaptionContext *ctx) av_log(ctx, AV_LOG_WARNING, "CLUT overflow (%d).\n", ctx->clut_overflow); } sub->num_rects = rect_idx; + aribcc_render_result_cleanup(&ctx->render_result); return rect_idx; @@ -508,6 +509,7 @@ fail: av_freep(&sub->rects); } sub->num_rects = 0; + aribcc_render_result_cleanup(&ctx->render_result); return ret; } -- 2.52.0 From 020ca7eaf44e6ebcc018d92871f4f6ba58d4f787 Mon Sep 17 00:00:00 2001 From: Ruk Doe <[email protected]> Date: Sat, 10 Oct 2026 12:04:49 +0900 Subject: [PATCH 2/3] avcodec/libaribcaption: allocate one rect in text mode aribcaption_trans_text_subtitle() always fills exactly one rect, but allocated the rects array with ctx->caption.region_count entries. For a caption without regions, such as a clear screen, that is a zero-sized allocation, and writing sub->rects[0] goes past its end. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01DdDiDRusmi6eD7qSrKBjHb --- libavcodec/libaribcaption.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/libaribcaption.c b/libavcodec/libaribcaption.c index b68f1a6453..391fb25da2 100644 --- a/libavcodec/libaribcaption.c +++ b/libavcodec/libaribcaption.c @@ -814,7 +814,7 @@ static int aribcaption_trans_text_subtitle(ARIBCaptionContext *ctx) int ret = 0; const char *text; - sub->rects = av_calloc(ctx->caption.region_count, sizeof(*sub->rects)); + sub->rects = av_calloc(1, sizeof(*sub->rects)); if (!sub->rects) { ret = AVERROR(ENOMEM); goto fail; -- 2.52.0 From 883aa1f7211056e316e703769963623248aec434 Mon Sep 17 00:00:00 2001 From: Ruk Doe <[email protected]> Date: Sat, 10 Oct 2026 12:09:03 +0900 Subject: [PATCH 3/3] avcodec/libaribcaption: only clear on CS in text and ass mode The text and ass paths treated every caption without regions as a clear and emitted an empty ("") subtitle. A statement containing only TIME also decodes to zero regions, without the clear-screen flag, so it erased the caption that was being displayed. Test ARIBCC_CAPTIONFLAGS_CLEARSCREEN as the bitmap path already does, and emit nothing for other regionless captions. Add text and ass mode tests on the existing aribcaption_clear.ts sample (visible caption, TIME-only statement, CS-only statement). Each reference has two events; before this change there was a third, empty one for the TIME-only statement. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01DdDiDRusmi6eD7qSrKBjHb --- libavcodec/libaribcaption.c | 14 +++++++++++++- tests/fate/subtitles.mak | 6 ++++++ tests/ref/fate/sub-aribcaption-clear-ass | 18 ++++++++++++++++++ tests/ref/fate/sub-aribcaption-clear-text | 18 ++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 tests/ref/fate/sub-aribcaption-clear-ass create mode 100644 tests/ref/fate/sub-aribcaption-clear-text diff --git a/libavcodec/libaribcaption.c b/libavcodec/libaribcaption.c index 391fb25da2..9c59e4235d 100644 --- a/libavcodec/libaribcaption.c +++ b/libavcodec/libaribcaption.c @@ -618,13 +618,19 @@ static int aribcaption_trans_ass_subtitle(ARIBCaptionContext *ctx) if (ctx->avctx->profile == AV_PROFILE_ARIB_PROFILE_C) single_rect = true; - sub->format = 1; /* text */ if (ctx->caption.region_count == 0) { + /* Only an explicit clear screen (CS) erases the previous caption; + a statement containing only TIME also has no region. */ + if (!(ctx->caption.flags & ARIBCC_CAPTIONFLAGS_CLEARSCREEN)) + return 0; /* clear previous caption for indefinite duration */ + sub->format = 1; /* text */ ff_ass_add_rect(sub, "", ctx->readorder++, 0, NULL, NULL); return 1; } + sub->format = 1; /* text */ + av_bprint_init(&buf, ARIBC_BPRINT_SIZE_INIT, ARIBC_BPRINT_SIZE_MAX); if (single_rect && ctx->avctx->profile != AV_PROFILE_ARIB_PROFILE_C) { @@ -814,6 +820,12 @@ static int aribcaption_trans_text_subtitle(ARIBCaptionContext *ctx) int ret = 0; const char *text; + /* Only an explicit clear screen (CS) erases the previous caption; + a statement containing only TIME also has no region. */ + if (ctx->caption.region_count == 0 && + !(ctx->caption.flags & ARIBCC_CAPTIONFLAGS_CLEARSCREEN)) + return 0; + sub->rects = av_calloc(1, sizeof(*sub->rects)); if (!sub->rects) { ret = AVERROR(ENOMEM); diff --git a/tests/fate/subtitles.mak b/tests/fate/subtitles.mak index 3ded96e044..66025a03c6 100644 --- a/tests/fate/subtitles.mak +++ b/tests/fate/subtitles.mak @@ -169,6 +169,12 @@ FATE_SUBTITLES_PROBE-$(call DEMDEC, MPEGTS, LIBARIBCAPTION) += fate-sub-aribcapt fate-sub-aribcaption-clear: CMD = probeframes -sub_type bitmap $(TARGET_SAMPLES)/sub/aribcaption_clear.ts fate-sub-aribcaption-clear: CMP = diff +# same sample in text and ass mode: only CS must emit an empty ("") event +FATE_SUBTITLES_PROBE-$(call DEMDEC, MPEGTS, LIBARIBCAPTION) += fate-sub-aribcaption-clear-text fate-sub-aribcaption-clear-ass +fate-sub-aribcaption-clear-text: CMD = probeframes -sub_type text $(TARGET_SAMPLES)/sub/aribcaption_clear.ts +fate-sub-aribcaption-clear-ass: CMD = probeframes -sub_type ass $(TARGET_SAMPLES)/sub/aribcaption_clear.ts +fate-sub-aribcaption-clear-text fate-sub-aribcaption-clear-ass: CMP = diff + FATE_SUBTITLES-$(call ENCMUX, ASS, ASS) += $(FATE_SUBTITLES_ASS-yes) FATE_SUBTITLES += $(FATE_SUBTITLES-yes) FATE_SUBTITLES := $(if $(CONFIG_PIPE_PROTOCOL), $(FATE_SUBTITLES)) diff --git a/tests/ref/fate/sub-aribcaption-clear-ass b/tests/ref/fate/sub-aribcaption-clear-ass new file mode 100644 index 0000000000..caa0d942b9 --- /dev/null +++ b/tests/ref/fate/sub-aribcaption-clear-ass @@ -0,0 +1,18 @@ +[SUBTITLE] +media_type=subtitle +pts=2000000 +pts_time=2.000000 +format=1 +start_display_time=0 +end_display_time=4294967295 +num_rects=1 +[/SUBTITLE] +[SUBTITLE] +media_type=subtitle +pts=4000000 +pts_time=4.000000 +format=1 +start_display_time=0 +end_display_time=4294967295 +num_rects=1 +[/SUBTITLE] diff --git a/tests/ref/fate/sub-aribcaption-clear-text b/tests/ref/fate/sub-aribcaption-clear-text new file mode 100644 index 0000000000..caa0d942b9 --- /dev/null +++ b/tests/ref/fate/sub-aribcaption-clear-text @@ -0,0 +1,18 @@ +[SUBTITLE] +media_type=subtitle +pts=2000000 +pts_time=2.000000 +format=1 +start_display_time=0 +end_display_time=4294967295 +num_rects=1 +[/SUBTITLE] +[SUBTITLE] +media_type=subtitle +pts=4000000 +pts_time=4.000000 +format=1 +start_display_time=0 +end_display_time=4294967295 +num_rects=1 +[/SUBTITLE] -- 2.52.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
