diff mbox series

[FFmpeg-devel,2/3] avcodec/pgxdec: Check depth more completely

Message ID 20201008191915.18532-2-michael@niedermayer.cc
State Accepted
Commit 389b9e9b4f680ff2f7a957559ab6f4edfd94f6ae
Headers show
Series [FFmpeg-devel,1/3] avcodec/h264_slice: fix undefined integer overflow with POC in error concealment
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Michael Niedermayer Oct. 8, 2020, 7:19 p.m. UTC
Fixes: shift exponent -1 is negative
Fixes: 26107/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PGX_fuzzer-5378790047612928

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

Comments

Andreas Rheinhardt Oct. 8, 2020, 7:48 p.m. UTC | #1
Michael Niedermayer:
> Fixes: shift exponent -1 is negative
> Fixes: 26107/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PGX_fuzzer-5378790047612928
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/pgxdec.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c
> index 150f8bbf66..5c735894ab 100644
> --- a/libavcodec/pgxdec.c
> +++ b/libavcodec/pgxdec.c
> @@ -133,14 +133,14 @@ static int pgx_decode_frame(AVCodecContext *avctx, void *data,
>      if ((ret = ff_set_dimensions(avctx, width, height)) < 0)
>          return ret;
>  
> -    if (depth <= 8) {
> +    if (depth > 0 && depth <= 8) {
>          avctx->pix_fmt = AV_PIX_FMT_GRAY8;
>          bpp = 8;
> -    } else if (depth <= 16) {
> +    } else if (depth > 0 && depth <= 16) {
>          avctx->pix_fmt = AV_PIX_FMT_GRAY16;
>          bpp = 16;
>      } else {
> -        av_log(avctx, AV_LOG_ERROR, "Maximum depth of 16 bits supported.\n");
> +        av_log(avctx, AV_LOG_ERROR, "depth %d is invalid or unsupported.\n", depth);
>          return AVERROR_PATCHWELCOME;
>      }
>      if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3))
> 
I presume the parsed depth to be zero in your testcase. Said number
comes from pgx_get_number() which is only used to parse values for which
a value of zero makes no sense. Wouldn't it make more sense to error out
there if the number is zero?

- Andreas
Michael Niedermayer Oct. 9, 2020, 4:52 p.m. UTC | #2
On Thu, Oct 08, 2020 at 09:48:41PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: shift exponent -1 is negative
> > Fixes: 26107/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PGX_fuzzer-5378790047612928
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/pgxdec.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c
> > index 150f8bbf66..5c735894ab 100644
> > --- a/libavcodec/pgxdec.c
> > +++ b/libavcodec/pgxdec.c
> > @@ -133,14 +133,14 @@ static int pgx_decode_frame(AVCodecContext *avctx, void *data,
> >      if ((ret = ff_set_dimensions(avctx, width, height)) < 0)
> >          return ret;
> >  
> > -    if (depth <= 8) {
> > +    if (depth > 0 && depth <= 8) {
> >          avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> >          bpp = 8;
> > -    } else if (depth <= 16) {
> > +    } else if (depth > 0 && depth <= 16) {
> >          avctx->pix_fmt = AV_PIX_FMT_GRAY16;
> >          bpp = 16;
> >      } else {
> > -        av_log(avctx, AV_LOG_ERROR, "Maximum depth of 16 bits supported.\n");
> > +        av_log(avctx, AV_LOG_ERROR, "depth %d is invalid or unsupported.\n", depth);
> >          return AVERROR_PATCHWELCOME;
> >      }
> >      if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3))
> > 
> I presume the parsed depth to be zero in your testcase. Said number
> comes from pgx_get_number() which is only used to parse values for which
> a value of zero makes no sense. Wouldn't it make more sense to error out
> there if the number is zero?

I was considering that too when i wrote the patch
the reason why i picked this is thats where depth is checked already.
Checking for depth == 0 in one function and depth > 16 in another feels
like its not the clearest way to implement that ...

thx

