Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
From: "softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>
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
Date: Mon, 14 Apr 2025 23:59:02 +0000
Message-ID: <DM8P223MB0365928500AB53069869DC99BAB32@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20250414231950.GN4991@pb2>



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

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







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

  reply	other threads:[~2025-04-14 23:59 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-04-11 22:27 [FFmpeg-devel] [PATCH 1/2] avformat/id3v2: Print the unknown encoding Michael Niedermayer
2025-04-11 22:27 ` [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance Michael Niedermayer
2025-04-12  1:49   ` softworkz .
2025-04-14 23:19     ` Michael Niedermayer
2025-04-14 23:59       ` softworkz . [this message]
2025-04-15  0:02         ` Ridley Combs via ffmpeg-devel
2025-04-15  0:17           ` softworkz .
2025-04-15 18:32             ` Michael Niedermayer
2025-04-15 19:03               ` Nicolas George
2025-04-15 20:12                 ` softworkz .
2025-04-15 20:47                   ` Nicolas George
2025-04-15 22:20                     ` softworkz .
2025-04-16  5:15                       ` Nicolas George
2025-04-16  5:59                         ` softworkz .
2025-04-16  7:02                           ` Nicolas George
2025-04-15 19:15               ` softworkz .
2025-04-15 19:02           ` Nicolas George
2025-04-16  8:02           ` Andreas Rheinhardt
2025-04-15 18:55         ` Michael Niedermayer
2025-04-15 19:59           ` softworkz .
2025-04-15 22:50             ` Michael Niedermayer
2025-04-15 22:59               ` softworkz .
2025-04-15 23:01                 ` softworkz .
2025-04-16  0:53                   ` Michael Niedermayer
2025-04-16  1:01                     ` softworkz .
2025-04-16  2:39                     ` softworkz .
2025-04-16  7:04                       ` Nicolas George
2025-04-16 11:07                       ` Michael Niedermayer
2025-04-16 11:15                         ` softworkz .
2025-04-16  1:21                 ` Michael Niedermayer
2025-04-16  1:29                   ` softworkz .
2025-04-16  1:33                     ` Michael Niedermayer
2025-04-16  2:18                       ` softworkz .
2025-04-16  2:31                       ` softworkz .
2025-04-16 10:41                         ` Michael Niedermayer
2025-04-16 10:59                           ` softworkz .
2025-04-16  2:52                       ` softworkz .
2025-04-16 10:53                         ` Michael Niedermayer
2025-04-16 10:57                           ` Andreas Rheinhardt
2025-04-16 11:02                             ` Michael Niedermayer
2025-04-16 10:58                           ` softworkz .
2025-04-15  1:37       ` softworkz .
2025-04-15 18:25         ` Michael Niedermayer
2025-04-15 19:07           ` softworkz .

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM8P223MB0365928500AB53069869DC99BAB32@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM \
    --to=softworkz-at-hotmail.com@ffmpeg.org \
    --cc=ffmpeg-devel@ffmpeg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git