diff mbox

[FFmpeg-devel,1/2] avcodec/mpeg12dec: Fix invalid shift in mpeg2_fast_decode_block_intra()

Message ID 20191226001809.28204-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Dec. 26, 2019, 12:18 a.m. UTC
Fixes: left shift of negative value -695
Fixes: 19232/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG1VIDEO_fuzzer-5702856963522560
Fixes: 19555/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG1VIDEO_fuzzer-5741218147598336

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

Comments

Kieran Kunhya Dec. 26, 2019, 2:13 a.m. UTC | #1
On Thu, 26 Dec 2019 at 00:27, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> Fixes: left shift of negative value -695
> Fixes:
> 19232/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG1VIDEO_fuzzer-5702856963522560
> Fixes:
> 19555/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG1VIDEO_fuzzer-5741218147598336
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by
> <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
> Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/mpeg12dec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index 775579f9f0..4643992d28 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -586,7 +586,7 @@ static inline int
> mpeg2_fast_decode_block_intra(MpegEncContext *s,


Can we not remove this "fast" code? Decoding MPEG-2 is not exactly hard in
2019.
Also it has the following comment associated with it:

/**
 * Note: this function can read out of range and crash for corrupt streams.
 * Changing this would eat up any speed benefits it has.
 * Do not use "fast" flag if you need the code to be robust.
 */

If you want to make it robust you might as well just use the real decode
function

Kieran
Michael Niedermayer Jan. 30, 2020, 6:54 p.m. UTC | #2
On Thu, Dec 26, 2019 at 02:13:59AM +0000, Kieran Kunhya wrote:
> On Thu, 26 Dec 2019 at 00:27, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > Fixes: left shift of negative value -695
> > Fixes:
> > 19232/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG1VIDEO_fuzzer-5702856963522560
> > Fixes:
> > 19555/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_MPEG1VIDEO_fuzzer-5741218147598336
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by
> > <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
> > Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/mpeg12dec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> > index 775579f9f0..4643992d28 100644
> > --- a/libavcodec/mpeg12dec.c
> > +++ b/libavcodec/mpeg12dec.c
> > @@ -586,7 +586,7 @@ static inline int
> > mpeg2_fast_decode_block_intra(MpegEncContext *s,
> 
> 
[...]

> Also it has the following comment associated with it:
> 
> /**
>  * Note: this function can read out of range and crash for corrupt streams.
>  * Changing this would eat up any speed benefits it has.
>  * Do not use "fast" flag if you need the code to be robust.
>  */
> 
> If you want to make it robust you might as well just use the real decode
> function

People wanted to maximize code coverage of the fuzzer. So it fuzzes such
cases too now. 
I dont see any harm in fixing issues like the one this patch is about.
and that the codepath is inherently not robust as part of what its intended
for shouldnt be an argument to not fix issues we can easily fix.

About removing it, its very easy to fix (the case here) but removing it from old releases
would not be easy and also i dont see how that could be justified to its
users.

So i think this patch should be applied

Thanks


[...]
Carl Eugen Hoyos Jan. 30, 2020, 10:35 p.m. UTC | #3
Am Do., 26. Dez. 2019 um 03:22 Uhr schrieb Kieran Kunhya <kierank@obe.tv>:

> Can we not remove this "fast" code? Decoding MPEG-2 is not exactly hard in
> 2019.

The world is not a VAX, or multi-cpu x86-

> Also it has the following comment associated with it:
>
> /**
>  * Note: this function can read out of range and crash for corrupt streams.
>  * Changing this would eat up any speed benefits it has.
>  * Do not use "fast" flag if you need the code to be robust.
>  */
>
> If you want to make it robust you might as well just use the real decode
> function

Is it possible that the comment is wrong?
I seem to remember a change when the issue was last discussed.

Carl Eugen
diff mbox

Patch

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 775579f9f0..4643992d28 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -586,7 +586,7 @@  static inline int mpeg2_fast_decode_block_intra(MpegEncContext *s,
     dc = s->last_dc[component];
     dc += diff;
     s->last_dc[component] = dc;
-    block[0] = dc << (3 - s->intra_dc_precision);
+    block[0] = dc * (1 << (3 - s->intra_dc_precision));
     i = 0;
     if (s->intra_vlc_format)
         rl = &ff_rl_mpeg2;