diff mbox

[FFmpeg-devel,2/2] avcodec/adpcm: Clip predictor for APC

Message ID 20191121222612.22954-2-michael@niedermayer.cc
State Accepted
Commit 9fe07908c3f67d59cf4db5668d61b34506189590
Headers show

Commit Message

Michael Niedermayer Nov. 21, 2019, 10:26 p.m. UTC
Fixes: signed integer overflow: -2147483648 - 13 cannot be represented in type 'int'
Fixes: 18893/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ADPCM_IMA_APC_fuzzer-5630760442920960

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

Comments

Limin Wang Nov. 22, 2019, 11:20 a.m. UTC | #1
On Thu, Nov 21, 2019 at 11:26:12PM +0100, Michael Niedermayer wrote:
> Fixes: signed integer overflow: -2147483648 - 13 cannot be represented in type 'int'
> Fixes: 18893/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ADPCM_IMA_APC_fuzzer-5630760442920960
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/adpcm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/adpcm.c b/libavcodec/adpcm.c
> index edf7052636..d3919288b1 100644
> --- a/libavcodec/adpcm.c
> +++ b/libavcodec/adpcm.c
> @@ -140,8 +140,8 @@ static av_cold int adpcm_decode_init(AVCodecContext * avctx)
>          break;
>      case AV_CODEC_ID_ADPCM_IMA_APC:
>          if (avctx->extradata && avctx->extradata_size >= 8) {
> -            c->status[0].predictor = AV_RL32(avctx->extradata);
> -            c->status[1].predictor = AV_RL32(avctx->extradata + 4);
> +            c->status[0].predictor = av_clip_intp2(AV_RL32(avctx->extradata    ), 18);
> +            c->status[1].predictor = av_clip_intp2(AV_RL32(avctx->extradata + 4), 18);

18 is 16? I understand c->status[i].predictor is 16bit.

>          }
>          break;
>      case AV_CODEC_ID_ADPCM_IMA_WS:
> -- 
> 2.23.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".
Michael Niedermayer Nov. 23, 2019, 5:40 p.m. UTC | #2
On Fri, Nov 22, 2019 at 07:20:55PM +0800, Limin Wang wrote:
> On Thu, Nov 21, 2019 at 11:26:12PM +0100, Michael Niedermayer wrote:
> > Fixes: signed integer overflow: -2147483648 - 13 cannot be represented in type 'int'
> > Fixes: 18893/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ADPCM_IMA_APC_fuzzer-5630760442920960
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/adpcm.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/adpcm.c b/libavcodec/adpcm.c
> > index edf7052636..d3919288b1 100644
> > --- a/libavcodec/adpcm.c
> > +++ b/libavcodec/adpcm.c
> > @@ -140,8 +140,8 @@ static av_cold int adpcm_decode_init(AVCodecContext * avctx)
> >          break;
> >      case AV_CODEC_ID_ADPCM_IMA_APC:
> >          if (avctx->extradata && avctx->extradata_size >= 8) {
> > -            c->status[0].predictor = AV_RL32(avctx->extradata);
> > -            c->status[1].predictor = AV_RL32(avctx->extradata + 4);
> > +            c->status[0].predictor = av_clip_intp2(AV_RL32(avctx->extradata    ), 18);
> > +            c->status[1].predictor = av_clip_intp2(AV_RL32(avctx->extradata + 4), 18);
> 
> 18 is 16?

18


> I understand c->status[i].predictor is 16bit.

ADPCMChannelStatus.predictor is an int so 32bit

the place where predictor is used is adpcm_ima_expand_nibble()
there its added to diff 

diff is maximally 32767*15 / 8 = 61438
if now predictor was -70000 then we would get -8562
if predictor was cliped to 16bit (-32768) would produce 28670
thats different. Same for 17bit which is why i choose 18.
Its the smallest that does not change the output in any case.

Also i dont see any cliping of the global header values in
https://wiki.multimedia.cx/index.php/CRYO_APC

thx

[...]
diff mbox

Patch

diff --git a/libavcodec/adpcm.c b/libavcodec/adpcm.c
index edf7052636..d3919288b1 100644
--- a/libavcodec/adpcm.c
+++ b/libavcodec/adpcm.c
@@ -140,8 +140,8 @@  static av_cold int adpcm_decode_init(AVCodecContext * avctx)
         break;
     case AV_CODEC_ID_ADPCM_IMA_APC:
         if (avctx->extradata && avctx->extradata_size >= 8) {
-            c->status[0].predictor = AV_RL32(avctx->extradata);
-            c->status[1].predictor = AV_RL32(avctx->extradata + 4);
+            c->status[0].predictor = av_clip_intp2(AV_RL32(avctx->extradata    ), 18);
+            c->status[1].predictor = av_clip_intp2(AV_RL32(avctx->extradata + 4), 18);
         }
         break;
     case AV_CODEC_ID_ADPCM_IMA_WS: