| Message ID | 20201109230456.11188-5-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | f26b5acfc03375c96e593197a8145ada1b63b1fd |
| Headers | show |
| Series | [FFmpeg-devel,1/7] avformat/sbgdec: Check that end is not before start | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
Quoting Michael Niedermayer (2020-11-10 00:04:54) > Fixes: OOM > Fixes: 26934/clusterfuzz-testcase-minimized-ffmpeg_dem_W64_fuzzer-5996784213819392 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/wavdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c > index a81f2c7a67..6e5f4ccc12 100644 > --- a/libavformat/wavdec.c > +++ b/libavformat/wavdec.c > @@ -920,7 +920,7 @@ static int w64_read_header(AVFormatContext *s) > if (chunk_size == UINT32_MAX || (filesize >= 0 && chunk_size > filesize)) > return AVERROR_INVALIDDATA; > > - value = av_mallocz(chunk_size + 1); > + value = av_malloc(chunk_size + 1); This looks highly suspicious as a fix for anything other than performance.
On Sat, Nov 14, 2020 at 11:12:15AM +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2020-11-10 00:04:54) > > Fixes: OOM > > Fixes: 26934/clusterfuzz-testcase-minimized-ffmpeg_dem_W64_fuzzer-5996784213819392 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/wavdec.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c > > index a81f2c7a67..6e5f4ccc12 100644 > > --- a/libavformat/wavdec.c > > +++ b/libavformat/wavdec.c > > @@ -920,7 +920,7 @@ static int w64_read_header(AVFormatContext *s) > > if (chunk_size == UINT32_MAX || (filesize >= 0 && chunk_size > filesize)) > > return AVERROR_INVALIDDATA; > > > > - value = av_mallocz(chunk_size + 1); > > + value = av_malloc(chunk_size + 1); > > This looks highly suspicious as a fix for anything other than > performance. if iam not mistaken: The allocation doesnzt trigger OOM as no physical memory is allocated but once it is written to "z" it does and then OOMs if OTOH its written too while data is read from somewhere then a EOF ends writing and no OOM would happen thx [...]
Quoting Michael Niedermayer (2020-11-16 01:05:07) > On Sat, Nov 14, 2020 at 11:12:15AM +0100, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2020-11-10 00:04:54) > > > Fixes: OOM > > > Fixes: 26934/clusterfuzz-testcase-minimized-ffmpeg_dem_W64_fuzzer-5996784213819392 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/wavdec.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c > > > index a81f2c7a67..6e5f4ccc12 100644 > > > --- a/libavformat/wavdec.c > > > +++ b/libavformat/wavdec.c > > > @@ -920,7 +920,7 @@ static int w64_read_header(AVFormatContext *s) > > > if (chunk_size == UINT32_MAX || (filesize >= 0 && chunk_size > filesize)) > > > return AVERROR_INVALIDDATA; > > > > > > - value = av_mallocz(chunk_size + 1); > > > + value = av_malloc(chunk_size + 1); > > > > This looks highly suspicious as a fix for anything other than > > performance. > > if iam not mistaken: > The allocation doesnzt trigger OOM as no physical memory is allocated > but once it is written to "z" it does and then OOMs > if OTOH its written too while data is read from somewhere then a > EOF ends writing and no OOM would happen That is highly dependent on your OS and its configuration (e.g. you can disable overcommit on Linux). Also, there seems to be a check right above that fails if chunk_size is larger than the file size.
On Mon, Nov 16, 2020 at 07:07:21AM +0100, Anton Khirnov wrote: > Quoting Michael Niedermayer (2020-11-16 01:05:07) > > On Sat, Nov 14, 2020 at 11:12:15AM +0100, Anton Khirnov wrote: > > > Quoting Michael Niedermayer (2020-11-10 00:04:54) > > > > Fixes: OOM > > > > Fixes: 26934/clusterfuzz-testcase-minimized-ffmpeg_dem_W64_fuzzer-5996784213819392 > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/wavdec.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c > > > > index a81f2c7a67..6e5f4ccc12 100644 > > > > --- a/libavformat/wavdec.c > > > > +++ b/libavformat/wavdec.c > > > > @@ -920,7 +920,7 @@ static int w64_read_header(AVFormatContext *s) > > > > if (chunk_size == UINT32_MAX || (filesize >= 0 && chunk_size > filesize)) > > > > return AVERROR_INVALIDDATA; > > > > > > > > - value = av_mallocz(chunk_size + 1); > > > > + value = av_malloc(chunk_size + 1); > > > > > > This looks highly suspicious as a fix for anything other than > > > performance. > > > > if iam not mistaken: > > The allocation doesnzt trigger OOM as no physical memory is allocated > > but once it is written to "z" it does and then OOMs > > if OTOH its written too while data is read from somewhere then a > > EOF ends writing and no OOM would happen > > That is highly dependent on your OS and its configuration (e.g. you can > disable overcommit on Linux). yes thats correct If the OS supports this then a OOM can be avoided, otherwise it will OOM Avoiding the OOM in the remaining cases seems too messy to me. OTOH this here is a rather simple change ... > > Also, there seems to be a check right above that fails if chunk_size is > larger than the file size. The file size is not always known thx [...]
Quoting Michael Niedermayer (2020-11-16 22:28:48) > On Mon, Nov 16, 2020 at 07:07:21AM +0100, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2020-11-16 01:05:07) > > > On Sat, Nov 14, 2020 at 11:12:15AM +0100, Anton Khirnov wrote: > > > > Quoting Michael Niedermayer (2020-11-10 00:04:54) > > > > > Fixes: OOM > > > > > Fixes: 26934/clusterfuzz-testcase-minimized-ffmpeg_dem_W64_fuzzer-5996784213819392 > > > > > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > > --- > > > > > libavformat/wavdec.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c > > > > > index a81f2c7a67..6e5f4ccc12 100644 > > > > > --- a/libavformat/wavdec.c > > > > > +++ b/libavformat/wavdec.c > > > > > @@ -920,7 +920,7 @@ static int w64_read_header(AVFormatContext *s) > > > > > if (chunk_size == UINT32_MAX || (filesize >= 0 && chunk_size > filesize)) > > > > > return AVERROR_INVALIDDATA; > > > > > > > > > > - value = av_mallocz(chunk_size + 1); > > > > > + value = av_malloc(chunk_size + 1); > > > > > > > > This looks highly suspicious as a fix for anything other than > > > > performance. > > > > > > if iam not mistaken: > > > The allocation doesnzt trigger OOM as no physical memory is allocated > > > but once it is written to "z" it does and then OOMs > > > if OTOH its written too while data is read from somewhere then a > > > EOF ends writing and no OOM would happen > > > > That is highly dependent on your OS and its configuration (e.g. you can > > disable overcommit on Linux). > > yes thats correct > If the OS supports this then a OOM can be avoided, otherwise it will OOM > Avoiding the OOM in the remaining cases seems too messy to me. OTOH > this here is a rather simple change ... I am okay with the change, I object to it being committed as a (security) fix, since security fixes for platform-agnostic issues should not be so highly platform-dependent. Feel free to push if the commit message stops claiming it fixes OOM.
On Mon, Nov 16, 2020 at 01:05:07AM +0100, Michael Niedermayer wrote: > On Sat, Nov 14, 2020 at 11:12:15AM +0100, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2020-11-10 00:04:54) > > > Fixes: OOM > > > Fixes: 26934/clusterfuzz-testcase-minimized-ffmpeg_dem_W64_fuzzer-5996784213819392 > > > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/wavdec.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c > > > index a81f2c7a67..6e5f4ccc12 100644 > > > --- a/libavformat/wavdec.c > > > +++ b/libavformat/wavdec.c > > > @@ -920,7 +920,7 @@ static int w64_read_header(AVFormatContext *s) > > > if (chunk_size == UINT32_MAX || (filesize >= 0 && chunk_size > filesize)) > > > return AVERROR_INVALIDDATA; > > > > > > - value = av_mallocz(chunk_size + 1); > > > + value = av_malloc(chunk_size + 1); > > > > This looks highly suspicious as a fix for anything other than > > performance. > > if iam not mistaken: > The allocation doesnzt trigger OOM as no physical memory is allocated > but once it is written to "z" it does and then OOMs > if OTOH its written too while data is read from somewhere then a > EOF ends writing and no OOM would happen will apply [...]
diff --git a/libavformat/wavdec.c b/libavformat/wavdec.c index a81f2c7a67..6e5f4ccc12 100644 --- a/libavformat/wavdec.c +++ b/libavformat/wavdec.c @@ -920,7 +920,7 @@ static int w64_read_header(AVFormatContext *s) if (chunk_size == UINT32_MAX || (filesize >= 0 && chunk_size > filesize)) return AVERROR_INVALIDDATA; - value = av_mallocz(chunk_size + 1); + value = av_malloc(chunk_size + 1); if (!value) return AVERROR(ENOMEM);
Fixes: OOM Fixes: 26934/clusterfuzz-testcase-minimized-ffmpeg_dem_W64_fuzzer-5996784213819392 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/wavdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)