| Message ID | 20220208202353.19554-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | b0cac7082d8a3ff2d4f039af01b45c48bb578de7 |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/demux: Make read_frame_internal() return AVERREOR(EAGAIN) on stuck empty input parser | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
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; } Regards, Marton
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 [...]
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; continue; + } if (ret < 0) { /* EOF or error*/
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(-)