[...]
Michael Niedermayer Oct. 24, 2020, 3:20 p.m. UTC | #3
On Fri, Oct 09, 2020 at 06:52:18PM +0200, Michael Niedermayer wrote:
> On Thu, Oct 08, 2020 at 09:48:41PM +0200, Andreas Rheinhardt wrote:
> > Michael Niedermayer:
> > > Fixes: shift exponent -1 is negative
> > > Fixes: 26107/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PGX_fuzzer-5378790047612928
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/pgxdec.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c
> > > index 150f8bbf66..5c735894ab 100644
> > > --- a/libavcodec/pgxdec.c
> > > +++ b/libavcodec/pgxdec.c
> > > @@ -133,14 +133,14 @@ static int pgx_decode_frame(AVCodecContext *avctx, void *data,
> > >      if ((ret = ff_set_dimensions(avctx, width, height)) < 0)
> > >          return ret;
> > >  
> > > -    if (depth <= 8) {
> > > +    if (depth > 0 && depth <= 8) {
> > >          avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> > >          bpp = 8;
> > > -    } else if (depth <= 16) {
> > > +    } else if (depth > 0 && depth <= 16) {
> > >          avctx->pix_fmt = AV_PIX_FMT_GRAY16;
> > >          bpp = 16;
> > >      } else {
> > > -        av_log(avctx, AV_LOG_ERROR, "Maximum depth of 16 bits supported.\n");
> > > +        av_log(avctx, AV_LOG_ERROR, "depth %d is invalid or unsupported.\n", depth);
> > >          return AVERROR_PATCHWELCOME;
> > >      }
> > >      if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3))
> > > 
> > I presume the parsed depth to be zero in your testcase. Said number
> > comes from pgx_get_number() which is only used to parse values for which
> > a value of zero makes no sense. Wouldn't it make more sense to error out
> > there if the number is zero?
> 
> I was considering that too when i wrote the patch
> the reason why i picked this is thats where depth is checked already.
> Checking for depth == 0 in one function and depth > 16 in another feels
> like its not the clearest way to implement that ...

ok if i apply this ? 
or you still want it changed ?

[...]
Andreas Rheinhardt Oct. 24, 2020, 3:23 p.m. UTC | #4
Michael Niedermayer:
> On Fri, Oct 09, 2020 at 06:52:18PM +0200, Michael Niedermayer wrote:
>> On Thu, Oct 08, 2020 at 09:48:41PM +0200, Andreas Rheinhardt wrote:
>>> Michael Niedermayer:
>>>> Fixes: shift exponent -1 is negative
>>>> Fixes: 26107/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PGX_fuzzer-5378790047612928
>>>>
>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>>  libavcodec/pgxdec.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c
>>>> index 150f8bbf66..5c735894ab 100644
>>>> --- a/libavcodec/pgxdec.c
>>>> +++ b/libavcodec/pgxdec.c
>>>> @@ -133,14 +133,14 @@ static int pgx_decode_frame(AVCodecContext *avctx, void *data,
>>>>      if ((ret = ff_set_dimensions(avctx, width, height)) < 0)
>>>>          return ret;
>>>>  
>>>> -    if (depth <= 8) {
>>>> +    if (depth > 0 && depth <= 8) {
>>>>          avctx->pix_fmt = AV_PIX_FMT_GRAY8;
>>>>          bpp = 8;
>>>> -    } else if (depth <= 16) {
>>>> +    } else if (depth > 0 && depth <= 16) {
>>>>          avctx->pix_fmt = AV_PIX_FMT_GRAY16;
>>>>          bpp = 16;
>>>>      } else {
>>>> -        av_log(avctx, AV_LOG_ERROR, "Maximum depth of 16 bits supported.\n");
>>>> +        av_log(avctx, AV_LOG_ERROR, "depth %d is invalid or unsupported.\n", depth);
>>>>          return AVERROR_PATCHWELCOME;
>>>>      }
>>>>      if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3))
>>>>
>>> I presume the parsed depth to be zero in your testcase. Said number
>>> comes from pgx_get_number() which is only used to parse values for which
>>> a value of zero makes no sense. Wouldn't it make more sense to error out
>>> there if the number is zero?
>>
>> I was considering that too when i wrote the patch
>> the reason why i picked this is thats where depth is checked already.
>> Checking for depth == 0 in one function and depth > 16 in another feels
>> like its not the clearest way to implement that ...
> 
> ok if i apply this ? 
> or you still want it changed ?
> 
I don't care. Proceed as you want.

