| Message ID | 20220323093033.24577-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/aaxdec: Check for overlaping segments | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
Michael Niedermayer: > Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int' > Fixes: 45891/clusterfuzz-testcase-minimized-ffmpeg_dem_AIFF_fuzzer-6159183893889024 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/aiffdec.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c > index 3634bb4960..c2b8e0dede 100644 > --- a/libavformat/aiffdec.c > +++ b/libavformat/aiffdec.c > @@ -72,7 +72,12 @@ static int get_tag(AVIOContext *pb, uint32_t * tag) > /* Metadata string read */ > static void get_meta(AVFormatContext *s, const char *key, int size) > { > - uint8_t *str = av_malloc(size+1); > + uint8_t *str; > + > + if (size == INT_MAX) > + return; > + > + str = av_malloc(size+1); > > if (str) { > int res = avio_read(s->pb, str, size); If a size of INT_MAX is legal, then you can just use "size + 1U" to avoid the wraparound. (The allocation will then fail with the default value of max_alloc_size and in this case the avio_skip() will be executed, so that we don't lose sync (your patch does that).) Looking at get_tag() shows that the size actually comes from a uint32_t which gets truncated to INT_MAX if it is not representable in an int. I don't know whether the specs mandate this behaviour (probably not, so one loses sync). If one wanted to impose an arbitrary limit, I will use something much smaller than INT_MAX. - Andreas
On Wed, Mar 23, 2022 at 12:07:25PM +0100, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int' > > Fixes: 45891/clusterfuzz-testcase-minimized-ffmpeg_dem_AIFF_fuzzer-6159183893889024 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/aiffdec.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c > > index 3634bb4960..c2b8e0dede 100644 > > --- a/libavformat/aiffdec.c > > +++ b/libavformat/aiffdec.c > > @@ -72,7 +72,12 @@ static int get_tag(AVIOContext *pb, uint32_t * tag) > > /* Metadata string read */ > > static void get_meta(AVFormatContext *s, const char *key, int size) > > { > > - uint8_t *str = av_malloc(size+1); > > + uint8_t *str; > > + > > + if (size == INT_MAX) > > + return; > > + > > + str = av_malloc(size+1); > > > > if (str) { > > int res = avio_read(s->pb, str, size); > > If a size of INT_MAX is legal, then you can just use "size + 1U" to > avoid the wraparound. (The allocation will then fail with the default > value of max_alloc_size and in this case the avio_skip() will be > executed, so that we don't lose sync (your patch does that).) > Looking at get_tag() shows that the size actually comes from a uint32_t > which gets truncated to INT_MAX if it is not representable in an int. I > don't know whether the specs mandate this behaviour (probably not, so > one loses sync). If one wanted to impose an arbitrary limit, I will use > something much smaller than INT_MAX. The goal of my patch was to fix the undefined behavior. You are very correct that aiffdec has other bugs. ill send a more complete set of fixes thx [...]
diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c index 3634bb4960..c2b8e0dede 100644 --- a/libavformat/aiffdec.c +++ b/libavformat/aiffdec.c @@ -72,7 +72,12 @@ static int get_tag(AVIOContext *pb, uint32_t * tag) /* Metadata string read */ static void get_meta(AVFormatContext *s, const char *key, int size) { - uint8_t *str = av_malloc(size+1); + uint8_t *str; + + if (size == INT_MAX) + return; + + str = av_malloc(size+1); if (str) { int res = avio_read(s->pb, str, size);
Fixes: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int' Fixes: 45891/clusterfuzz-testcase-minimized-ffmpeg_dem_AIFF_fuzzer-6159183893889024 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/aiffdec.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)