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

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA
-------------- 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/20210703/bdda543b/attachment.sig>