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

[Michael Niedermayer]

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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Never trust a computer, one day, it may think you are the virus. -- Compn
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20201025/3836ecc7/attachment.sig>