Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance

["softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Michael Niedermayer
> Sent: Dienstag, 15. April 2025 20:33
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@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@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 <rcombs@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@ffmpeg.org> wrote:
> > > >
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org
> <mailto:ffmpeg-
> > > devel-bounces@ffmpeg.org>> On Behalf Of
> > > >> Michael Niedermayer
> > > >> Sent: Dienstag, 15. April 2025 01:20
> > > >> To: FFmpeg development discussions and patches <ffmpeg-
> > > >> devel@ffmpeg.org <mailto:devel@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@ffmpeg.org> On
> Behalf Of
> > > >>>> Michael Niedermayer
> > > >>>> Sent: Samstag, 12. April 2025 00:27
> > > >>>> To: FFmpeg development discussions and patches <ffmpeg-
> > > >> devel@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@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@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


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".