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

[Michael Niedermayer]

On Tue, Sep 14, 2021 at 06:13:43PM +0200, Michael Niedermayer wrote:
> 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 ...

noone ? :(

if noone wants to look into this then ill apply the original patch

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.
-------------- 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/20211006/c2f11634/attachment.sig>