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

[Andreas Rheinhardt]

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?

>  }
>  
>  static int matroska_resync(MatroskaDemuxContext *matroska, int64_t last_pos)
> @@ -1873,6 +1875,7 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
>      uint32_t saved_id  = matroska->current_id;
>      int64_t before_pos = avio_tell(matroska->ctx->pb);
>      int ret = 0;
> +    int ret2;
>  
>      /* seek */
>      if (avio_seek(matroska->ctx->pb, pos, SEEK_SET) == pos) {
> @@ -1897,7 +1900,9 @@ static int matroska_parse_seekhead_entry(MatroskaDemuxContext *matroska,
>      }
>      /* Seek back - notice that in all instances where this is used
>       * it is safe to set the level to 1. */
> -    matroska_reset_status(matroska, saved_id, before_pos);
> +    ret2 = matroska_reset_status(matroska, saved_id, before_pos);
> +    if (ret >= 0)
> +        ret = ret2;
>  
>      return ret;
>  }
>