diff mbox series

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

Message ID 20200531135020.5675-3-michael@niedermayer.cc
State Accepted
Commit 5bd5c3108786bf69f108c55c375f1956f67ca7a4
Headers show
Series [FFmpeg-devel,1/4] avcodec/jpeg2000dec: clear pointer which become stale in get_ppt()
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer May 31, 2020, 1:50 p.m. UTC
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@niedermayer.cc>
---
 libavformat/mlvdec.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

James Almer May 31, 2020, 1:58 p.m. UTC | #1
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@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?

> +
> +    st = avctx->streams[mlv->stream_index];
>      if (mlv->pts >= st->duration)
>          return AVERROR_EOF;
>  
>
Michael Niedermayer May 31, 2020, 9:48 p.m. UTC | #2
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@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
but i can add the check for no streams in there where theres already
a check for that in read_header ...

thx

[...]
James Almer May 31, 2020, 10:07 p.m. UTC | #3
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@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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer June 1, 2020, 9:51 p.m. UTC | #4
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@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 Niedermayer June 7, 2020, 8:29 p.m. UTC | #5
On Mon, Jun 01, 2020 at 11:51:41PM +0200, Michael Niedermayer wrote:
> 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@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

just remembered, so will apply, feel free to replace by another solution
or if peter comes across this in  the future. iam happy to replace it
but trying to move forward, leaving it open is worst

thx

[...]
diff mbox series

Patch

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;
+
+    st = avctx->streams[mlv->stream_index];
     if (mlv->pts >= st->duration)
         return AVERROR_EOF;