PR #25650 opened by luoye-cpu URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25650 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25650.patch
\ >From c1917a0936d7b534019393a5f9591a44ba9d0180 Mon Sep 17 00:00:00 2001 From: luoye-cpu <[email protected]> Date: Sun, 1 Nov 2026 16:15:56 +0800 Subject: [PATCH 1/2] avutil/hdr_dynamic_metadata: tolerate zero padding past the payload Some phone-recorded HDR10+ HEVC streams fail to decode at all. The length check in av_dynamic_hdr_plus_from_t35() returns AVERROR(EINVAL) for a payload longer than AV_HDR_PLUS_MAX_PAYLOAD_SIZE. The HEVC SEI error handling only treats AVERROR_INVALIDDATA as a recoverable error (decode_nal_unit() skips such a NAL unless AV_EF_EXPLODE is set and returns anything else), so one over-long optional metadata message ends the packet and every picture after it. The container, parameter sets and slice data parse successfully; the failure is triggered by the oversized HDR10+ SEI payload, and the same file decodes once that message is short enough. AV_HDR_PLUS_MAX_PAYLOAD_SIZE is 907 because that is the byte-aligned upper bound derived from the largest field combination represented by the current parser and serializer. It is not a fixed payload length specified by CTA-861. A Xiaomi REDMI K90 Pro Max recording (HEVC Main10 1920x1080 59.94p, 8169 pictures) starts every picture with a PREFIX_SEI holding one user_data_registered_itu_t_t35 message of payload_size 1024, T.35 header country_code 0xB5, provider_code 0x003C, provider_oriented_code 1, application_identifier 4, i.e. the HDR10+ case in libavcodec/itut35.c. ff_itut_t35_parse_buffer() consumes 6 bytes and hands 1018 bytes to this function. The fields consume 339 bits on every picture, between 37 and 40 of the 1018 bytes are non-zero (39 on the first), and the rest are zeros. On master at 92457ada93cf that file decodes to zero frames and ffmpeg exits with code 69: [hevc @ ...] Error parsing NAL unit #0. ... Error submitting packet to decoder: Invalid argument ... Decode error rate 1 exceeds maximum 0.666667 This patch accepts a longer input only when it is padding: if the size exceeds the limit, every byte in [AV_HDR_PLUS_MAX_PAYLOAD_SIZE, size) is checked. All zero lets the parse continue over the first AV_HDR_PLUS_MAX_PAYLOAD_SIZE bytes; one non-zero byte returns AVERROR_INVALIDDATA, so a truncated or corrupted payload is still reported as a data error rather than quietly shortened. The clamp happens before memcpy(), so the fixed stack buffer is bounded exactly as before, and the retained bytes still go through every field validation -- padding does not excuse a malformed payload. The entry check rejects both a NULL destination and a NULL data pointer. The latter is necessary because scanning an over-limit region would otherwise dereference it. Passing NULL with a length of at most the limit already crashed in memcpy() before this change; the check fixes that too. CTA-861 Annex S defines no padding element for this syntax. This patch therefore does not claim that the extra bytes are syntax-defined padding; it only tolerates them when they are all zero. With this change all 8169 pictures decode and carry the metadata. The fields vary from picture to picture; on all 8169 frames the decoded HDR10+ fields match those extracted by unpatched master from the corresponding trimmed stream. The first picture parses as application version 1, num_windows 1, targeted_system_display_maximum_luminance 400/1, maxscl 7230/100000 (x3), average_maxrgb 1649/100000 and 9 distribution percentiles. Payloads that already fitted are unaffected: 51- and 251-byte API payloads (the latter with a non-zero tail) behave the same on both builds, and a 913-byte SEI message payload -- 907 after the 6-byte T.35 header -- keeps its frames, frame hashes and fields. The zero-tail handling was also tested with AV1 HDR10+ bitstreams, using both av1dec (D3D11VA) and libdav1d: zero-padded payloads decode, while non-zero tails remain invalid. The next patch adds an API test covering the boundary: at the limit, one byte past it zero and non-zero, a long zero tail, a non-zero byte at several positions inside the tail, a zero tail over a malformed payload, and NULL destination and data arguments. Signed-off-by: luoye-cpu <[email protected]> --- libavutil/hdr_dynamic_metadata.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/libavutil/hdr_dynamic_metadata.c b/libavutil/hdr_dynamic_metadata.c index 9b464993433..88a5fdcb04f 100644 --- a/libavutil/hdr_dynamic_metadata.c +++ b/libavutil/hdr_dynamic_metadata.c @@ -61,13 +61,23 @@ int av_dynamic_hdr_plus_from_t35(AVDynamicHDRPlus *s, const uint8_t *data, { uint8_t padded_buf[AV_HDR_PLUS_MAX_PAYLOAD_SIZE + AV_INPUT_BUFFER_PADDING_SIZE]; GetBitContext gbc, *gb = &gbc; + size_t i; int ret; - if (!s) + if (!s || !data) return AVERROR(EINVAL); - if (size > AV_HDR_PLUS_MAX_PAYLOAD_SIZE) - return AVERROR(EINVAL); + if (size > AV_HDR_PLUS_MAX_PAYLOAD_SIZE) { + // The parser supports payloads up to AV_HDR_PLUS_MAX_PAYLOAD_SIZE + // bytes. Accept a longer input only when all bytes beyond this limit + // are zero; otherwise, report invalid data rather than silently + // truncating it. + for (i = AV_HDR_PLUS_MAX_PAYLOAD_SIZE; i < size; i++) { + if (data[i]) + return AVERROR_INVALIDDATA; + } + size = AV_HDR_PLUS_MAX_PAYLOAD_SIZE; + } memcpy(padded_buf, data, size); // Zero-initialize the buffer padding to avoid overreads into uninitialized data. -- 2.54.0 >From 99d8bcccbb5cdd168b0fa629667696c06393008b Mon Sep 17 00:00:00 2001 From: luoye-cpu <[email protected]> Date: Sun, 1 Nov 2026 16:15:57 +0800 Subject: [PATCH 2/2] tests/api: cover the size handling of av_dynamic_hdr_plus_from_t35() av_dynamic_hdr_plus_from_t35() only ever sees a payload of at most AV_HDR_PLUS_MAX_PAYLOAD_SIZE, and the preceding patch made an all-zero tail past that limit acceptable while keeping a non-zero one a data error. Pin that down without depending on a decoder or a sample file. The payload starts from av_dynamic_hdr_plus_to_t35() output, so the bytes the grammar actually reads are a real payload and the padding is zeros. All sizes below are the input length of av_dynamic_hdr_plus_from_t35(), i.e. the SEI message payload minus the 6-byte T.35 header. Cases: the exact length; an input of exactly AV_HDR_PLUS_MAX_PAYLOAD_SIZE; one byte past it both zero and non-zero; a zero tail of 977 bytes (1884 in total); a non-zero byte planted at several positions of that tail; a zero tail over a payload whose num_windows is 0, which the field checks reject; and NULL arguments, which stay caller errors -- a NULL destination at both a normal and an over-limit length, and a NULL data pointer at both an over-limit length and exactly the limit, where the scan and the memcpy() would otherwise dereference it. The accepted cases must reproduce the fields of the exact-length decode for the single-window payload this test builds, including the optional per-window blocks it enables (tone mapping with Bezier anchors, colour saturation mapping), so the padding cannot silently shift or drop a field. Disabling that comparison makes three of the checks fail, i.e. it is load-bearing. Multi-window payloads are not covered. On unpatched master, the over-limit checks fail, and a NULL data pointer at exactly AV_HDR_PLUS_MAX_PAYLOAD_SIZE reaches memcpy() and crashes the test. With the preceding patch, all eighteen checks pass. Signed-off-by: luoye-cpu <[email protected]> --- tests/api/Makefile | 2 +- tests/api/api-hdr10plus-test.c | 305 +++++++++++++++++++++++++++++++++ tests/fate/api.mak | 5 + 3 files changed, 311 insertions(+), 1 deletion(-) create mode 100644 tests/api/api-hdr10plus-test.c diff --git a/tests/api/Makefile b/tests/api/Makefile index c411e82e5f1..57f68340c01 100644 --- a/tests/api/Makefile +++ b/tests/api/Makefile @@ -6,7 +6,7 @@ APITESTPROGS-$(call DEMDEC, H264, H264) += api-h264-slice APITESTPROGS-$(CONFIG_MOV_MUXER) += api-movenc APITESTPROGS-$(CONFIG_MOVTEXT_ENCODER) += api-movtext APITESTPROGS-$(call ALLYES, HEVC_MP4TOANNEXB_BSF HEVC_DECODER) += api-hevc-mp4toannexb -APITESTPROGS-yes += api-seek api-dump-stream-meta +APITESTPROGS-yes += api-seek api-dump-stream-meta api-hdr10plus APITESTPROGS-$(call DEMDEC, H263, H263) += api-band APITESTPROGS-$(HAVE_THREADS) += api-threadmessage APITESTPROGS-$(CONFIG_AVCODEC) += api-enc-info diff --git a/tests/api/api-hdr10plus-test.c b/tests/api/api-hdr10plus-test.c new file mode 100644 index 00000000000..f973dfd2500 --- /dev/null +++ b/tests/api/api-hdr10plus-test.c @@ -0,0 +1,305 @@ +/* + * Copyright (c) 2026 luoye-cpu + * + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* + * Boundary checks for av_dynamic_hdr_plus_from_t35(): a payload longer than + * AV_HDR_PLUS_MAX_PAYLOAD_SIZE is accepted only when every byte past the + * limit is zero, and the first AV_HDR_PLUS_MAX_PAYLOAD_SIZE bytes must still + * parse -- padding must not let a malformed payload through. + */ + +#include <stdio.h> +#include <string.h> + +#include "libavutil/error.h" +#include "libavutil/hdr_dynamic_metadata.h" +#include "libavutil/macros.h" +#include "libavutil/mem.h" + +#define CAP AV_HDR_PLUS_MAX_PAYLOAD_SIZE + +static int failures; + +static void expect(const char *what, int got, int want) +{ + if (got != want) { + printf("FAIL %s: got %d (%s), want %d\n", what, got, + got < 0 ? av_err2str(got) : "success", want); + failures++; + return; + } + printf("ok %s\n", what); +} + +static int rat_ne(AVRational a, AVRational b) +{ + return a.num != b.num || a.den != b.den; +} + +/** + * Compare the fields the test actually populates, including the optional + * per-window blocks. Not a full-struct comparison. + */ +static int fields_equal(const AVDynamicHDRPlus *x, const AVDynamicHDRPlus *y) +{ + if (x->application_version != y->application_version) + return 0; + if (x->num_windows != y->num_windows) + return 0; + if (rat_ne(x->targeted_system_display_maximum_luminance, + y->targeted_system_display_maximum_luminance)) + return 0; + for (int i = 0; i < 3; i++) + if (rat_ne(x->params[0].maxscl[i], y->params[0].maxscl[i])) + return 0; + if (rat_ne(x->params[0].average_maxrgb, y->params[0].average_maxrgb)) + return 0; + + /* Bounds first: a test helper must not index past the array even if the + payload it is handed later grows more percentiles than fit. */ + if (x->params[0].num_distribution_maxrgb_percentiles != + y->params[0].num_distribution_maxrgb_percentiles) + return 0; + if (x->params[0].num_distribution_maxrgb_percentiles > + FF_ARRAY_ELEMS(x->params[0].distribution_maxrgb)) + return 0; + for (int i = 0; i < x->params[0].num_distribution_maxrgb_percentiles; i++) { + if (x->params[0].distribution_maxrgb[i].percentage != + y->params[0].distribution_maxrgb[i].percentage) + return 0; + if (rat_ne(x->params[0].distribution_maxrgb[i].percentile, + y->params[0].distribution_maxrgb[i].percentile)) + return 0; + } + if (rat_ne(x->params[0].fraction_bright_pixels, + y->params[0].fraction_bright_pixels)) + return 0; + + /* optional per-window blocks: the clamps must not disturb these either */ + if (x->params[0].tone_mapping_flag != y->params[0].tone_mapping_flag) + return 0; + if (x->params[0].tone_mapping_flag) { + if (rat_ne(x->params[0].knee_point_x, y->params[0].knee_point_x)) + return 0; + if (rat_ne(x->params[0].knee_point_y, y->params[0].knee_point_y)) + return 0; + if (x->params[0].num_bezier_curve_anchors != + y->params[0].num_bezier_curve_anchors) + return 0; + if (x->params[0].num_bezier_curve_anchors > + FF_ARRAY_ELEMS(x->params[0].bezier_curve_anchors)) + return 0; + for (int i = 0; i < x->params[0].num_bezier_curve_anchors; i++) + if (rat_ne(x->params[0].bezier_curve_anchors[i], + y->params[0].bezier_curve_anchors[i])) + return 0; + } + if (x->params[0].color_saturation_mapping_flag != + y->params[0].color_saturation_mapping_flag) + return 0; + if (x->params[0].color_saturation_mapping_flag && + rat_ne(x->params[0].color_saturation_weight, + y->params[0].color_saturation_weight)) + return 0; + return 1; +} + +static void check_same(const char *what, const AVDynamicHDRPlus *x, + const AVDynamicHDRPlus *y) +{ + if (fields_equal(x, y)) { + printf("ok %s gives the same fields\n", what); + } else { + printf("FAIL %s changed the decoded fields\n", what); + failures++; + } +} + +/** + * `len` valid bytes followed by zero padding up to `size`. + */ +static uint8_t *pad(const uint8_t *src, size_t len, size_t size) +{ + uint8_t *buf = av_mallocz(size); + if (!buf) + return NULL; + memcpy(buf, src, len < size ? len : size); + return buf; +} +int main(void) +{ + size_t alloc_size = 0, len = 0, long_size; + uint8_t *t35 = NULL, *atcap = NULL, *one_extra = NULL; + uint8_t *long_tail = NULL, *broken = NULL; + AVDynamicHDRPlus *src = NULL, *a = NULL, *b = NULL; + int ret, err = 1; + + src = av_dynamic_hdr_plus_alloc(&alloc_size); + a = av_dynamic_hdr_plus_alloc(&alloc_size); + b = av_dynamic_hdr_plus_alloc(&alloc_size); + if (!src || !a || !b) { + printf("alloc failed\n"); + goto end; + } + + src->application_version = 1; + src->num_windows = 1; + src->targeted_system_display_maximum_luminance = (AVRational){ 400, 10000 }; + for (int i = 0; i < 3; i++) + src->params[0].maxscl[i] = (AVRational){ 7230, 131071 }; + src->params[0].average_maxrgb = (AVRational){ 1649, 131071 }; + src->params[0].num_distribution_maxrgb_percentiles = 2; + src->params[0].distribution_maxrgb[0].percentage = 1; + src->params[0].distribution_maxrgb[0].percentile = + (AVRational){ 40, 131071 }; + src->params[0].distribution_maxrgb[1].percentage = 5; + src->params[0].distribution_maxrgb[1].percentile = + (AVRational){ 6788, 131071 }; + src->params[0].fraction_bright_pixels = (AVRational){ 0, 1023 }; + + /* Enable the optional per-window blocks, so the clamps below are also + checked against a payload where tone mapping and colour saturation + mapping are present -- not just the minimal single-window shape. */ + src->params[0].tone_mapping_flag = 1; + src->params[0].knee_point_x = (AVRational){ 1000, 4095 }; + src->params[0].knee_point_y = (AVRational){ 500, 4095 }; + src->params[0].num_bezier_curve_anchors = 3; + src->params[0].bezier_curve_anchors[0] = (AVRational){ 100, 1023 }; + src->params[0].bezier_curve_anchors[1] = (AVRational){ 500, 1023 }; + src->params[0].bezier_curve_anchors[2] = (AVRational){ 900, 1023 }; + src->params[0].color_saturation_mapping_flag = 1; + src->params[0].color_saturation_weight = (AVRational){ 4, 8 }; + + ret = av_dynamic_hdr_plus_to_t35(src, &t35, &len); + if (ret < 0) { + printf("to_t35 failed: %s\n", av_err2str(ret)); + goto end; + } + printf("serialised payload: %zu bytes, cap: %d\n", len, CAP); + + /* 1) the exact length parses */ + expect("exact length", av_dynamic_hdr_plus_from_t35(a, t35, len), 0); + + /* 2) exactly at the cap: the tail is inside the limit, i.e. the + * pre-existing path, and must still give the same fields */ + atcap = pad(t35, len, CAP); + if (!atcap) + goto end; + expect("cap bytes", av_dynamic_hdr_plus_from_t35(b, atcap, CAP), 0); + check_same("cap bytes", a, b); + + /* 3) one byte past the cap, zero: accepted with the same fields */ + one_extra = pad(t35, len, CAP + 1); + if (!one_extra) + goto end; + ret = av_dynamic_hdr_plus_from_t35(b, one_extra, CAP + 1); + expect("cap+1, zero tail", ret, 0); + if (ret >= 0) + check_same("cap+1, zero tail", a, b); + + /* 4) one byte past the cap, non-zero: a data error */ + one_extra[CAP] = 0x01; + expect("cap+1, non-zero tail", + av_dynamic_hdr_plus_from_t35(b, one_extra, CAP + 1), + AVERROR_INVALIDDATA); + + /* 5) a long zero tail, the shape a device writes: accepted */ + long_size = CAP + 977; + long_tail = pad(t35, len, long_size); + if (!long_tail) + goto end; + ret = av_dynamic_hdr_plus_from_t35(b, long_tail, long_size); + expect("cap+977, zero tail", ret, 0); + if (ret >= 0) + check_same("cap+977, zero tail", a, b); + + /* 6) the whole over-limit region is scanned, not just its start */ + static const size_t probes[] = { CAP, CAP + 1, 1200, CAP + 976 }; + for (int i = 0; i < 4; i++) { + char what[64]; + long_tail[probes[i]] = 0x80; + snprintf(what, sizeof(what), "cap+977, non-zero byte at %zu", + probes[i]); + expect(what, av_dynamic_hdr_plus_from_t35(b, long_tail, long_size), + AVERROR_INVALIDDATA); + long_tail[probes[i]] = 0; + } + + /* 7) a zero tail must not excuse a malformed payload: num_windows = 0 is + * representable in the bitstream but rejected by the field checks. + * Run the same malformed bytes at two lengths so the two questions + * stay separated: does the parser still reject bad fields (in-limit + * and over-limit), and does the over-limit tolerance wrongly relax + * field validation? */ + broken = pad(t35, len, long_size); + if (!broken) + goto end; + broken[1] &= 0x3F; /* clear the two num_windows bits */ + + /* 7a) malformed fields, exactly at the cap (no over-limit path taken) */ + expect("cap bytes, num_windows = 0", + av_dynamic_hdr_plus_from_t35(b, broken, CAP), + AVERROR_INVALIDDATA); + + /* 7b) malformed fields with a zero tail past the cap */ + expect("cap+977, zero tail, num_windows = 0", + av_dynamic_hdr_plus_from_t35(b, broken, long_size), + AVERROR_INVALIDDATA); + + /* 8) a NULL destination stays a caller error, not a data error */ + expect("NULL destination", + av_dynamic_hdr_plus_from_t35(NULL, t35, len), AVERROR(EINVAL)); + + /* 9) same with a length past the cap: the over-limit scan must never + * run without a destination */ + expect("NULL destination, long tail", + av_dynamic_hdr_plus_from_t35(NULL, long_tail, long_size), + AVERROR(EINVAL)); + + /* 10) a NULL data pointer must be refused, not scanned: an over-limit + * length used to dereference data[i] directly */ + expect("NULL data, long tail", + av_dynamic_hdr_plus_from_t35(b, NULL, long_size), + AVERROR(EINVAL)); + + /* 11) ditto exactly at the cap: this one reaches memcpy() even in code + * without the over-limit scan */ + expect("NULL data, cap bytes", + av_dynamic_hdr_plus_from_t35(b, NULL, CAP), + AVERROR(EINVAL)); + + if (failures) { + printf("%d failure(s)\n", failures); + goto end; + } + printf("all boundary checks passed\n"); + err = 0; + +end: + av_free(broken); + av_free(long_tail); + av_free(one_extra); + av_free(atcap); + av_free(t35); + av_free(src); + av_free(a); + av_free(b); + return err; +} diff --git a/tests/fate/api.mak b/tests/fate/api.mak index 1659c7979c0..eb6841dac0f 100644 --- a/tests/fate/api.mak +++ b/tests/fate/api.mak @@ -54,6 +54,11 @@ FATE_API_LIBAVFORMAT-$(call ALLYES, MP4_MUXER ISMV_MUXER) += fate-api-movenc fate-api-movenc: $(APITESTSDIR)/api-movenc-test$(EXESUF) fate-api-movenc: CMD = run $(APITESTSDIR)/api-movenc-test$(EXESUF) +FATE_API-yes += fate-api-hdr10plus +fate-api-hdr10plus: $(APITESTSDIR)/api-hdr10plus-test$(EXESUF) +fate-api-hdr10plus: CMD = run $(APITESTSDIR)/api-hdr10plus-test$(EXESUF) +fate-api-hdr10plus: CMP = null + FATE_API-$(HAVE_THREADS) += fate-api-threadmessage fate-api-threadmessage: $(APITESTSDIR)/api-threadmessage-test$(EXESUF) fate-api-threadmessage: CMD = run $(APITESTSDIR)/api-threadmessage-test$(EXESUF) 3 10 30 50 2 20 40 -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
