PR #25388 opened by Jun Zhao (mypopydev) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25388 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25388.patch
This follow-up series fixes two pre-existing ID3v2 parser alignment and boundary issues: read_chapter() now skips any unread trailing bytes after read_ttag() so multi-string or padded text subframes do not misalign subsequent embedded subframes, and id3v2_parse() tightens its ID3v2.4 non-synchsafe frame-size heuristic by rejecting values with high bits set as synchsafe candidates and verifying that a candidate next-frame boundary fits within the remaining tag bytes. fate-id3v2chap is extended to cover both cases. >From fb8f1271dcf001cb261303d3b838adb352da23b9 Mon Sep 17 00:00:00 2001 From: Jun Zhao <[email protected]> Date: Fri, 23 Oct 2026 21:16:41 +0800 Subject: [PATCH 1/3] avformat/id3v2: skip unread bytes after chapter text subframes read_ttag() stops at the first NUL terminator or on decoding errors without consuming the remainder of the declared frame payload. Subtracting tlen in read_chapter() without advancing pb over the unread bytes leaves the stream misaligned for any subsequent embedded subframe. Track the number of bytes consumed by read_ttag() and skip the remainder of tlen before reading the next subframe header. Signed-off-by: Jun Zhao <[email protected]> --- libavformat/id3v2.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 59493033db3..22a15ab7947 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -739,6 +739,7 @@ static void read_chapter(AVFormatContext *s, AVIOContext *pb, int len, len -= 16; while (len > 10) { unsigned tlen; + int64_t start, consumed; if (avio_read(pb, tag, 4) < 4) break; @@ -757,10 +758,13 @@ static void read_chapter(AVFormatContext *s, AVIOContext *pb, int len, tag, tlen); break; } + start = avio_tell(pb); if (tag[0] == 'T') read_ttag(s, pb, tlen, &chap->meta, tag); - else - avio_skip(pb, tlen); + consumed = avio_tell(pb) - start; + if (consumed < 0 || consumed > tlen || + avio_skip(pb, tlen - consumed) < 0) + break; len -= tlen; } -- 2.54.0 >From 4a0bf34c1ba42c97a687cb1ef60125fddb659425 Mon Sep 17 00:00:00 2001 From: Jun Zhao <[email protected]> Date: Fri, 23 Oct 2026 21:40:46 +0800 Subject: [PATCH 2/3] avformat/id3v2: tighten v2.4 non-synchsafe frame size boundary check A valid synchsafe integer has the MSB cleared in every byte, and a candidate next-frame boundary inside an ID3v2.4 tag must be followed by the end of the tag, zero padding, or a plausible 10-byte frame header whose size fits in the remaining tag bytes. Previously, size_to_syncsafe(tlen) was probed even when tlen had bit 7 set in one of its bytes (such as 0x000000b6), and check_tag() accepted any four uppercase ASCII characters (such as "LLLL" inside a chapter title) as a valid next frame ID without checking the following frame size. It could also read past the end of the ID3v2 tag when the last frame had fewer than 4 bytes of padding. Check tlen with is_syncsafe() before probing size_to_syncsafe(tlen), bound the probe to the remaining tag bytes, and verify in check_tag() that a candidate frame header fits in the remaining tag length. Signed-off-by: Jun Zhao <[email protected]> --- libavformat/id3v2.c | 59 +++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 22a15ab7947..143d5fdefc5 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -182,6 +182,14 @@ static unsigned int get_size(AVIOContext *s, int len) return v; } +static int is_syncsafe(unsigned int size) +{ + return !(size & ((0x80U << 0) | + (0x80U << 8) | + (0x80U << 16) | + (0x80U << 24))); +} + static unsigned int size_to_syncsafe(unsigned int size) { return (((size) & (0x7f << 0)) >> 0) + @@ -208,20 +216,34 @@ static int is_tag(const char *buf, unsigned int len) } /** - * Return 1 if the tag of length len at the given offset is valid, 0 if not, -1 on error + * Return 1 if the candidate frame boundary at offset (with rem bytes left in + * the ID3v2 tag after it) is followed by the end of the tag, zero padding, or + * a plausible 10-byte ID3v2.3/2.4 frame header; 0 if not, -1 on error. */ -static int check_tag(AVIOContext *s, int offset, unsigned int len) +static int check_tag(AVIOContext *s, int64_t offset, unsigned int rem) { char tag[4]; + unsigned int nsize, nlen; - if (len > 4 || - avio_seek(s, offset, SEEK_SET) < 0 || - avio_read(s, tag, len) < (int)len) - return -1; - else if (!AV_RB32(tag) || is_tag(tag, len)) + if (!rem) return 1; - - return 0; + if (avio_seek(s, offset, SEEK_SET) < 0) + return -1; + if (rem < sizeof(tag)) { + while (rem--) + if (avio_r8(s)) + return 0; + return 1; + } + if (avio_read(s, tag, sizeof(tag)) < (int)sizeof(tag)) + return -1; + if (!AV_RB32(tag)) + return 1; + if (rem < ID3v2_HEADER_SIZE || !is_tag(tag, sizeof(tag))) + return 0; + nsize = avio_rb32(s); + nlen = is_syncsafe(nsize) ? size_to_syncsafe(nsize) : nsize; + return nlen > 0 && nlen <= rem - ID3v2_HEADER_SIZE; } /** @@ -944,18 +966,23 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, * so check the next tag to see which one to use */ tlen = avio_rb32(pb); if (tlen > 0x7f) { - if (tlen < len) { - int64_t cur = avio_tell(pb); + unsigned int rem = len - taghdrlen; - if (ffio_ensure_seekback(pb, 2 /* tflags */ + tlen + 4 /* next tag */)) + if (tlen <= rem) { + int64_t cur = avio_tell(pb); + unsigned int ss_tlen = size_to_syncsafe(tlen); + + if (ffio_ensure_seekback(pb, 2 /* tflags */ + + FFMIN(rem, tlen + ID3v2_HEADER_SIZE))) break; - if (check_tag(pb, cur + 2 + size_to_syncsafe(tlen), 4) == 1) - tlen = size_to_syncsafe(tlen); - else if (check_tag(pb, cur + 2 + tlen, 4) != 1) + if (is_syncsafe(tlen) && + check_tag(pb, cur + 2 + ss_tlen, rem - ss_tlen) == 1) + tlen = ss_tlen; + else if (check_tag(pb, cur + 2 + tlen, rem - tlen) != 1) break; avio_seek(pb, cur, SEEK_SET); - } else + } else if (is_syncsafe(tlen)) tlen = size_to_syncsafe(tlen); } } -- 2.54.0 >From 4b221795dcfee75841cba17f1e6342e16cc895ec Mon Sep 17 00:00:00 2001 From: Jun Zhao <[email protected]> Date: Fri, 23 Oct 2026 21:40:46 +0800 Subject: [PATCH 3/3] tests/fate: cover chapter subframe alignment and non-synchsafe v2.4 CHAP Extend fate-id3v2chap to verify that: - unread trailing bytes after a NUL-terminated text subframe (TPE1) are skipped so both TPE1 ("artist") and the following embedded subframe (TIT2) are parsed; - a non-conforming v2.4 CHAP frame with a plain 32-bit size whose high bit is set (0x000000b6) is not truncated when the stripped offset lands on uppercase ASCII inside the chapter title. Signed-off-by: Jun Zhao <[email protected]> --- libavformat/tests/id3v2chap.c | 45 ++++++++++++++++++++++++++++++----- tests/ref/fate/id3v2chap | 12 ++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/libavformat/tests/id3v2chap.c b/libavformat/tests/id3v2chap.c index 760255bc177..a7b9bc700cb 100644 --- a/libavformat/tests/id3v2chap.c +++ b/libavformat/tests/id3v2chap.c @@ -47,11 +47,15 @@ static int readback(const uint8_t *buf, int size) printf("chapters=%u\n", in->nb_chapters); for (unsigned i = 0; i < in->nb_chapters; i++) { const AVChapter *ch = in->chapters[i]; - const AVDictionaryEntry *title = av_dict_get(ch->metadata, "title", NULL, 0); + const AVDictionaryEntry *title = av_dict_get(ch->metadata, "title", NULL, 0); + const AVDictionaryEntry *artist = av_dict_get(ch->metadata, "artist", NULL, 0); - printf("chapter=%u time_base=%d/%d start=%"PRId64" end=%"PRId64" title=%s\n", + printf("chapter=%u time_base=%d/%d start=%"PRId64" end=%"PRId64" title=%s", i, ch->time_base.num, ch->time_base.den, ch->start, ch->end, title ? title->value : "(missing)"); + if (artist) + printf(" artist=%s", artist->value); + printf("\n"); } ret = 0; @@ -104,12 +108,41 @@ static int roundtrip(int version) if (readback(buf, size)) goto done; - /* Locate the first embedded TIT2 header inside the generated chapter. - * Give it an oversized length, leaving the chapter and following CHAP - * intact. The reader should retain both chapter timings. */ + /* Non-conforming v2.4 CHAP with a plain 32-bit size (0x000000b6) whose + * high bit is set and whose payload contains uppercase ASCII at the + * stripped offset (0x36). */ + if (version == 4 && chap_pos >= 0) { + buf[chap_pos + 6] = 0x00; + buf[chap_pos + 7] = 0xb6; + printf("non-synchsafe CHAP version=4\n"); + if (readback(buf, size)) + goto done; + buf[chap_pos + 6] = 0x01; + buf[chap_pos + 7] = 0x36; + } + + /* Split the 162-byte TIT2 subframe into a 22-byte TPE1 subframe with + * unread trailing bytes after its first NUL terminator, followed by a + * 140-byte TIT2 subframe, then test an oversized subframe length. */ if (chap_pos >= 0) { - for (int i = chap_pos + 10; i + 10 <= size; i++) { + for (int i = chap_pos + 10; i + 33 <= size; i++) { if (!memcmp(buf + i, "TIT2", 4)) { + uint8_t enc = buf[i + 10]; + memcpy(buf + i, "TPE1", 4); + buf[i + 6] = 0; + buf[i + 7] = 12; + buf[i + 13] = 0; + memcpy(buf + i + 22, "TIT2\x00\x00", 6); + buf[i + 28] = version == 4 ? 0x01 : 0x00; + buf[i + 29] = version == 4 ? 0x02 : 0x82; + buf[i + 30] = 0; + buf[i + 31] = 0; + buf[i + 32] = enc; + printf("trailing text subframe version=%d\n", version); + if (readback(buf, size)) + goto done; + + memcpy(buf + i, "TIT2", 4); memset(buf + i + 4, 0x7f, 4); printf("invalid subframe version=%d\n", version); if (readback(buf, size)) diff --git a/tests/ref/fate/id3v2chap b/tests/ref/fate/id3v2chap index 512fbd3bd73..0897b00ea01 100644 --- a/tests/ref/fate/id3v2chap +++ b/tests/ref/fate/id3v2chap @@ -3,6 +3,10 @@ CHAP size bytes=00 00 00 b6 chapters=2 chapter=0 time_base=1/1000 start=0 end=1000 title=LLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLL chapter=1 time_base=1/1000 start=1000 end=2000 title=End +trailing text subframe version=3 +chapters=2 +chapter=0 time_base=1/1000 start=0 end=1000 title=LLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLL artist=LL +chapter=1 time_base=1/1000 start=1000 end=2000 title=End invalid subframe version=3 chapters=2 chapter=0 time_base=1/1000 start=0 end=1000 title=ch0 @@ -12,6 +16,14 @@ CHAP size bytes=00 00 01 36 chapters=2 chapter=0 time_base=1/1000 start=0 end=1000 title=LLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLL chapter=1 time_base=1/1000 start=1000 end=2000 title=End +non-synchsafe CHAP version=4 +chapters=2 +chapter=0 time_base=1/1000 start=0 end=1000 title=LLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLL +chapter=1 time_base=1/1000 start=1000 end=2000 title=End +trailing text subframe version=4 +chapters=2 +chapter=0 time_base=1/1000 start=0 end=1000 title=LLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLLL artist=LL +chapter=1 time_base=1/1000 start=1000 end=2000 title=End invalid subframe version=4 chapters=2 chapter=0 time_base=1/1000 start=0 end=1000 title=ch0 -- 2.54.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
