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

[softworkz .]

> -----Original Message-----
> From: ffmpeg-devel <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>
> 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?

But in any case, what's happening with the rcombs patch is that 
when transcoding a file, the result looks like this:

  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


Which means that the extra values all get lost. This doesn't happen
with the patch that I have submitted. Even though we should ideally
also modify the encoder to write the semicolon-delimited values with
null-separation, a semicolon is treated by most apps as a value 
delimiter as well - so it's a largely solid way and it avoids loss of
information when transcoding.

(which is why I had submitted the patch even though I knew about 
the one from rcombs)

Thanks
sw