diff mbox

[FFmpeg-devel,2/2] avcodec/vc1_block: Fix invalid shifts in vc1_decode_i_blocks()

Message ID 20190622122143.24908-2-michael@niedermayer.cc
State Accepted
Commit c9415e815a996d287850a3572ce2c1d663b9f657
Headers show

Commit Message

Michael Niedermayer June 22, 2019, 12:21 p.m. UTC
Fixes: left shift of negative value -9
Fixes: 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728

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

Comments

Paul B Mahol June 22, 2019, 2:55 p.m. UTC | #1
On 6/22/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> Fixes: left shift of negative value -9
> Fixes:
> 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vc1_block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c
> index 7e41791832..6137252580 100644
> --- a/libavcodec/vc1_block.c
> +++ b/libavcodec/vc1_block.c
> @@ -2600,13 +2600,13 @@ static void vc1_decode_i_blocks(VC1Context *v)
>                  if (v->rangeredfrm)
>                      for (k = 0; k < 6; k++)
>                          for (j = 0; j < 64; j++)
> -                            v->block[v->cur_blk_idx][block_map[k]][j] <<=
> 1;
> +                            v->block[v->cur_blk_idx][block_map[k]][j] *= 2;
>                  vc1_put_blocks_clamped(v, 1);
>              } else {
>                  if (v->rangeredfrm)
>                      for (k = 0; k < 6; k++)
>                          for (j = 0; j < 64; j++)
> -                            v->block[v->cur_blk_idx][block_map[k]][j] =
> (v->block[v->cur_blk_idx][block_map[k]][j] - 64) << 1;
> +                            v->block[v->cur_blk_idx][block_map[k]][j] =
> (v->block[v->cur_blk_idx][block_map[k]][j] - 64) * 2;
>                  vc1_put_blocks_clamped(v, 0);
>              }
>
> --
> 2.22.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".


This is much slower.
Michael Niedermayer June 26, 2019, 10:40 p.m. UTC | #2
On Sat, Jun 22, 2019 at 04:55:47PM +0200, Paul B Mahol wrote:
> On 6/22/19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> > Fixes: left shift of negative value -9
> > Fixes:
> > 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/vc1_block.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c
> > index 7e41791832..6137252580 100644
> > --- a/libavcodec/vc1_block.c
> > +++ b/libavcodec/vc1_block.c
> > @@ -2600,13 +2600,13 @@ static void vc1_decode_i_blocks(VC1Context *v)
> >                  if (v->rangeredfrm)
> >                      for (k = 0; k < 6; k++)
> >                          for (j = 0; j < 64; j++)
> > -                            v->block[v->cur_blk_idx][block_map[k]][j] <<=
> > 1;
> > +                            v->block[v->cur_blk_idx][block_map[k]][j] *= 2;
> >                  vc1_put_blocks_clamped(v, 1);
> >              } else {
> >                  if (v->rangeredfrm)
> >                      for (k = 0; k < 6; k++)
> >                          for (j = 0; j < 64; j++)
> > -                            v->block[v->cur_blk_idx][block_map[k]][j] =
> > (v->block[v->cur_blk_idx][block_map[k]][j] - 64) << 1;
> > +                            v->block[v->cur_blk_idx][block_map[k]][j] =
> > (v->block[v->cur_blk_idx][block_map[k]][j] - 64) * 2;
> >                  vc1_put_blocks_clamped(v, 0);
> >              }
> >
> > --
> > 2.22.0
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 
> 
> This is much slower.

please provide your testcase and benchmarks or disassmbly

thanks

[...]
Michael Niedermayer July 8, 2019, 6:40 a.m. UTC | #3
On Sat, Jun 22, 2019 at 02:21:43PM +0200, Michael Niedermayer wrote:
> Fixes: left shift of negative value -9
> Fixes: 15299/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MSS2_fuzzer-5660922678345728
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vc1_block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Will apply these 2 in a few days unless there are further objections, benchmarks, 
disassemblies showing a slowdown
My tests indicate as already stated that the asm code generated is identical


[...]
diff mbox

Patch

diff --git a/libavcodec/vc1_block.c b/libavcodec/vc1_block.c
index 7e41791832..6137252580 100644
--- a/libavcodec/vc1_block.c
+++ b/libavcodec/vc1_block.c
@@ -2600,13 +2600,13 @@  static void vc1_decode_i_blocks(VC1Context *v)
                 if (v->rangeredfrm)
                     for (k = 0; k < 6; k++)
                         for (j = 0; j < 64; j++)
-                            v->block[v->cur_blk_idx][block_map[k]][j] <<= 1;
+                            v->block[v->cur_blk_idx][block_map[k]][j] *= 2;
                 vc1_put_blocks_clamped(v, 1);
             } else {
                 if (v->rangeredfrm)
                     for (k = 0; k < 6; k++)
                         for (j = 0; j < 64; j++)
-                            v->block[v->cur_blk_idx][block_map[k]][j] = (v->block[v->cur_blk_idx][block_map[k]][j] - 64) << 1;
+                            v->block[v->cur_blk_idx][block_map[k]][j] = (v->block[v->cur_blk_idx][block_map[k]][j] - 64) * 2;
                 vc1_put_blocks_clamped(v, 0);
             }