From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.ffmpeg.org (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTPS id 0660646A9D for ; Mon, 14 Jul 2025 22:28:13 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTP id 31AF668F087; Tue, 15 Jul 2025 01:28:10 +0300 (EEST) Received: from btbn.de (btbn.de [144.76.60.213]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTPS id 3B7CF68F07D for ; Tue, 15 Jul 2025 01:28:04 +0300 (EEST) Received: from [authenticated] by btbn.de (Postfix) with ESMTPSA id 567E027FD3764 for ; Tue, 15 Jul 2025 00:28:03 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rothenpieler.org; s=mail; t=1752532083; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YVYuR5ggzmM0Bg+H1RwTaB/Xys2Ajmw7imyS6WuWE3g=; b=KYFvVhhwZyPZw902pWs3PDg9kShIUjvNE0imvAl5vP4HLXpAaZmFLEkNbzqSKd4wyWVudq 9VvvzT587Cni0mwiS0mtVnKE/bMYvXg5cJ9/oAAfwxjOwFASs/x8rb6KvapxghUDqkHEkC vdzPTj6Ssv0RWtNGb2t7VZz5MUiJkuMrXUECyvzXQnAwjrwtaaXaDtZgHbl+hMYrHSIwIh AN3wwg51REk3YZItSsAecPf3cxOWw2K85irknl1fc33YnSujwKYyHDUiXv6fXo1oxuzOkA WYyTnC7yjiAwrj1d24kM3slhy4+YwRdA2V05jtbHC1KYFKy+KorcaoSTihXMbA== Message-ID: <067d6931-29bd-4be3-9e87-336383897e65@rothenpieler.org> Date: Tue, 15 Jul 2025 00:28:04 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: ffmpeg-devel@ffmpeg.org References: <20250713011030.1156550-1-michael@niedermayer.cc> <20250714192133.GL29660@pb2> <71ce56e6-5d83-41f7-9872-e81e0a342cf9@rothenpieler.org> <20250714220140.GP29660@pb2> Content-Language: en-US From: Timo Rothenpieler In-Reply-To: <20250714220140.GP29660@pb2> Subject: Re: [FFmpeg-devel] [PATCH 1/5] avformat/flvdec: Check for EOF in AudioPacketTypeMultichannelConfig X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On 7/15/2025 12:01 AM, Michael Niedermayer wrote: > On Mon, Jul 14, 2025 at 10:00:19PM +0200, Timo Rothenpieler wrote: >> On 7/14/2025 9:21 PM, Michael Niedermayer wrote: >>> On Sun, Jul 13, 2025 at 01:42:28PM +0200, Timo Rothenpieler wrote: >>>> On 7/13/2025 3:10 AM, Michael Niedermayer wrote: >>>>> Fixes: Infinite loop >>>>> Fixes: 427538726/clusterfuzz-testcase-minimized-ffmpeg_dem_FLV_fuzzer-6582567304495104 >>>>> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>> Signed-off-by: Michael Niedermayer >>>>> --- >>>>> libavformat/flvdec.c | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c >>>>> index ac681954cb7..a4fa0157512 100644 >>>>> --- a/libavformat/flvdec.c >>>>> +++ b/libavformat/flvdec.c >>>>> @@ -1715,6 +1715,9 @@ retry_duration: >>>>> av_log(s, AV_LOG_DEBUG, "Set channel data from MultiChannel info.\n"); >>>>> + if (avio_feof(s->pb)) >>>>> + return AVERROR_EOF; >>>>> + >>>>> goto next_track; >>>>> } >>>>> } else if (stream_type == FLV_STREAM_TYPE_VIDEO) { >>>> >>>> I don't think just returning from here is correct. >>>> The goto next_track right after it already checks for EOF. >>> >>>> I do not see how between here and the eof check there there'd be any way to >>>> infinite loop. >>> >>> avio_skip() with a negative value will reset the EOF flag >>> >>> >>>> >>>> It returns FFERROR_REDO there, which is important to drain queued up >>>> packages. >>> >>> I think the state becomes corrupted once it reads into EOF >>> that is the size accounting goes "oops" as the code keeps running >>> things that read and keeps accounting for these reads but in reality >>> nothing is read as its at EOF >>> and then it seeks back all these "not reads" and thats where it hits the >>> infinite loop as theres a mismatch what the code thinks it moved forward >>> and what it actually moved forward. >>> Thats how it looked to me at least, i have not verified every step of this >>> >>> ill mail you the testcase, then you can check if my analysis is right >>> and fix the code in a way that can recover queued packets in such truncated >>> packet at EOF case. >>> Also please make sure its not forgotten that whatever fix this gets is backported >> I'm unable to reproduce any infinite loops, even with the sample. >> But the code there definitely is sub-optimal, given the seek can go the >> wrong way, and even when going the right way can potentially reset the EOF >> flag. >> >> Proposed patch is attached. > >> flvdec.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> 681dde0e1e99d4e0cee0f4eec92eb3dc229a25d4 0001-avformat-flvdec-don-t-skip-backwards-or-at-EOF.patch >> From 7ff394e1ecab504a4cb0fda4bd0f25d88ee4f6fe Mon Sep 17 00:00:00 2001 >> From: Timo Rothenpieler >> Date: Mon, 14 Jul 2025 21:54:35 +0200 >> Subject: [PATCH] avformat/flvdec: don't skip backwards or at EOF >> >> --- >> libavformat/flvdec.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c >> index b90ed34b1c..de5e688822 100644 >> --- a/libavformat/flvdec.c >> +++ b/libavformat/flvdec.c >> @@ -1860,8 +1860,16 @@ retry_duration: >> next_track: >> if (track_size) { >> av_log(s, AV_LOG_WARNING, "Track size mismatch: %d!\n", track_size); >> - avio_skip(s->pb, track_size); >> - size -= track_size; >> + if (!avio_feof(s->pb)) { >> + if (track_size > 0) { >> + avio_skip(s->pb, track_size); >> + size -= track_size; >> + } else { >> + /* We have somehow read more than the track had to offer, leave and re-sync */ >> + ret = FFERROR_REDO; >> + goto leave; >> + } >> + } >> } > > i think this is not correct > > if a corrupted packet pushes you 1gb forward into EOF you must seek back > and by extension (if that logic is correct) we also can require a seek back > without EOF If a corrupt package makes this avio_skip skip gigabytes ahead, reading will just EOF anyway a few lines down (or continue and try to re-sync from there, if the file is big enough). The main problem here is that avio_skip, which just calls avio_seek internally, can reset the EOF state. So the main thing to guard against here is seeking backwards, since that can lead to an infinite loop if it seeks back just right and ends up at the same corrupted spot. So this change makes sure that if the file is already EOF, no seeking/skipping is done, and additionally, even if not EOF, it'll only ever skip forward, which would instantly lead to an EOF if far enough. If a backwards seek has been calculated, it'll now also immediately abort parsing, since that can only mean that garbage was read. So it leaves and either aborts or tries to re-sync. > I have not deeply analyzed the flv code today, so i may miss something > > thx > > [...] > > > _______________________________________________ > 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". _______________________________________________ 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".