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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 3188 bytes --]

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.FFmpeg.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

thx


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

The day soldiers stop bringing you their problems is the day you have stopped 
leading them. They have either lost confidence that you can help or concluded 
you do not care. Either case is a failure of leadership. - Colin Powell

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".