[FFmpeg-devel,2/2] avcodec/tiff: Fix integer overflows in left shift in init_image()

Submitted by Michael Niedermayer on Nov. 23, 2018, 1:33 a.m.

Details

Message ID 20181123013305.16518-2-michael@niedermayer.cc
State Accepted
Headers show

Commit Message

Michael Niedermayer Nov. 23, 2018, 1:33 a.m.
Fixes: left shift of 255 by 24 places cannot be represented in type 'int'
Fixes: 11377/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TIFF_fuzzer-5694319101476864

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

Comments

Tomas Härdin Nov. 24, 2018, 10:46 a.m.
fre 2018-11-23 klockan 02:33 +0100 skrev Michael Niedermayer:
> Fixes: left shift of 255 by 24 places cannot be represented in type 'int'
> Fixes: 11377/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TIFF_fuzzer-5694319101476864
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/tiff.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> index 6271c937c3..64a3261fd9 100644
> --- a/libavcodec/tiff.c
> +++ b/libavcodec/tiff.c
> @@ -706,7 +706,7 @@ static int init_image(TiffContext *s, ThreadFrame *frame)
>          s->avctx->pix_fmt = s->palette_is_set ? AV_PIX_FMT_PAL8 : AV_PIX_FMT_GRAY8;
>          break;
>      case 10081:
> -        switch (s->pattern[0] | (s->pattern[1] << 8) | (s->pattern[2] << 16) | (s->pattern[3] << 24)) {
> +        switch (s->pattern[0] | (s->pattern[1] << 8) | (s->pattern[2] << 16) | ((unsigned)s->pattern[3] << 24)) {
>          case 0x02010100:
>              s->avctx->pix_fmt = AV_PIX_FMT_BAYER_RGGB8;
>              break;
> @@ -721,7 +721,7 @@ static int init_image(TiffContext *s, ThreadFrame *frame)
>              break;
>          default:
>              av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
> -                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
> +                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
>              return AVERROR_PATCHWELCOME;
>          }
>          break;
> @@ -741,12 +741,12 @@ static int init_image(TiffContext *s, ThreadFrame *frame)
>              break;
>          default:
>              av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
> -                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
> +                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
>              return AVERROR_PATCHWELCOME;
>          }
>          break;
>      case 10161:
> -        switch (s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24) {
> +        switch (s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24) {
>          case 0x02010100:
>              s->avctx->pix_fmt = s->le ? AV_PIX_FMT_BAYER_RGGB16LE : AV_PIX_FMT_BAYER_RGGB16BE;
>              break;
> @@ -761,7 +761,7 @@ static int init_image(TiffContext *s, ThreadFrame *frame)
>              break;
>          default:
>              av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
> -                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
> +                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
>              return AVERROR_PATCHWELCOME;
>          }
>          break;

So fixing undefined behavior mostly? A function might make this nicer
looking, else there's not much to say

/Tomas
Michael Niedermayer Nov. 25, 2018, 12:28 a.m.
On Sat, Nov 24, 2018 at 11:46:36AM +0100, Tomas Härdin wrote:
> fre 2018-11-23 klockan 02:33 +0100 skrev Michael Niedermayer:
> > Fixes: left shift of 255 by 24 places cannot be represented in type 'int'
> > Fixes: 11377/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TIFF_fuzzer-5694319101476864
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/tiff.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> > index 6271c937c3..64a3261fd9 100644
> > --- a/libavcodec/tiff.c
> > +++ b/libavcodec/tiff.c
> > @@ -706,7 +706,7 @@ static int init_image(TiffContext *s, ThreadFrame *frame)
> >          s->avctx->pix_fmt = s->palette_is_set ? AV_PIX_FMT_PAL8 : AV_PIX_FMT_GRAY8;
> >          break;
> >      case 10081:
> > -        switch (s->pattern[0] | (s->pattern[1] << 8) | (s->pattern[2] << 16) | (s->pattern[3] << 24)) {
> > +        switch (s->pattern[0] | (s->pattern[1] << 8) | (s->pattern[2] << 16) | ((unsigned)s->pattern[3] << 24)) {
> >          case 0x02010100:
> >              s->avctx->pix_fmt = AV_PIX_FMT_BAYER_RGGB8;
> >              break;
> > @@ -721,7 +721,7 @@ static int init_image(TiffContext *s, ThreadFrame *frame)
> >              break;
> >          default:
> >              av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
> > -                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
> > +                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
> >              return AVERROR_PATCHWELCOME;
> >          }
> >          break;
> > @@ -741,12 +741,12 @@ static int init_image(TiffContext *s, ThreadFrame *frame)
> >              break;
> >          default:
> >              av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
> > -                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
> > +                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
> >              return AVERROR_PATCHWELCOME;
> >          }
> >          break;
> >      case 10161:
> > -        switch (s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24) {
> > +        switch (s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24) {
> >          case 0x02010100:
> >              s->avctx->pix_fmt = s->le ? AV_PIX_FMT_BAYER_RGGB16LE : AV_PIX_FMT_BAYER_RGGB16BE;
> >              break;
> > @@ -761,7 +761,7 @@ static int init_image(TiffContext *s, ThreadFrame *frame)
> >              break;
> >          default:
> >              av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
> > -                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
> > +                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
> >              return AVERROR_PATCHWELCOME;
> >          }
> >          break;
> 
> So fixing undefined behavior mostly? A function might make this nicer
> looking, else there's not much to say

yes, indeed, ill post a patch doing that

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index 6271c937c3..64a3261fd9 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -706,7 +706,7 @@  static int init_image(TiffContext *s, ThreadFrame *frame)
         s->avctx->pix_fmt = s->palette_is_set ? AV_PIX_FMT_PAL8 : AV_PIX_FMT_GRAY8;
         break;
     case 10081:
-        switch (s->pattern[0] | (s->pattern[1] << 8) | (s->pattern[2] << 16) | (s->pattern[3] << 24)) {
+        switch (s->pattern[0] | (s->pattern[1] << 8) | (s->pattern[2] << 16) | ((unsigned)s->pattern[3] << 24)) {
         case 0x02010100:
             s->avctx->pix_fmt = AV_PIX_FMT_BAYER_RGGB8;
             break;
@@ -721,7 +721,7 @@  static int init_image(TiffContext *s, ThreadFrame *frame)
             break;
         default:
             av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
-                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
+                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
             return AVERROR_PATCHWELCOME;
         }
         break;
@@ -741,12 +741,12 @@  static int init_image(TiffContext *s, ThreadFrame *frame)
             break;
         default:
             av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
-                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
+                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
             return AVERROR_PATCHWELCOME;
         }
         break;
     case 10161:
-        switch (s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24) {
+        switch (s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24) {
         case 0x02010100:
             s->avctx->pix_fmt = s->le ? AV_PIX_FMT_BAYER_RGGB16LE : AV_PIX_FMT_BAYER_RGGB16BE;
             break;
@@ -761,7 +761,7 @@  static int init_image(TiffContext *s, ThreadFrame *frame)
             break;
         default:
             av_log(s->avctx, AV_LOG_ERROR, "Unsupported Bayer pattern: 0x%X\n",
-                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | s->pattern[3] << 24);
+                   s->pattern[0] | s->pattern[1] << 8 | s->pattern[2] << 16 | (unsigned)s->pattern[3] << 24);
             return AVERROR_PATCHWELCOME;
         }
         break;