[FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance
softworkz .
softworkz at hotmail.com
Tue Apr 15 03:17:32 EEST 2025
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Ridley Combs via ffmpeg-devel
> Sent: Dienstag, 15. April 2025 02:03
> To: ffmpeg-devel <ffmpeg-devel at ffmpeg.org>
> Cc: Ridley Combs <rcombs at 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.com at ffmpeg.org> wrote:
> >
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org <mailto:ffmpeg-
> devel-bounces at ffmpeg.org>> On Behalf Of
> >> Michael Niedermayer
> >> Sent: Dienstag, 15. April 2025 01:20
> >> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel at ffmpeg.org <mailto:devel at 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-bounces at ffmpeg.org> On Behalf Of
> >>>> Michael Niedermayer
> >>>> Sent: Samstag, 12. April 2025 00:27
> >>>> To: FFmpeg development discussions and patches <ffmpeg-
> >> devel at 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 <michael at 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.ffmpegagent at 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
More information about the ffmpeg-devel
mailing list