From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <ffmpeg-devel-bounces@ffmpeg.org> Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTPS id CE52C4CE8E for <ffmpegdev@gitmailbox.com>; Tue, 15 Apr 2025 00:03:13 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3A056687CB5; Tue, 15 Apr 2025 03:03:08 +0300 (EEST) Received: from mail-pf1-f180.google.com (mail-pf1-f180.google.com [209.85.210.180]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id E2B95687C57 for <ffmpeg-devel@ffmpeg.org>; Tue, 15 Apr 2025 03:03:00 +0300 (EEST) Received: by mail-pf1-f180.google.com with SMTP id d2e1a72fcca58-7376dd56f8fso5999702b3a.2 for <ffmpeg-devel@ffmpeg.org>; Mon, 14 Apr 2025 17:03:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1744675379; x=1745280179; h=message-id:in-reply-to:to:references:date:subject:mime-version:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=3AkkpxcFF+cLzI3s18mARy25E6CVUSzEbEfG2acqhd0=; b=NsSZb+Zf+Fa6Q0DJqGaKdbtINbrpLnvH4E56DG3UvmZceU/zJVednyCX19DnTlWo7y MirfESh5o4rfDLpxjEpJl/mGn5JwJj/SgoiV6Dm8UzvQ13t3f4b3sPMHbTmMnFin3IF0 kh3Rk5YqbIIuPi08mtnuhqeYftO6HMBWUdkK/TfDJE5LeXIFqGuETA7VtD7xPesnHhiL umL5iQxYQvlpsrHkd5Ud7EXWwBWdMA9eU0/LZdRSnEnyi5lrcirrRFpljNVEp+m32k/7 ajrwBTrmnriYbRmLmHG8csRDWZlb1swy5uihABm31XH24zBGJmCNTL5/5sZn9miqNbnw ZeEw== X-Gm-Message-State: AOJu0Ywqu9b9ud/MfemTm5OHT8uqPnAgaxajhHwcXsLK8lEaqsFjytRG oJFIMJwI9gSjJBiH7xL87hU4aRRbBUpz9iCUEXpQkSalXO+6KLp+4OFPZeVvatqHn/PkLf82sA8 Abw== X-Gm-Gg: ASbGncs/66Q/7oxjTwi98BguASSSk0wS0ugsH6SKDCi0kFPy2FuOvsuZVbzcXz+ZMo7 ZDrGfIGQJ5HnYVg7jYDve1e70DMXmdpcbSVfs7aIKswvow250u8L1bPWm7I6jjOtY1O+rrQPaFy zyka/n5EURsM+V+H3dW2nRufORq5h4SJ2w/PvZv3fMyyG8LnPF72trlbvUPNIc9QLUm9FEtZR48 C4runds6w0Mh80YHF4HoHp/8ACGui1SCzCVVfU4Fsu/o4FGxRXL4nbVVEF1RDwagfLSbLDKbOCX VQq2U7pL6n3hGLQZapSd38yHcgppWr54Iz2KF6PS9i5BhoYMkEtcYTeBvrsPoQ== X-Google-Smtp-Source: AGHT+IGhaYZ/74vMAmUYSHtiec8xARLE14Ei3yPfocF+EY+UmGSiI6mdYBAWmFoxUQfQr2z7lrE7Tg== X-Received: by 2002:a05:6a21:9981:b0:1fa:995a:5004 with SMTP id adf61e73a8af0-20179921cd4mr16621170637.26.1744675378050; Mon, 14 Apr 2025 17:02:58 -0700 (PDT) Received: from smtpclient.apple ([2405:6580:bf61:6a00:589:4b09:cc91:fea6]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-b02a2d3abffsm8253409a12.48.2025.04.14.17.02.56 for <ffmpeg-devel@ffmpeg.org> (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Apr 2025 17:02:57 -0700 (PDT) Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.400.131.1.6\)) Date: Tue, 15 Apr 2025 09:02:45 +0900 References: <20250411222719.2779176-1-michael@niedermayer.cc> <20250411222719.2779176-2-michael@niedermayer.cc> <DM8P223MB0365A90C10FBC9EF6512F9E1BAB12@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM> <20250414231950.GN4991@pb2> <DM8P223MB0365928500AB53069869DC99BAB32@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM> To: ffmpeg-devel <ffmpeg-devel@ffmpeg.org> In-Reply-To: <DM8P223MB0365928500AB53069869DC99BAB32@DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM> Message-Id: <3D7D11CA-0A71-41DB-BD95-3EF31E990BCF@rcombs.me> X-Mailer: Apple Mail (2.3826.400.131.1.6) X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/id3v2: Check that decode_str() did advance X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches <ffmpeg-devel.ffmpeg.org> List-Unsubscribe: <https://ffmpeg.org/mailman/options/ffmpeg-devel>, <mailto:ffmpeg-devel-request@ffmpeg.org?subject=unsubscribe> List-Archive: <https://ffmpeg.org/pipermail/ffmpeg-devel> List-Post: <mailto:ffmpeg-devel@ffmpeg.org> List-Help: <mailto:ffmpeg-devel-request@ffmpeg.org?subject=help> List-Subscribe: <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>, <mailto:ffmpeg-devel-request@ffmpeg.org?subject=subscribe> From: Ridley Combs via ffmpeg-devel <ffmpeg-devel@ffmpeg.org> Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> Cc: Ridley Combs <rcombs@rcombs.me> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org> Archived-At: <https://master.gitmailbox.com/ffmpegdev/3D7D11CA-0A71-41DB-BD95-3EF31E990BCF@rcombs.me/> List-Archive: <https://master.gitmailbox.com/ffmpegdev/> List-Post: <mailto:ffmpegdev@gitmailbox.com> > 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".