| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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 [...]
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 ? [...]
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
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 --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))
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(-)