[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


Thanks a lot for the test file. I was able to reproduce the eternal loop
that you were intending to fix, but I noticed that after removing the 
patches from rcombs, that endless loop doesn't happen in the first place.

Best,
sw