- Andreas
Michael Niedermayer Oct. 25, 2020, 8:44 a.m. UTC | #5
On Sat, Oct 24, 2020 at 05:23:47PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Fri, Oct 09, 2020 at 06:52:18PM +0200, Michael Niedermayer wrote:
> >> On Thu, Oct 08, 2020 at 09:48:41PM +0200, Andreas Rheinhardt wrote:
> >>> Michael Niedermayer:
> >>>> Fixes: shift exponent -1 is negative
> >>>> Fixes: 26107/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PGX_fuzzer-5378790047612928
> >>>>
> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>> ---
> >>>>  libavcodec/pgxdec.c | 6 +++---
> >>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c
> >>>> index 150f8bbf66..5c735894ab 100644
> >>>> --- a/libavcodec/pgxdec.c
> >>>> +++ b/libavcodec/pgxdec.c
> >>>> @@ -133,14 +133,14 @@ static int pgx_decode_frame(AVCodecContext *avctx, void *data,
> >>>>      if ((ret = ff_set_dimensions(avctx, width, height)) < 0)
> >>>>          return ret;
> >>>>  
> >>>> -    if (depth <= 8) {
> >>>> +    if (depth > 0 && depth <= 8) {
> >>>>          avctx->pix_fmt = AV_PIX_FMT_GRAY8;
> >>>>          bpp = 8;
> >>>> -    } else if (depth <= 16) {
> >>>> +    } else if (depth > 0 && depth <= 16) {
> >>>>          avctx->pix_fmt = AV_PIX_FMT_GRAY16;
> >>>>          bpp = 16;
> >>>>      } else {
> >>>> -        av_log(avctx, AV_LOG_ERROR, "Maximum depth of 16 bits supported.\n");
> >>>> +        av_log(avctx, AV_LOG_ERROR, "depth %d is invalid or unsupported.\n", depth);
> >>>>          return AVERROR_PATCHWELCOME;
> >>>>      }
> >>>>      if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3))
> >>>>
> >>> I presume the parsed depth to be zero in your testcase. Said number
> >>> comes from pgx_get_number() which is only used to parse values for which
> >>> a value of zero makes no sense. Wouldn't it make more sense to error out
> >>> there if the number is zero?
> >>
> >> I was considering that too when i wrote the patch
> >> the reason why i picked this is thats where depth is checked already.
> >> Checking for depth == 0 in one function and depth > 16 in another feels
> >> like its not the clearest way to implement that ...
> > 
> > ok if i apply this ? 
> > or you still want it changed ?
> > 
> I don't care. Proceed as you want.

ok

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/pgxdec.c b/libavcodec/pgxdec.c
index 150f8bbf66..5c735894ab 100644
--- a/libavcodec/pgxdec.c
+++ b/libavcodec/pgxdec.c
@@ -133,14 +133,14 @@  static int pgx_decode_frame(AVCodecContext *avctx, void *data,
     if ((ret = ff_set_dimensions(avctx, width, height)) < 0)
         return ret;
 
-    if (depth <= 8) {
+    if (depth > 0 && depth <= 8) {
         avctx->pix_fmt = AV_PIX_FMT_GRAY8;
         bpp = 8;
-    } else if (depth <= 16) {
+    } else if (depth > 0 && depth <= 16) {
         avctx->pix_fmt = AV_PIX_FMT_GRAY16;
         bpp = 16;
     } else {
-        av_log(avctx, AV_LOG_ERROR, "Maximum depth of 16 bits supported.\n");
+        av_log(avctx, AV_LOG_ERROR, "depth %d is invalid or unsupported.\n", depth);
         return AVERROR_PATCHWELCOME;
     }
     if (bytestream2_get_bytes_left(&g) < width * height * (bpp >> 3))