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

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Sat, 18 Jun 2022, Michael Niedermayer wrote:
> 
>> 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.
> 
> AFAIK a demuxer is not supposed to generate 0-sized packets. Or do you
> think otherwise?

Is it not possible to use zero-sized packets to clear the screen (i.e.
convey the end timestamp of subtitles transmitted earlier)? (The
subtitle formats without duration currently use packets with a size > 0
for that, but there is no real reason why this need to be so.)
Another reason for zero-sized packets were side-data only packets (maybe
not now, but it might be used in the future; after all, we are
constantly adding new side-data-types).

(This does not mean that I am against your proposed patch.)

- Andreas

PS: The fact that the parsing API is based upon buffers and not
AVPackets btw leads to buggy side-data handling: If a parser introduces
delay, then the side-data will not be attached to the correct packet,
but to the preceding one.