diff mbox series

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

Message ID 20211205211907.30010-4-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/7] avformat/vivo: Do not use the general expression evaluator for parsing a floating point value
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Michael Niedermayer Dec. 5, 2021, 9:19 p.m. UTC
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@niedermayer.cc>
---
 libavformat/4xm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt Dec. 6, 2021, 6:01 p.m. UTC | #1
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@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
Michael Niedermayer Dec. 6, 2021, 10:04 p.m. UTC | #2
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@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

[...]
diff mbox series

Patch

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;
     }