> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > Ridley Combs via ffmpeg-devel > Sent: Dienstag, 15. April 2025 02:03 > To: ffmpeg-devel <ffmpeg-devel@ffmpeg.org> > Cc: Ridley Combs <rco...@rcombs.me> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that > decode_str() did advance > > > > > On Apr 15, 2025, at 08:59, softworkz . <softworkz-at- > hotmail....@ffmpeg.org> wrote: > > > > > > > >> -----Original Message----- > >> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org <mailto:ffmpeg- > devel-boun...@ffmpeg.org>> On Behalf Of > >> Michael Niedermayer > >> Sent: Dienstag, 15. April 2025 01:20 > >> To: FFmpeg development discussions and patches <ffmpeg- > >> de...@ffmpeg.org <mailto:de...@ffmpeg.org>> > >> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that > >> decode_str() did advance > >> > >> On Sat, Apr 12, 2025 at 01:49:53AM +0000, softworkz . wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of > >>>> Michael Niedermayer > >>>> Sent: Samstag, 12. April 2025 00:27 > >>>> To: FFmpeg development discussions and patches <ffmpeg- > >> de...@ffmpeg.org> > >>>> Subject: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that > >>>> decode_str() did advance > >>>> > >>>> Fixes infinite loop with unknown encodings > >>>> > >>>> We could alternatively error out from decode_str() or consume all > >> of > >>>> taglen > >>>> this would affect other callers though. > >>>> > >>>> Fixes: 409819224/clusterfuzz-testcase-minimized- > >> ffmpeg_dem_H261_fuzzer- > >>>> 6003527535362048 > >>>> Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> > >>>> --- > >>>> libavformat/id3v2.c | 3 +++ > >>>> 1 file changed, 3 insertions(+) > >>>> > >>>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c > >>>> index 90314583a74..e3f7f9e2a90 100644 > >>>> --- a/libavformat/id3v2.c > >>>> +++ b/libavformat/id3v2.c > >>>> @@ -341,10 +341,13 @@ static void read_ttag(AVFormatContext *s, > >>>> AVIOContext *pb, int taglen, > >>>> taglen--; /* account for encoding type byte */ > >>>> > >>>> while (taglen > 1) { > >>>> + int current_taglen = taglen; > >>>> if (decode_str(s, pb, encoding, &dst, &taglen) < 0) { > >>>> av_log(s, AV_LOG_ERROR, "Error reading frame %s, > >>>> skipped\n", key); > >>>> return; > >>>> } > >>>> + if (current_taglen == taglen) > >>>> + return; > >>>> > >>>> count++; > >>>> > >>>> -- > >>>> 2.49.0 > >>>> > >>>> _______________________________________________ > >>> > >>> Hi Michael, > >>> > >>> this kind of conflicts with this patch that I had submitted > >> recently: > >>> > >>> > >> > https://patchwork.ffmpeg.org/project/ffmpeg/patch/pull.54.ffstaging.FF > >> mpeg.1740873449247.ffmpegag...@gmail.com/ > >>> > >>> > >>> I wonder whether my patch would still be prone to the issue your > >> patch is addressing - > >> > >> This already conflicts with rcombs patch in git master, i think > >> Applying: Fixes Trac ticket https://trac.ffmpeg.org/ticket/6949 > >> Using index info to reconstruct a base tree... > >> M libavformat/id3v2.c > >> Falling back to patching base and 3-way merge... > >> Auto-merging libavformat/id3v2.c > >> CONFLICT (content): Merge conflict in libavformat/id3v2.c > >> error: Failed to merge in the changes. > >> Patch failed at 0001 Fixes Trac ticket > >> https://trac.ffmpeg.org/ticket/6949 > >> > >> > >>> do you have a test file perhaps? > >> > >> Will email you one, but the loop with a function that doesnt > advance > >> is an issue even if the specific file doesnt trigger it in a > different > >> implementation > >> > >> also probaly a good idea if you contact rcombs as you seemed to > work > >> on > >> the same code > >> > >> I was looking at teh ticket and saw a link to rcombs patch, looked > at > >> the patch and applied it. I did not realize there where 2 patches > > > > > > Hi Michael, > > > > I know the rcombs patch, but it has a - let's say - different > behavior. > > Let's look at an example where artist and genre have multiple > values: > > > > > > This was ffmpeg output unpatched: > > > > Metadata: > > title : Infinite (Original Mix) > > artist : B-Front > > track : 1 > > album : Infinite > > date : 2017 > > genre : Hardstyle > > TBPM : 150 > > compilation : 0 > > album_artist : B-Front > > publisher : Roughstate > > > > > > This is what the rcombs patch does: > > > > Metadata: > > title : Infinite (Original Mix) > > artist : B-Front > > artist : Second Artist Example > > track : 1 > > album : Infinite > > date : 2017 > > genre : Hardstyle > > genre : Test > > genre : Example > > genre : Hard Dance > > TBPM : 150 > > compilation : 0 > > album_artist : B-Front > > publisher : Roughstate > > > > > > > > My path does that: > > > > Metadata: > > title : Infinite (Original Mix) > > artist : B-Front;Second Artist Example > > track : 1 > > album : Infinite > > date : 2017 > > genre : Hardstyle;Test;Example;Hard Dance > > TBPM : 150 > > compilation : 0 > > album_artist : B-Front > > publisher : Roughstate > > > > > > > > I'm not sure whether it is even allowed or intended that there are > > multiple metadata entries with the same key? > > It is indeed an intended feature of the AVDictionary system, and the > metadata feature in particular.
Hi, you meant to say that it is an intended change introduced by your patch? 😉 Because we haven't had duplicate metadata keys before. I'm afraid but I think these changes should be reverted, because it also creates invalid FFprobe output - like in case of JSON for example: { "format": { "filename": "multiple_id3v2_4_values.mp3", "nb_streams": 1, "nb_programs": 0, "nb_stream_groups": 0, "format_name": "mp3", "format_long_name": "MP2/3 (MPEG audio layer 2/3)", "start_time": "0.011995", "duration": "20.035918", "size": "804936", "bit_rate": "321397", "probe_score": 51, "tags": { "title": "Infinite (Original Mix)", "artist": "B-Front", "artist": "Second Artist Example", "track": "1", "album": "Infinite", "date": "2017", "genre": "Hardstyle", "genre": "Test", "genre": "Example", "genre": "Hard Dance", "TBPM": "150", "compilation": "0", "album_artist": "B-Front", "publisher": "Roughstate", "encoder": "Lavf57.83.100", } } } Thanks, sw _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".