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

[Michael Niedermayer]

On Sun, May 31, 2020 at 07:07:11PM -0300, James Almer wrote:
> 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.

    if (vst && ast)
        A
    else if (vst)
        B
    else if (ast)
        C
        
if one didnt think of "no streams" this would be
    if (vst && ast)
        A
    else if (vst)
        B
    else
        C

if one wanted to reject "no streams" this would be

    if (vst && ast)
        A
    else if (vst)
        B
    else if (ast)
        C
    else
        FAIL
        
to me this looked like the intend was to support that

I have no preferrance between any of this, just wanted to not error
out on a path that seemed the author didnt want it to error ...


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

ok, ill wait for peter to reply then or in absence of a reply, next time
i remember ill apply something

thanks

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

Those who are best at talking, realize last or never when they are wrong.
-------------- 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/20200601/9d12f3ea/attachment.sig>