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

[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 ?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- 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/20210913/77417e0c/attachment.sig>