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

[Michael Niedermayer]

On Mon, Apr 14, 2025 at 11:59:02PM +0000, softworkz . wrote:
> 
> 
> > -----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

Iam perfectly fine with either way
but i have to point out that the 2nd method has some problems too

for example checking if foo is an author becomes more difficult
theres also a question about scalability if there are many entries

And what exactly do you do if an Artist or Title itself contains a ;


Thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250415/de14615d/attachment.sig>