[FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance
softworkz .
softworkz at hotmail.com
Tue Apr 15 22:15:25 EEST 2025
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Dienstag, 15. April 2025 20:33
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that
> decode_str() did advance
>
> On Tue, Apr 15, 2025 at 12:17:32AM +0000, softworkz . wrote:
> >
> >
> > > -----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",
> > }
> > }
> > }
>
> 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.
The problem is not at the side of FFprobe, it's the JSON format which
doesn't allow to express duplicate keys in that way.
(also applies to the upcoming YML output format)
The only thing that FFprobe could do is to suppress duplicate-key entries
but that would lead to an incomplete output of data. Since the code which
outputs the data is agnostic of the text formatter, the duplicate key entries
would be suppressed for all output formats. In the end, nothing would be
won because the output would be the same as without the rcombs patch.
In order to allow duplicate keys, the output format would need a general
change for metadata, like to an array of items (with key and value).
But this would be a breaking change for everybody who uses the metadata
information from FFprobe output in whatever way.
Best,
sw
More information about the ffmpeg-devel
mailing list