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: 4575 bytes --]

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

I have not deeply analyzed the flv code today, so i may miss something

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange

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