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

[Andreas Rheinhardt]

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.

- Andreas