| Message ID | 20191226001809.28204-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Headers | show |
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
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 [...]
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 --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;
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(-)