[FFmpeg-devel] [PATCH 1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser

[Michael Niedermayer]

On Fri, Jun 17, 2022 at 09:53:13PM +0200, Marton Balint wrote:
> 
> 
> On Tue, 8 Feb 2022, Michael Niedermayer wrote:
> 
> > Fixes: read_frame_internal() which does not return even though both demuxer and parser do return
> > Fixes: 43717/clusterfuzz-testcase-minimized-ffmpeg_IO_DEMUXER_fuzzer-5206008287330304
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > ---
> > libavformat/demux.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> > 
> > diff --git a/libavformat/demux.c b/libavformat/demux.c
> > index ec34b65288..dd42d32710 100644
> > --- a/libavformat/demux.c
> > +++ b/libavformat/demux.c
> > @@ -1222,11 +1222,15 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
> >     FFFormatContext *const si = ffformatcontext(s);
> >     int ret, got_packet = 0;
> >     AVDictionary *metadata = NULL;
> > +    int empty = 0;
> > 
> >     while (!got_packet && !si->parse_queue.head) {
> >         AVStream *st;
> >         FFStream *sti;
> > 
> > +        if (empty > 1)
> > +            return AVERROR(EAGAIN);
> > +
> >         /* read next packet */
> >         ret = ff_read_packet(s, pkt);
> >         if (ret < 0) {
> > @@ -1317,6 +1321,8 @@ static int read_frame_internal(AVFormatContext *s, AVPacket *pkt)
> >             }
> >             got_packet = 1;
> >         } else if (st->discard < AVDISCARD_ALL) {
> > +            if (pkt->size == 0)
> > +                empty ++;
> >             if ((ret = parse_packet(s, pkt, pkt->stream_index, 0)) < 0)
> >                 return ret;
> >             st->codecpar->sample_rate = sti->avctx->sample_rate;
> > -- 
> > 2.17.1
> 
> Sorry, just noticed this patchset, and it is very hackish.

yes thats why i waited so many month before i applied it
some patchsets i forget but this i kept pushing away

> 
> For starters the meaning of AVERROR(EAGAIN) for
> av_read_frame()/read_frame_internal() is not very well defined. Should the
> user retry immediately? Should the user sleep() sometime and the retry? Can
> the user expect that a loop of av_read_frame() will eventually return
> something different than AVERROR(EAGAIN)?

In the context of this problem the idea is to give the user an opertunity
to do something else in a single threaded environment or error out if
its taking too long not produingh anything


> 
> I am not sure I understand the original issue entirely, but it looks that
> instead of fixing the component which returns infinite amount of zero sized
> packets you implemented a check that makes the user get an EAGAIN() on the
> second zero-sized packet.

IIRC the problem was that the demuxer produced 0 sized packets and alot of them
for a file only a few hundread bytes large.
now the zero sized packets go into the parser which has no output because
theres no bytes input so no packet is ever produced by the parser and nothing
comes out of the read_frame_internal() code, its just stuck in a loop

From the point of view of the parser, the parser seems behaving correct
From the point of view of the demuxer, the demuxer seems behaving correct
if for example it produces a billion empty packets
but from the point of view of the user the code is stuck and hangs


> 
> And this helps the user how? Instead of a busy CPU loop in the library he
> either gets a busy CPU loop in its application or a non-busy CPU loop in its
> application (if he sleeps() on EAGAIN).

The advantage for the user is, she can cleanly error out instead of killing
the thread / process


> 
> Is there a file which causes this? Can't the underlying components be fixed
> instead?

i have 2 files it seems, i will mail them to you privatly
As said, its not clear if this is fundamentally any component misbehaving
maybe something does break some rule somewhere i dont know

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220618/3643d437/attachment.sig>