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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20210914/cbf88971/attachment.sig>