Re: [FFmpeg-devel] [PATCH 2/2] avformat/demux: Count EAGAIN as 100 bytes in relation to read limit in avformat_find_stream_info()

From: Michael Niedermayer <michael@niedermayer.cc>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 2/2] avformat/demux: Count EAGAIN as 100 bytes in relation to read limit in avformat_find_stream_info()
Date: Sat, 18 Jun 2022 18:18:14 +0200
Message-ID: <20220618161814.GY396728@pb2> (raw)
In-Reply-To: <7e97fc0-25ab-7ba8-ece8-a9e75f60a120@passwd.hu>

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

On Fri, Jun 17, 2022 at 10:15:39PM +0200, Marton Balint wrote:
> 
> 
> On Tue, 8 Feb 2022, Michael Niedermayer wrote:
> 
> > Fixes: Timeout
> > 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@niedermayer.cc>
> > ---
> > libavformat/demux.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/demux.c b/libavformat/demux.c
> > index dd42d32710..1acba0c608 100644
> > --- a/libavformat/demux.c
> > +++ b/libavformat/demux.c
> > @@ -2590,8 +2590,10 @@ int avformat_find_stream_info(AVFormatContext *ic, AVDictionary **options)
> >         /* NOTE: A new stream can be added there if no header in file
> >          * (AVFMTCTX_NOHEADER). */
> >         ret = read_frame_internal(ic, pkt1);
> > -        if (ret == AVERROR(EAGAIN))
> > +        if (ret == AVERROR(EAGAIN)) {
> > +            read_size += 100;
> 
> Sorry, same here, very hackish. I especially dislike that you use read_size
> for a limit because that intereferes with probesize and silently ignores the
> problem.
> 
> Can't we fix the underlying issue? If not, then counting the number of
> EAGAINs and returning a hard failure if that becomes more than e.g. 10000
> would be a lot more acceptable to me. E.g.
> 

> if (nb_egains > 10000) {
>    av_log(NULL, AV_LOG_ERROR, "read_frame stuck in an EAGAIN loop, this should not happen\n);
>    return AVERROR_BUG;
> }

I dont know if AVERROR_BUG is the correct return code.
Also with this the question is do you reset nb_egains after a non zero output
or not ?
if you do then you could have a case where there are 9999 empty packets 
followed by 1 byte followed by 9999 empty packets ...
if you do not then a file with 10001 1 byte packets where after each theres 1
empty one would fail
Surely these are constructed hypothetical cases but the idea of adding it
with the number of reads is so that the real "cost" is bound
maybe a sepertate nb_egains tahts non reseting could be done but with a larger
limit

I dont really have an opinion on this, i dont like mixing it in read_size
either but then i also dont like any other solution. needing an extra
threshold and potentially user setable parameter all suck too

so if you like this nb_egains code better, sure go ahead and replace it

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact

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