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

[Michael Niedermayer]

On Mon, Dec 06, 2021 at 07:01:24PM +0100, Andreas Rheinhardt wrote:
> 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.

0 was used as no limit in other cases, i did not check nor remembered that
AVFormatContext.max_streams doesnt have 0 as a documented exception
i will remove the 0 check


> 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

will add that


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

Most of these files have 1 stream and an id of 0. I have found 1 file 
which has 7 streams, with id 0,1,2,3,4,5,6 in that order IIRC

so it feels a bit overkillish to remap this around, but i surely see
your argument behind this

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/20211206/2cc3fd10/attachment.sig>