[FFmpeg-devel] [PATCH 4/7] avformat/4xm: Consider max_streams on reallocating tracks array

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: OOM
> Fixes: 41595/clusterfuzz-testcase-minimized-ffmpeg_dem_FOURXM_fuzzer-6355979363549184
> 
> 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/4xm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/4xm.c b/libavformat/4xm.c
> index f918b1fc572..5cbae7053d8 100644
> --- a/libavformat/4xm.c
> +++ b/libavformat/4xm.c
> @@ -137,7 +137,8 @@ static int parse_strk(AVFormatContext *s,
>          return AVERROR_INVALIDDATA;
>  
>      track = AV_RL32(buf + 8);
> -    if ((unsigned)track >= UINT_MAX / sizeof(AudioTrack) - 1) {
> +    if ((unsigned)track >= UINT_MAX / sizeof(AudioTrack) - 1 ||
> +        s->max_streams && track >= s->max_streams) {
>          av_log(s, AV_LOG_ERROR, "current_track too large\n");
>          return AVERROR_INVALIDDATA;
>      }
> 

Why do you treat s->max_streams == 0 as "no limit on the number of
streams"? Neither the documentation nor avformat_new_stream() treat it
as such.
I think a better way to fix this is to stop allocating based upon track
and rather allocate based upon the actual number of tracks seen (so
AudioTrack needs to have a track field, but the stream_index field could
be dropped).
Also notice that this demuxer currently doesn't check that the track ids
encountered are unique. As a result, there can be multiple AVStreams
with the same id, only the last of which will return packets.
Another way to limit allocations of this demuxer is to not read the
whole file header into memory.

- Andreas