On Tue, Jul 15, 2025 at 12:28:04AM +0200, Timo Rothenpieler wrote: > 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 <mich...@niedermayer.cc> > > > > > > --- > > > > > > 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 <t...@rothenpieler.org> > > > 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).
i think re sync should happen from end of last good packet + minimum packet size or so not from random position Otherwise resync is not very robust thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
