[FFmpeg-devel,5/8] avcodec/flicvideo: Optimize and Simplify FLI_COPY in flic_decode_frame_24BPP() by using bytestream2_get_buffer()

Submitted by Michael Niedermayer on Aug. 12, 2019, 7:17 p.m.

Details

Message ID 20190812191708.22608-5-michael@niedermayer.cc
State Accepted
Commit e301736862f18a449c317a47d0d60d3484e41667
Headers show

Commit Message

Michael Niedermayer Aug. 12, 2019, 7:17 p.m.
Fixes: Timeout (31sec  -> 22sec)
Fixes: 16217/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLIC_fuzzer-5658084189405184

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/flicvideo.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

Tomas Härdin Aug. 14, 2019, 3:21 p.m.
mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> Fixes: Timeout (31sec  -> 22sec)

Is this a large test case? 22sec still sounds excessive

> -                    pixel_countdown = s->avctx->width;
> -                    pixel_ptr = 0;
> -                    while (pixel_countdown > 0) {
> -                        pixel = bytestream2_get_le24(&g2);
> -                        AV_WL24(&pixels[y_ptr + pixel_ptr], pixel);
> -                        pixel_ptr += 3;
> -                        pixel_countdown--;
> -                    }
> +                    bytestream2_get_buffer(&g2, pixels + y_ptr, 3*s-
> >avctx->width);

Looks OK

/Tomas
Michael Niedermayer Aug. 14, 2019, 5:42 p.m.
On Wed, Aug 14, 2019 at 05:21:49PM +0200, Tomas Härdin wrote:
> mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > Fixes: Timeout (31sec  -> 22sec)
> 
> Is this a large test case? 22sec still sounds excessive

the input is about 240kbyte so 22sec is not great but thats what you get
copying large frames around


> 
> > -                    pixel_countdown = s->avctx->width;
> > -                    pixel_ptr = 0;
> > -                    while (pixel_countdown > 0) {
> > -                        pixel = bytestream2_get_le24(&g2);
> > -                        AV_WL24(&pixels[y_ptr + pixel_ptr], pixel);
> > -                        pixel_ptr += 3;
> > -                        pixel_countdown--;
> > -                    }
> > +                    bytestream2_get_buffer(&g2, pixels + y_ptr, 3*s-
> > >avctx->width);
> 
> Looks OK

will apply

thx

[...]
Tomas Härdin Aug. 15, 2019, 8:55 a.m.
ons 2019-08-14 klockan 19:42 +0200 skrev Michael Niedermayer:
> On Wed, Aug 14, 2019 at 05:21:49PM +0200, Tomas Härdin wrote:
> > mån 2019-08-12 klockan 21:17 +0200 skrev Michael Niedermayer:
> > > Fixes: Timeout (31sec  -> 22sec)
> > 
> > Is this a large test case? 22sec still sounds excessive
> 
> the input is about 240kbyte so 22sec is not great but thats what you get
> copying large frames around

Is there any way to take ownership of the buffer? The best copy is the
one you avoid.. Of course that won't work if there's more than one copy
chunk.

Come to think of it, any codec is "vulnerable" to these kinds of
crafted files. Eh

/Tomas

Patch hide | download patch | download mbox

diff --git a/libavcodec/flicvideo.c b/libavcodec/flicvideo.c
index 99868f3ba3..bf8ffeba4f 100644
--- a/libavcodec/flicvideo.c
+++ b/libavcodec/flicvideo.c
@@ -1024,14 +1024,7 @@  static int flic_decode_frame_24BPP(AVCodecContext *avctx,
                 for (y_ptr = 0; y_ptr < s->frame->linesize[0] * s->avctx->height;
                      y_ptr += s->frame->linesize[0]) {
 
-                    pixel_countdown = s->avctx->width;
-                    pixel_ptr = 0;
-                    while (pixel_countdown > 0) {
-                        pixel = bytestream2_get_le24(&g2);
-                        AV_WL24(&pixels[y_ptr + pixel_ptr], pixel);
-                        pixel_ptr += 3;
-                        pixel_countdown--;
-                    }
+                    bytestream2_get_buffer(&g2, pixels + y_ptr, 3*s->avctx->width);
                     if (s->avctx->width & 1)
                         bytestream2_skip(&g2, 3);
                 }