From e828b52165440b885d3faf48fd70530fc8fd28ea Mon Sep 17 00:00:00 2001 From: Jun Zhao Date: Fri, 23 Oct 2026 07:06:02 +0800 Subject: [PATCH] avformat/id3v2: honor the tag version in CHAP frame sizes ID3v2.3 stores a frame size as a plain 32-bit integer. ID3v2.4 stores it as a synchsafe integer. write_chapter() always wrote the plain form, and read_chapter() always read it that way. Once an embedded text frame reached 128 bytes, the size no longer fitted and the chapter was dropped. Write the CHAP header with the same version rule as other frames. Read an embedded frame with that rule, and keep the chapter when a subframe size does not fit. Fixes: https://code.ffmpeg.org/FFmpeg/FFmpeg/issues/25357 Signed-off-by: Jun Zhao --- libavformat/id3v2.c | 42 ++++++++++++++++++++++++++---------------- libavformat/id3v2enc.c | 26 ++++++++++++++++---------- 2 files changed, 42 insertions(+), 26 deletions(-) diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c index 5ae931da9b..59493033db 100644 --- a/libavformat/id3v2.c +++ b/libavformat/id3v2.c @@ -487,7 +487,7 @@ static void list_append(ID3v2ExtraMeta *new_elem, ExtraMetaList *list) * Parse GEOB tag into a ID3v2ExtraMetaGEOB struct. */ static void read_geobtag(AVFormatContext *s, AVIOContext *pb, int taglen, - const char *tag, ExtraMetaList *extra_meta, int isv34) + const char *tag, ExtraMetaList *extra_meta, int version) { ID3v2ExtraMetaGEOB *geob_data = NULL; ID3v2ExtraMeta *new_extra = NULL; @@ -618,7 +618,7 @@ static void rstrip_spaces(char *buf) } static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen, - const char *tag, ExtraMetaList *extra_meta, int isv34) + const char *tag, ExtraMetaList *extra_meta, int version) { int enc, pic_type; char mimetype[64] = {0}; @@ -628,7 +628,7 @@ static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen, ID3v2ExtraMeta *new_extra = NULL; int64_t end = avio_tell(pb) + taglen; - if (taglen <= 4 || (!isv34 && taglen <= 6)) + if (taglen <= 4 || (version == 2 && taglen <= 6)) goto fail; new_extra = av_mallocz(sizeof(*new_extra)); @@ -641,7 +641,7 @@ static void read_apic(AVFormatContext *s, AVIOContext *pb, int taglen, taglen--; /* mimetype */ - if (isv34) { + if (version != 2) { int ret = avio_get_str(pb, taglen, mimetype, sizeof(mimetype)); if (ret < 0 || ret >= taglen) goto fail; @@ -714,9 +714,8 @@ static void free_chapter(void *obj) } static void read_chapter(AVFormatContext *s, AVIOContext *pb, int len, - const char *ttag, ExtraMetaList *extra_meta, int isv34) + const char *ttag, ExtraMetaList *extra_meta, int version) { - int taglen; char tag[5]; ID3v2ExtraMeta *new_extra = NULL; ID3v2ExtraMetaCHAP *chap = NULL; @@ -739,19 +738,30 @@ static void read_chapter(AVFormatContext *s, AVIOContext *pb, int len, len -= 16; while (len > 10) { + unsigned tlen; + if (avio_read(pb, tag, 4) < 4) - goto fail; + break; tag[4] = 0; - taglen = avio_rb32(pb); + /* Embedded frames use the same header as the surrounding tag. + * 2.2 has no CHAP. 2.3 sizes are plain; 2.4 sizes are synchsafe. */ + if (version == 4) + tlen = get_size(pb, 4); + else + tlen = avio_rb32(pb); avio_skip(pb, 2); len -= 10; - if (taglen < 0 || taglen > len) - goto fail; + if (tlen > (unsigned)len) { + av_log(s, AV_LOG_WARNING, + "Invalid ID3v2 chapter subframe %.4s size %u, keeping the chapter\n", + tag, tlen); + break; + } if (tag[0] == 'T') - read_ttag(s, pb, taglen, &chap->meta, tag); + read_ttag(s, pb, tlen, &chap->meta, tag); else - avio_skip(pb, taglen); - len -= taglen; + avio_skip(pb, tlen); + len -= tlen; } ff_metadata_conv(&chap->meta, NULL, ff_id3v2_34_metadata_conv); @@ -775,7 +785,7 @@ static void free_priv(void *obj) } static void read_priv(AVFormatContext *s, AVIOContext *pb, int taglen, - const char *tag, ExtraMetaList *extra_meta, int isv34) + const char *tag, ExtraMetaList *extra_meta, int version) { ID3v2ExtraMeta *meta; ID3v2ExtraMetaPRIV *priv; @@ -813,7 +823,7 @@ typedef struct ID3v2EMFunc { const char *tag4; void (*read)(AVFormatContext *s, AVIOContext *pb, int taglen, const char *tag, ExtraMetaList *extra_meta, - int isv34); + int version); void (*free)(void *obj); } ID3v2EMFunc; @@ -1092,7 +1102,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata, read_lang_descr_tag(s, pbx, "comment", tlen, metadata); else /* parse special meta tag */ - extra_func->read(s, pbx, tlen, tag, extra_meta, isv34); + extra_func->read(s, pbx, tlen, tag, extra_meta, version); } else if (!tag[0]) { if (tag[1]) av_log(s, AV_LOG_WARNING, "invalid frame id, assuming padding\n"); diff --git a/libavformat/id3v2enc.c b/libavformat/id3v2enc.c index 9206af8837..c538d13829 100644 --- a/libavformat/id3v2enc.c +++ b/libavformat/id3v2enc.c @@ -60,6 +60,18 @@ static void id3v2_encode_string(AVIOContext *pb, const uint8_t *str, put(pb, str); } +/* ID3v2.3 frame size is not synchsafe; 2.4 is. */ +static void id3v2_put_frame_header(ID3v2EncContext *id3, AVIOContext *pb, + uint32_t tag, int len, int flags) +{ + avio_wb32(pb, tag); + if (id3->version == 3) + avio_wb32(pb, len); + else + id3v2_put_size(pb, len); + avio_wb16(pb, flags); +} + static int id3v2_put_frame(ID3v2EncContext *id3, AVIOContext *avioc, AVIOContext *dyn_buf, const uint32_t tag, const uint8_t flags) { @@ -67,13 +79,7 @@ static int id3v2_put_frame(ID3v2EncContext *id3, AVIOContext *avioc, AVIOContext uint8_t *pb; len = avio_get_dyn_buf(dyn_buf, &pb); - avio_wb32(avioc, tag); - /* ID3v2.3 frame size is not sync-safe */ - if (id3->version == 3) - avio_wb32(avioc, len); - else - id3v2_put_size(avioc, len); - avio_wb16(avioc, flags); + id3v2_put_frame_header(id3, avioc, tag, len, flags); avio_write(avioc, pb, len); id3->len += len + ID3v2_HEADER_SIZE; @@ -463,9 +469,9 @@ static int write_chapter(AVFormatContext *s, AVIOContext *pb, len = avio_get_dyn_buf(dyn_bc, &dyn_buf); id3->len += 16 + ID3v2_HEADER_SIZE; - avio_wb32(pb, MKBETAG('C', 'H', 'A', 'P')); - avio_wb32(pb, len); - avio_wb16(pb, 0); + /* Nested frames already updated id3->len, so this header is written + * directly instead of through id3v2_put_frame(). */ + id3v2_put_frame_header(id3, pb, MKBETAG('C', 'H', 'A', 'P'), len, 0); avio_write(pb, dyn_buf, len); fail: