diff mbox series

[FFmpeg-devel,1/3] avformat/matroskadec: Reset state also on failure in matroska_reset_status()

Message ID 20210624205724.18711-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/3] avformat/matroskadec: Reset state also on failure in matroska_reset_status()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Michael Niedermayer June 24, 2021, 8:57 p.m. UTC
The calling code does not handle failures and will fail with assertion failures later.
Seeking can always fail even when the position was previously read.

Fixes: Assertion failure
Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/matroskadec.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Nicolas George June 24, 2021, 9:33 p.m. UTC | #1
Michael Niedermayer (12021-06-24):
> Fixes: signed integer overflow: 2788626175500000000 + 7118941284000000000 cannot be represented in type 'long'
> Fixes: 35215/clusterfuzz-testcase-minimized-ffmpeg_dem_SBG_fuzzer-6123272247836672
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/sbgdec.c | 3 +++
>  1 file changed, 3 insertions(+)

Ok.

Regards,
Andreas Rheinhardt July 2, 2021, 4:17 p.m. UTC | #2
Michael Niedermayer:
> The calling code does not handle failures and will fail with assertion failures later.
> Seeking can always fail even when the position was previously read.
> 
> Fixes: Assertion failure
> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/matroskadec.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 356a02339c..a0e6e0cf8b 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s);
>  static int matroska_reset_status(MatroskaDemuxContext *matroska,
>                                   uint32_t id, int64_t position)
>  {
> +    int64_t err = 0;
>      if (position >= 0) {
> -        int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
> -        if (err < 0)
> -            return err;
> -    }
> +        err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
> +        if (err > 0)
> +            err = 0;
> +    } else
> +        position = avio_tell(matroska->ctx->pb);
>  
>      matroska->current_id    = id;
>      matroska->num_levels    = 1;
>      matroska->unknown_count = 0;
> -    matroska->resync_pos = avio_tell(matroska->ctx->pb);
> +    matroska->resync_pos    = position;
>      if (id)
>          matroska->resync_pos -= (av_log2(id) + 7) / 8;
>  
> -    return 0;
> +    return err;

The changes here will make the demuxer update its internal state as if
it had seeked to its target level-1-element, even though it didn't. Is
this really good?

>  }
>  
>  static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
> @@ -1873,6 +1875,7 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
>      uint32_t saved_id  = matroska->current_id;
>      int64_t before_pos = avio_tell(matroska->ctx->pb);
>      int ret = 0;
> +    int ret2;
>  
>      /* seek */
>      if (avio_seek(matroska->ctx->pb, pos, SEEK_SET) == pos) {
> @@ -1897,7 +1900,9 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
>      }
>      /* Seek back - notice that in all instances where this is used
>       * it is safe to set the level to 1. */
> -    matroska_reset_status(matroska, saved_id, before_pos);
> +    ret2 = matroska_reset_status(matroska, saved_id, before_pos);
> +    if (ret >= 0)
> +        ret = ret2;
>  
>      return ret;
>  }
>
Michael Niedermayer July 3, 2021, 4:34 p.m. UTC | #3
On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > The calling code does not handle failures and will fail with assertion failures later.
> > Seeking can always fail even when the position was previously read.
> > 
> > Fixes: Assertion failure
> > Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/matroskadec.c | 19 ++++++++++++-------
> >  1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > index 356a02339c..a0e6e0cf8b 100644
> > --- a/libavformat/matroskadec.c
> > +++ b/libavformat/matroskadec.c
> > @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s);
> >  static int matroska_reset_status(MatroskaDemuxContext *matroska,
> >                                   uint32_t id, int64_t position)
> >  {
> > +    int64_t err = 0;
> >      if (position >= 0) {
> > -        int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
> > -        if (err < 0)
> > -            return err;
> > -    }
> > +        err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
> > +        if (err > 0)
> > +            err = 0;
> > +    } else
> > +        position = avio_tell(matroska->ctx->pb);
> >  
> >      matroska->current_id    = id;
> >      matroska->num_levels    = 1;
> >      matroska->unknown_count = 0;
> > -    matroska->resync_pos = avio_tell(matroska->ctx->pb);
> > +    matroska->resync_pos    = position;
> >      if (id)
> >          matroska->resync_pos -= (av_log2(id) + 7) / 8;
> >  
> > -    return 0;
> > +    return err;
> 
> The changes here will make the demuxer update its internal state as if
> it had seeked to its target level-1-element, even though it didn't. Is
> this really good?

I dont know.
Ive not seen this issue happen in reality just in a fuzzer
environment.

[...]
Andreas Rheinhardt July 19, 2021, 9:12 p.m. UTC | #4
Michael Niedermayer:
> On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> The calling code does not handle failures and will fail with assertion failures later.
>>> Seeking can always fail even when the position was previously read.
>>>
>>> Fixes: Assertion failure
>>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavformat/matroskadec.c | 19 ++++++++++++-------
>>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>> index 356a02339c..a0e6e0cf8b 100644
>>> --- a/libavformat/matroskadec.c
>>> +++ b/libavformat/matroskadec.c
>>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s);
>>>  static int matroska_reset_status(MatroskaDemuxContext *matroska,
>>>                                   uint32_t id, int64_t position)
>>>  {
>>> +    int64_t err = 0;
>>>      if (position >= 0) {
>>> -        int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
>>> -        if (err < 0)
>>> -            return err;
>>> -    }
>>> +        err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
>>> +        if (err > 0)
>>> +            err = 0;
>>> +    } else
>>> +        position = avio_tell(matroska->ctx->pb);
>>>  
>>>      matroska->current_id    = id;
>>>      matroska->num_levels    = 1;
>>>      matroska->unknown_count = 0;
>>> -    matroska->resync_pos = avio_tell(matroska->ctx->pb);
>>> +    matroska->resync_pos    = position;
>>>      if (id)
>>>          matroska->resync_pos -= (av_log2(id) + 7) / 8;
>>>  
>>> -    return 0;
>>> +    return err;
>>
>> The changes here will make the demuxer update its internal state as if
>> it had seeked to its target level-1-element, even though it didn't. Is
>> this really good?
> 
> I dont know.
> Ive not seen this issue happen in reality just in a fuzzer
> environment.
> 

Can you send me this sample (with instructions how to reproduce it if
necessary)?

- Andreas
Michael Niedermayer Sept. 13, 2021, 8:55 p.m. UTC | #5
On Mon, Jul 19, 2021 at 11:12:11PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote:
> >> Michael Niedermayer:
> >>> The calling code does not handle failures and will fail with assertion failures later.
> >>> Seeking can always fail even when the position was previously read.
> >>>
> >>> Fixes: Assertion failure
> >>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>> ---
> >>>  libavformat/matroskadec.c | 19 ++++++++++++-------
> >>>  1 file changed, 12 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> >>> index 356a02339c..a0e6e0cf8b 100644
> >>> --- a/libavformat/matroskadec.c
> >>> +++ b/libavformat/matroskadec.c
> >>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s);
> >>>  static int matroska_reset_status(MatroskaDemuxContext *matroska,
> >>>                                   uint32_t id, int64_t position)
> >>>  {
> >>> +    int64_t err = 0;
> >>>      if (position >= 0) {
> >>> -        int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
> >>> -        if (err < 0)
> >>> -            return err;
> >>> -    }
> >>> +        err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
> >>> +        if (err > 0)
> >>> +            err = 0;
> >>> +    } else
> >>> +        position = avio_tell(matroska->ctx->pb);
> >>>  
> >>>      matroska->current_id    = id;
> >>>      matroska->num_levels    = 1;
> >>>      matroska->unknown_count = 0;
> >>> -    matroska->resync_pos = avio_tell(matroska->ctx->pb);
> >>> +    matroska->resync_pos    = position;
> >>>      if (id)
> >>>          matroska->resync_pos -= (av_log2(id) + 7) / 8;
> >>>  
> >>> -    return 0;
> >>> +    return err;
> >>
> >> The changes here will make the demuxer update its internal state as if
> >> it had seeked to its target level-1-element, even though it didn't. Is
> >> this really good?
> > 
> > I dont know.
> > Ive not seen this issue happen in reality just in a fuzzer
> > environment.
> > 
> 
> Can you send me this sample (with instructions how to reproduce it if
> necessary)?

This seems still "crashing" according to the tracker
has this been fixed ?

thx

[...]
Andreas Rheinhardt Sept. 13, 2021, 8:57 p.m. UTC | #6
Michael Niedermayer:
> On Mon, Jul 19, 2021 at 11:12:11PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote:
>>>> Michael Niedermayer:
>>>>> The calling code does not handle failures and will fail with assertion failures later.
>>>>> Seeking can always fail even when the position was previously read.
>>>>>
>>>>> Fixes: Assertion failure
>>>>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168
>>>>>
>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>  libavformat/matroskadec.c | 19 ++++++++++++-------
>>>>>  1 file changed, 12 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>>>> index 356a02339c..a0e6e0cf8b 100644
>>>>> --- a/libavformat/matroskadec.c
>>>>> +++ b/libavformat/matroskadec.c
>>>>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s);
>>>>>  static int matroska_reset_status(MatroskaDemuxContext *matroska,
>>>>>                                   uint32_t id, int64_t position)
>>>>>  {
>>>>> +    int64_t err = 0;
>>>>>      if (position >= 0) {
>>>>> -        int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
>>>>> -        if (err < 0)
>>>>> -            return err;
>>>>> -    }
>>>>> +        err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
>>>>> +        if (err > 0)
>>>>> +            err = 0;
>>>>> +    } else
>>>>> +        position = avio_tell(matroska->ctx->pb);
>>>>>  
>>>>>      matroska->current_id    = id;
>>>>>      matroska->num_levels    = 1;
>>>>>      matroska->unknown_count = 0;
>>>>> -    matroska->resync_pos = avio_tell(matroska->ctx->pb);
>>>>> +    matroska->resync_pos    = position;
>>>>>      if (id)
>>>>>          matroska->resync_pos -= (av_log2(id) + 7) / 8;
>>>>>  
>>>>> -    return 0;
>>>>> +    return err;
>>>>
>>>> The changes here will make the demuxer update its internal state as if
>>>> it had seeked to its target level-1-element, even though it didn't. Is
>>>> this really good?
>>>
>>> I dont know.
>>> Ive not seen this issue happen in reality just in a fuzzer
>>> environment.
>>>
>>
>> Can you send me this sample (with instructions how to reproduce it if
>> necessary)?
> 
> This seems still "crashing" according to the tracker
> has this been fixed ?
> 

No, I have not found time to look at it. Proceed as you wish.

- Andreas
Michael Niedermayer Sept. 14, 2021, 4:13 p.m. UTC | #7
On Mon, Sep 13, 2021 at 10:57:41PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Mon, Jul 19, 2021 at 11:12:11PM +0200, Andreas Rheinhardt wrote:
> >> Michael Niedermayer:
> >>> On Fri, Jul 02, 2021 at 06:17:58PM +0200, Andreas Rheinhardt wrote:
> >>>> Michael Niedermayer:
> >>>>> The calling code does not handle failures and will fail with assertion failures later.
> >>>>> Seeking can always fail even when the position was previously read.
> >>>>>
> >>>>> Fixes: Assertion failure
> >>>>> Fixes: 35253/clusterfuzz-testcase-minimized-ffmpeg_dem_MATROSKA_fuzzer-4693059982983168
> >>>>>
> >>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>>> ---
> >>>>>  libavformat/matroskadec.c | 19 ++++++++++++-------
> >>>>>  1 file changed, 12 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> >>>>> index 356a02339c..a0e6e0cf8b 100644
> >>>>> --- a/libavformat/matroskadec.c
> >>>>> +++ b/libavformat/matroskadec.c
> >>>>> @@ -804,20 +804,22 @@ static int matroska_read_close(AVFormatContext *s);
> >>>>>  static int matroska_reset_status(MatroskaDemuxContext *matroska,
> >>>>>                                   uint32_t id, int64_t position)
> >>>>>  {
> >>>>> +    int64_t err = 0;
> >>>>>      if (position >= 0) {
> >>>>> -        int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
> >>>>> -        if (err < 0)
> >>>>> -            return err;
> >>>>> -    }
> >>>>> +        err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
> >>>>> +        if (err > 0)
> >>>>> +            err = 0;
> >>>>> +    } else
> >>>>> +        position = avio_tell(matroska->ctx->pb);
> >>>>>  
> >>>>>      matroska->current_id    = id;
> >>>>>      matroska->num_levels    = 1;
> >>>>>      matroska->unknown_count = 0;
> >>>>> -    matroska->resync_pos = avio_tell(matroska->ctx->pb);
> >>>>> +    matroska->resync_pos    = position;
> >>>>>      if (id)
> >>>>>          matroska->resync_pos -= (av_log2(id) + 7) / 8;
> >>>>>  
> >>>>> -    return 0;
> >>>>> +    return err;
> >>>>
> >>>> The changes here will make the demuxer update its internal state as if
> >>>> it had seeked to its target level-1-element, even though it didn't. Is
> >>>> this really good?
> >>>
> >>> I dont know.
> >>> Ive not seen this issue happen in reality just in a fuzzer
> >>> environment.
> >>>
> >>
> >> Can you send me this sample (with instructions how to reproduce it if
> >> necessary)?
> > 
> > This seems still "crashing" according to the tracker
> > has this been fixed ?
> > 
> 
> No, I have not found time to look at it. Proceed as you wish.

well, i will look at other issues first then, maybe someone else
will fix this ...

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 356a02339c..a0e6e0cf8b 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -804,20 +804,22 @@  static int matroska_read_close(AVFormatContext *s);
 static int matroska_reset_status(MatroskaDemuxContext *matroska,
                                  uint32_t id, int64_t position)
 {
+    int64_t err = 0;
     if (position >= 0) {
-        int64_t err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
-        if (err < 0)
-            return err;
-    }
+        err = avio_seek(matroska->ctx->pb, position, SEEK_SET);
+        if (err > 0)
+            err = 0;
+    } else
+        position = avio_tell(matroska->ctx->pb);
 
     matroska->current_id    = id;
     matroska->num_levels    = 1;
     matroska->unknown_count = 0;
-    matroska->resync_pos = avio_tell(matroska->ctx->pb);
+    matroska->resync_pos    = position;
     if (id)
         matroska->resync_pos -= (av_log2(id) + 7) / 8;
 
-    return 0;
+    return err;
 }
 
 static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
@@ -1873,6 +1875,7 @@  static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
     uint32_t saved_id  = matroska->current_id;
     int64_t before_pos = avio_tell(matroska->ctx->pb);
     int ret = 0;
+    int ret2;
 
     /* seek */
     if (avio_seek(matroska->ctx->pb, pos, SEEK_SET) == pos) {
@@ -1897,7 +1900,9 @@  static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
     }
     /* Seek back - notice that in all instances where this is used
      * it is safe to set the level to 1. */
-    matroska_reset_status(matroska, saved_id, before_pos);
+    ret2 = matroska_reset_status(matroska, saved_id, before_pos);
+    if (ret >= 0)
+        ret = ret2;
 
     return ret;
 }