Re: [FFmpeg-devel] [PATCH 1/5] avformat/flvdec: Check for EOF in AudioPacketTypeMultichannelConfig

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 5405 bytes --]

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 <michael@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 <timo@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 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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