diff mbox series

[FFmpeg-devel,5/7] avformat/wavdec: Avoid zeroing written to array

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
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Michael Niedermayer Nov. 9, 2020, 11:04 p.m. UTC
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(-)

Comments

Anton Khirnov Nov. 14, 2020, 10:12 a.m. UTC | #1
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.
Michael Niedermayer Nov. 16, 2020, 12:05 a.m. UTC | #2
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

[...]
Anton Khirnov Nov. 16, 2020, 6:07 a.m. UTC | #3
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.
Michael Niedermayer Nov. 16, 2020, 9:28 p.m. UTC | #4
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

[...]
Anton Khirnov Nov. 17, 2020, 9:51 a.m. UTC | #5
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.
Michael Niedermayer Jan. 2, 2021, 11:51 p.m. UTC | #6
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 mbox series

Patch

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