[FFmpeg-devel] [PATCH 3/4] avformat/mlvdec: fail reading a packet with 0 streams

[James Almer]

On 5/31/2020 6:48 PM, Michael Niedermayer wrote:
> On Sun, May 31, 2020 at 10:58:16AM -0300, James Almer wrote:
>> On 5/31/2020 10:50 AM, Michael Niedermayer wrote:
>>> Fixes: NULL pointer dereference
>>> Fixes: 22604/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-5667739074297856.fuzz
>>>
>>> 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/mlvdec.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/mlvdec.c b/libavformat/mlvdec.c
>>> index dae13cae53..03aed71024 100644
>>> --- a/libavformat/mlvdec.c
>>> +++ b/libavformat/mlvdec.c
>>> @@ -393,10 +393,14 @@ static int read_packet(AVFormatContext *avctx, AVPacket *pkt)
>>>  {
>>>      MlvContext *mlv = avctx->priv_data;
>>>      AVIOContext *pb;
>>> -    AVStream *st = avctx->streams[mlv->stream_index];
>>> +    AVStream *st;
>>>      int index, ret;
>>>      unsigned int size, space;
>>>  
>>> +    if (!avctx->nb_streams)
>>> +        return AVERROR_EOF;
>>
>> Shouldn't you abort during read_header() instead if no streams are ever
>> allocated?
> 
> read_header() should fail if the haeder is invalid.
> is there something which says that "no streams" is invalid ?
> 
> As it is the read_header() code contains a if()  which only make
> a difference for a "no stream" case. Which gave me the feeling that
> it wasnt intended to fail in that case

The checks i'm seeing are all considering video only, audio only, or
both, like the very last check in the function where there's no last
resort else that would cover a no streams case.

> but i can add the check for no streams in there where theres already
> a check for that in read_header ...

Unless there's something in a file that could be signaled without
streams (All the metadata strings in scan_file() look like they
definitely apply to some video or audio stream), such a file seems odd
to me, and if the end result will be just signaling EOF as soon as the
first attempt at fetching a packet, then might as well just abort in
read_header() and save the library caller further processing.

Maybe Peter Ross can comment. But in any case, no strong feelings about
it, so apply whatever you think is best.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>