On Tue, Apr 15, 2025 at 12:17:32AM +0000, softworkz . wrote: > > > > -----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", > } > } > }
I think ffprobe should not generate invalid output if metadata given to it has 2 entries with the same key, independant of that being the correct way to export metadata. thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Nations do behave wisely once they have exhausted all other alternatives. -- Abba Eban
signature.asc
Description: PGP signature
_______________________________________________ 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".
