| 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 | expand |
| 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 |
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
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 --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; }
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(-)