PR #25523 opened by Altafk960 URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25523 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25523.patch
Hi, the size checks in the cbs slice/OBU writers multiply the data size by 8 before comparing it against the space left in the write buffer. On a 32 bit build that multiplication wraps at 512 MiB, so the check can pass for data that does not fit and the copy then runs past the end of the buffer. cbs_mpeg2 is reachable: a generated 512 MiB slice unit passed through mpeg2_metadata makes the memcpy run off the 1 MiB write buffer (I hit this on an i486 build under qemu). cbs_h2645 and cbs_av1 have the same broken check, but their units are capped earlier (h2645_parse rejects anything over INT_MAX/8, and cbs_av1_split_fragment() does the same), so today they are defensive only. I fixed all three anyway, because a size check should not be correct only thanks to a cap in another module. This is the same rewrite that already landed for cbs_jpeg and cbs_vp9 earlier this month (compare the byte count against put_bits_left() / 8 instead). The slice writers keep the byte reserved for the rbsp_stop_one_bit by comparing data_size + 1; the two forms are the same for every non-wrapping size. There is also a FATE test that generates the big slice and expects a clean error instead of a crash. Heads up on two things: it writes 512 MiB into $TMPDIR and takes about 16 seconds, and it only actually distinguishes old from new code on targets with a 32 bit size_t. Happy to drop it if that cost is not welcome. What I ran: - i486 static build under qemu: old code dies with SIGSEGV on the generated slice, patched code refuses cleanly with "Out of memory", small inputs round-trip fine on both. - 64 bit: behaves the same before and after (the multiplication cannot wrap there). - the new FATE test plus fate-cbs-mpeg2 pass, in-tree and in an out-of-tree build. Thanks for taking a look. From 0509bea949a099a78f5101509f46bf9fe149d5a4 Mon Sep 17 00:00:00 2001 From: Altaf Khan <[email protected]> Date: Tue, 27 Oct 2026 17:54:18 +0530 Subject: [PATCH] avcodec/cbs: check slice and OBU data sizes without overflowing size_t The checks that compare a data size with the space left in the write buffer multiply the size by 8 first. That multiplication is done in size_t, so with a 32 bit size_t a data size of 2^29 - 1 bytes or more wraps and the check can pass for data that does not fit; the slice data is then copied past the end of the write buffer, which cbs_write_unit_data() caps at INT_MAX / 8 bytes. cbs_mpeg2 is reachable: the demuxer passes an oversized slice unit straight through to the metadata bitstream filter. On a 32 bit build a 512 MiB slice makes the check compare 16 against a non-zero put_bits_left() and the memcpy() runs off the 1 MiB buffer. cbs_h2645 and cbs_av1 cannot be reached today because h2645_parse and cbs_av1_split_fragment() reject units larger than INT_MAX / 8, but their checks are wrong in the same way and should not rely on that. Compare the byte count against put_bits_left() / 8 instead, as was already done for cbs_jpeg and cbs_vp9. The slice writers keep the byte reserved for the rbsp_stop_one_bit by comparing data_size + 1; the two forms are equivalent for every non-wrapping size. Add a regression test that runs mpeg2_metadata over such a slice and requires a clean error rather than a crash. It generates its own input (512 MiB, so it is slow) and only discriminates on targets with a 32 bit size_t. Signed-off-by: Altaf Khan <[email protected]> --- libavcodec/cbs_av1.c | 2 +- libavcodec/cbs_h2645.c | 2 +- libavcodec/cbs_mpeg2.c | 2 +- tests/fate/cbs-size-wrap.sh | 52 +++++++++++++++++++++++++++++++++++++ tests/fate/cbs.mak | 12 +++++++++ 5 files changed, 67 insertions(+), 3 deletions(-) create mode 100755 tests/fate/cbs-size-wrap.sh diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c index 4bacebf0865..e4b275199ba 100644 --- a/libavcodec/cbs_av1.c +++ b/libavcodec/cbs_av1.c @@ -1199,7 +1199,7 @@ static int cbs_av1_write_obu(CodedBitstreamContext *ctx, flush_put_bits(pbc); av_assert0(data_pos <= start_pos); - if (8 * obu->obu_size > put_bits_left(pbc)) { + if (obu->obu_size > put_bits_left(pbc) / 8) { av_refstruct_unref(&priv->sequence_header_ref); av_buffer_unref(&priv->frame_header_ref); *priv = av1ctx; diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index 61b70f41184..9329554cab9 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -272,7 +272,7 @@ int ff_cbs_h2645_write_slice_data(CodedBitstreamContext *ctx, av_assert0(data_bit_start >= 0 && data_size > data_bit_start / 8); - if (data_size * 8 + 8 > put_bits_left(pbc)) + if (data_size + 1 > put_bits_left(pbc) / 8) return AVERROR(ENOSPC); if (!rest) diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index 87401d15436..57c22053938 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -319,7 +319,7 @@ static int cbs_mpeg2_write_slice(CodedBitstreamContext *ctx, av_assert0(slice->data_bit_start >= 0 && slice->data_size > slice->data_bit_start / 8); - if (slice->data_size * 8 + 8 > put_bits_left(pbc)) + if (slice->data_size + 1 > put_bits_left(pbc) / 8) return AVERROR(ENOSPC); // First copy the remaining bits of the first byte diff --git a/tests/fate/cbs-size-wrap.sh b/tests/fate/cbs-size-wrap.sh new file mode 100755 index 00000000000..8d95b58ab7e --- /dev/null +++ b/tests/fate/cbs-size-wrap.sh @@ -0,0 +1,52 @@ +#!/bin/sh +# Regression test for the cbs slice-data size check. +# +# Builds a 512 MiB MPEG-2 frame whose slice unit is large enough that +# "data_size * 8" wraps a 32 bit size_t. On a 32 bit build the old check +# passes, the slice is memcpy()ed past the end of the 1 MiB write buffer and +# ffmpeg dies with SIGSEGV. With the check rewritten as +# "data_size + 1 > put_bits_left(pbc) / 8" the guard fires instead and ffmpeg +# exits normally (with ENOMEM, because 512 MiB does not fit the 256 MiB +# write-buffer ceiling either way). +# +# On a 64 bit build both versions behave the same, so this only discriminates +# on 32 bit targets. +# +# Invoked from the FATE build directory (in-tree or out-of-tree): the ffmpeg +# binary is ./ffmpeg relative to the current directory, and all scratch files +# go to a temporary directory so a read-only source tree works too. + +set -e + +FFMPEG=${FFMPEG:-./ffmpeg} +TMPD=$(mktemp -d) +trap 'rm -rf "$TMPD"' EXIT +POC=$TMPD/cbs-size-wrap.m2v +ERR=$TMPD/cbs-size-wrap.err + +# 47 byte MPEG-2 prefix: sequence header, sequence extension, GOP header, +# picture header, picture coding extension (64x64, frame picture). +printf '\000\000\001\263\004\000\100\022\377\377\340\030' > "$POC" +printf '\000\000\001\265\024\212\000\001\000\027\000\000' >> "$POC" +printf '\001\270\000\010\000\100\000\000\001\000\000\017\377\370' >> "$POC" +printf '\000\000\001\265\217\377\363\101\200' >> "$POC" +# slice start code + slice header whose last bit is extra_bit_slice=1 +printf '\000\000\001\001\014\002\000' >> "$POC" +# 0x20000001 bytes of slice data +head -c 536870913 /dev/zero | LC_ALL=C tr '\000' '\377' >> "$POC" + +rc=0 +"$FFMPEG" -hide_banner -nostdin -i "$POC" -c copy \ + -bsf:v mpeg2_metadata -f mpeg2video /dev/null -y >/dev/null 2>"$ERR" || rc=$? + +# 128 and above means killed by a signal. +if [ "$rc" -ge 128 ]; then + echo "FAIL: ffmpeg died with signal $((rc - 128))" + exit 1 +fi + +# The guard must have fired: 512 MiB does not fit the 256 MiB write buffer. +if ! grep -qE "Cannot allocate memory|Out of memory" "$ERR"; then + echo "FAIL: the size check did not fire" + exit 1 +fi diff --git a/tests/fate/cbs.mak b/tests/fate/cbs.mak index d9200debc95..1bb3307a76c 100644 --- a/tests/fate/cbs.mak +++ b/tests/fate/cbs.mak @@ -196,6 +196,18 @@ FATE_CBS_MPEG2-$(call FATE_CBS_DEPS, MPEGVIDEO, MPEGVIDEO, MPEG2, MPEG2VIDEO, MP FATE_SAMPLES_AVCONV += $(FATE_CBS_MPEG2-yes) fate-cbs-mpeg2: $(FATE_CBS_MPEG2-yes) +# 512 MiB slice: exercises the size check against a 32 bit size_t wrap. +# Generates its own input, so it needs no external sample. Registered +# through FATE_CBS_MPEG2-yes, which lands in FATE_SAMPLES_AVCONV — it runs +# when SAMPLES is set. Only discriminates on 32 bit targets; on 64 bit the +# old and the new check behave the same. Runs the script straight from the +# source tree via runlocal (FATE's CWD is the build directory, where the +# freshly built ./ffmpeg lives); "run" would look for the script in the +# build tree, which only works for in-tree builds. +fate-cbs-mpeg2-size-wrap: CMD = runlocal fate/cbs-size-wrap.sh +fate-cbs-mpeg2-size-wrap: REF = /dev/null +FATE_CBS_MPEG2-$(call ALLYES, MPEGVIDEO_DEMUXER MPEGVIDEO_PARSER MPEG2_METADATA_BSF MPEG2VIDEO_MUXER) += fate-cbs-mpeg2-size-wrap + # VP9 read/write FATE_CBS_VP9_SAMPLES = \ -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
