diff mbox series

[FFmpeg-devel,2/2] avformat/swfdec: Check outlen before allocation

Message ID 20210122140947.18479-2-michael@niedermayer.cc
State Accepted
Commit 1957095e808b80cbe607347a6f23207d6318a48d
Headers show
Series [FFmpeg-devel,1/2] avformat/mxfdec: Fix integer overflow in next position in mxf_read_local_tags()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Michael Niedermayer Jan. 22, 2021, 2:09 p.m. UTC
Fixes: Timeout (too long -> 241ms)
Fixes: 29083/clusterfuzz-testcase-minimized-ffmpeg_dem_SWF_fuzzer-6273684478230528

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/swfdec.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Anton Khirnov Jan. 23, 2021, 2:29 p.m. UTC | #1
Quoting Michael Niedermayer (2021-01-22 15:09:47)
> Fixes: Timeout (too long -> 241ms)
> Fixes: 29083/clusterfuzz-testcase-minimized-ffmpeg_dem_SWF_fuzzer-6273684478230528
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/swfdec.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavformat/swfdec.c b/libavformat/swfdec.c
> index 1463f0ad4d..aa4be88f91 100644
> --- a/libavformat/swfdec.c
> +++ b/libavformat/swfdec.c
> @@ -367,6 +367,9 @@ static int swf_read_packet(AVFormatContext *s, AVPacket *pkt)
>              ff_dlog(s, "bitmap: ch=%d fmt=%d %dx%d (linesize=%d) len=%d->%ld pal=%d\n",
>                      ch_id, bmp_fmt, width, height, linesize, len, out_len, colormapsize);
>  
> +            if (len * 17373LL < out_len)

Where does the magic number come from?
Michael Niedermayer Jan. 23, 2021, 10:34 p.m. UTC | #2
On Sat, Jan 23, 2021 at 03:29:38PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-01-22 15:09:47)
> > Fixes: Timeout (too long -> 241ms)
> > Fixes: 29083/clusterfuzz-testcase-minimized-ffmpeg_dem_SWF_fuzzer-6273684478230528
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/swfdec.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavformat/swfdec.c b/libavformat/swfdec.c
> > index 1463f0ad4d..aa4be88f91 100644
> > --- a/libavformat/swfdec.c
> > +++ b/libavformat/swfdec.c
> > @@ -367,6 +367,9 @@ static int swf_read_packet(AVFormatContext *s, AVPacket *pkt)
> >              ff_dlog(s, "bitmap: ch=%d fmt=%d %dx%d (linesize=%d) len=%d->%ld pal=%d\n",
> >                      ch_id, bmp_fmt, width, height, linesize, len, out_len, colormapsize);
> >  
> > +            if (len * 17373LL < out_len)
> 
> Where does the magic number come from?

A very quick simulation of the best case compression for "compress"
below is not nice written code as i did not expect I or anyone else
would ever see it again

I would have preferred some nicer expression or course, but thats
what it seems to be asymptotically. For smaller amounts of data a
tighter bound is possible but i saw no nice way to consider that 
and it seems also overkill to try to do it more fine grained for
just this 

main(){
    int64_t bits = 0;
    int bank = 256;
    int bitbank = 8;
    for(unsigned i = 0; i<1024*1024*1024*4U-100000;) {
        int word_size = bank-255;
        i += word_size;
        bits += bitbank;

        if (!(bank & (bank-1)))
            bitbank ++;
        bank++;
        if (bitbank > 16) {
            printf("BEST %f \n", 8.0 * i / bits );
            bank = 256;
            bitbank = 8;
        }
    }
}

above assumes i remembered correctly how the algorithm works but the
value was close to what actual compession of zeros gave


