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

[Ridley Combs via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

> On Apr 15, 2025, at 08:59, softworkz . <softworkz-at-hotmail.com@ffmpeg.org> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org <mailto: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 <mailto: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?

It is indeed an intended feature of the AVDictionary system, and the metadata feature in particular.

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

This is an issue that should be resolved by supporting multiple entries all the way through the relevant ffmpeg.c and muxer code paths, rather than adding new fragile usage of in-band signaling within metadata strings.

> 
> (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 <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org <mailto:ffmpeg-devel-request@ffmpeg.org> with subject "unsubscribe".

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