[...]
Anton Khirnov Jan. 24, 2021, 1:05 p.m. UTC | #3
Quoting Michael Niedermayer (2021-01-23 23:34:19)
> On Sat, Jan 23, 2021 at 03:29:38PM +0100, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2021-01-22 15:09:47)
> > > Fixes: Timeout (too long -> 241ms)
> > > Fixes: 29083/clusterfuzz-testcase-minimized-ffmpeg_dem_SWF_fuzzer-6273684478230528
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/swfdec.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/libavformat/swfdec.c b/libavformat/swfdec.c
> > > index 1463f0ad4d..aa4be88f91 100644
> > > --- a/libavformat/swfdec.c
> > > +++ b/libavformat/swfdec.c
> > > @@ -367,6 +367,9 @@ static int swf_read_packet(AVFormatContext *s, AVPacket *pkt)
> > >              ff_dlog(s, "bitmap: ch=%d fmt=%d %dx%d (linesize=%d) len=%d->%ld pal=%d\n",
> > >                      ch_id, bmp_fmt, width, height, linesize, len, out_len, colormapsize);
> > >  
> > > +            if (len * 17373LL < out_len)
> > 
> > Where does the magic number come from?
> 
> A very quick simulation of the best case compression for "compress"
> below is not nice written code as i did not expect I or anyone else
> would ever see it again
> 
> I would have preferred some nicer expression or course, but thats
> what it seems to be asymptotically. For smaller amounts of data a
> tighter bound is possible but i saw no nice way to consider that 
> and it seems also overkill to try to do it more fine grained for
> just this 
> 
> main(){
>     int64_t bits = 0;
>     int bank = 256;
>     int bitbank = 8;
>     for(unsigned i = 0; i<1024*1024*1024*4U-100000;) {
>         int word_size = bank-255;
>         i += word_size;
>         bits += bitbank;
> 
>         if (!(bank & (bank-1)))
>             bitbank ++;
>         bank++;
>         if (bitbank > 16) {
>             printf("BEST %f \n", 8.0 * i / bits );
>             bank = 256;
>             bitbank = 8;
>         }
>     }
> }
> 
> above assumes i remembered correctly how the algorithm works but the
> value was close to what actual compession of zeros gave

People who read this code in the future will be interested in all this.
So the content of your reply should be added to the commit message
and/or the code itself.
Michael Niedermayer March 8, 2021, 8:48 p.m. UTC | #4
On Sun, Jan 24, 2021 at 02:05:17PM +0100, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2021-01-23 23:34:19)
> > On Sat, Jan 23, 2021 at 03:29:38PM +0100, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2021-01-22 15:09:47)
> > > > Fixes: Timeout (too long -> 241ms)
> > > > Fixes: 29083/clusterfuzz-testcase-minimized-ffmpeg_dem_SWF_fuzzer-6273684478230528
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavformat/swfdec.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/libavformat/swfdec.c b/libavformat/swfdec.c
> > > > index 1463f0ad4d..aa4be88f91 100644
> > > > --- a/libavformat/swfdec.c
> > > > +++ b/libavformat/swfdec.c
> > > > @@ -367,6 +367,9 @@ static int swf_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > >              ff_dlog(s, "bitmap: ch=%d fmt=%d %dx%d (linesize=%d) len=%d->%ld pal=%d\n",
> > > >                      ch_id, bmp_fmt, width, height, linesize, len, out_len, colormapsize);
> > > >  
> > > > +            if (len * 17373LL < out_len)
> > > 
> > > Where does the magic number come from?
> > 
> > A very quick simulation of the best case compression for "compress"
> > below is not nice written code as i did not expect I or anyone else
> > would ever see it again
> > 
> > I would have preferred some nicer expression or course, but thats
> > what it seems to be asymptotically. For smaller amounts of data a
> > tighter bound is possible but i saw no nice way to consider that 
> > and it seems also overkill to try to do it more fine grained for
> > just this 
> > 
> > main(){
> >     int64_t bits = 0;
> >     int bank = 256;
> >     int bitbank = 8;
> >     for(unsigned i = 0; i<1024*1024*1024*4U-100000;) {
> >         int word_size = bank-255;
> >         i += word_size;
> >         bits += bitbank;
> > 
> >         if (!(bank & (bank-1)))
> >             bitbank ++;
> >         bank++;
> >         if (bitbank > 16) {
> >             printf("BEST %f \n", 8.0 * i / bits );
> >             bank = 256;
> >             bitbank = 8;
> >         }
> >     }
> > }
> > 
> > above assumes i remembered correctly how the algorithm works but the
> > value was close to what actual compession of zeros gave
> 
> People who read this code in the future will be interested in all this.
> So the content of your reply should be added to the commit message
> and/or the code itself.

will apply with the message in the commit message

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/swfdec.c b/libavformat/swfdec.c
index 1463f0ad4d..aa4be88f91 100644
--- a/libavformat/swfdec.c
+++ b/libavformat/swfdec.c
@@ -367,6 +367,9 @@  static int swf_read_packet(AVFormatContext *s, AVPacket *pkt)
             ff_dlog(s, "bitmap: ch=%d fmt=%d %dx%d (linesize=%d) len=%d->%ld pal=%d\n",
                     ch_id, bmp_fmt, width, height, linesize, len, out_len, colormapsize);
 
+            if (len * 17373LL < out_len)
+                goto bitmap_end_skip;
+
             zbuf = av_malloc(len);
             if (!zbuf) {
                 res = AVERROR(ENOMEM);