On 6/19/2019 3:59 PM, James Almer wrote: > On 6/19/2019 3:13 PM, Michael Niedermayer wrote: >> On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote: >>> On 6/19/2019 6:22 AM, Michael Niedermayer wrote: >>>> On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote: >>>>> On 6/17/2019 6:54 PM, Michael Niedermayer wrote: >>>>>> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote: >>>>>>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote: >>>>>>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented >>>>>>>> in type 'int' >>>>>>>> Fixes: >>>>>>>> 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536 >>>>>>>> >>>>>>>> Found-by: continuous fuzzing process >>>>>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>>>>>>> Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> >>>>>>>> --- >>>>>>>> libavcodec/hevc_ps.c | 2 +- >>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c >>>>>>>> index 80df417e4f..0ed6682bb4 100644 >>>>>>>> --- a/libavcodec/hevc_ps.c >>>>>>>> +++ b/libavcodec/hevc_ps.c >>>>>>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, >>>>>>>> AVCodecContext *avctx, >>>>>>>> if (pps->num_tile_rows <= 0 || >>>>>>>> pps->num_tile_rows >= sps->height) { >>>>>>>> av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of >>>>>>>> range: %d\n", >>>>>>>> - pps->num_tile_rows - 1); >>>>>>>> + pps->num_tile_rows - 1U); >>>>>>> >>>>>>> The proper fix for this is making pps->num_tile_rows/cols unsigned. >>>> [...] >>>>>> >>>>>> is this here ok if num_tile_rows is 0 ? >>>>>> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg >>>>>> git) >>>>>> >>>>>> i would guess nearly everyone wold say yes without having seen the >>>>>> discussion about the type. but of course if this is unsigned its not >>>>>> going to be safe with it being 0. >>>>> >>>>> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() + >>>>> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as >>>>> it would be a bug. Int is definitely not the right type for it. >>>> >>>> i dont think num_tile_rows of more than 2^31 (that is the negative when >>>> signed range) >>>> makes much sense but sure i can change it to unsigned if preferred. >>> >>> Of course it doesn't. In pretty much any sample it will be at least 1 >>> and at most 22, which is the max value allowed by hevc level 6.2 in >>> table A.6. Only if the stream reports an undefined level it could go up >>> to, if i'm reading this right, sps->ctb_height and not sps->height as >>> the decoder is currently checking. This means you can even replace >>> get_ue_golomb_long() for a get_ue_golomb(). That would also fix this. >>> >>> In any case, the bitstream value is unsigned, so the struct field should >>> be unsigned as well. Having it be signed and assigning it a value using >>> a function that potentially returns huge unsigned values introduced this >>> issue to being with, so instead of working around it using type >>> promotion, just make it follow the spec. >> >> what would be your feeling/oppinon about making the field uint16_t ? >> this would make it unsigned in the struct but avoid the problems with >> unsigned int ? > > That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also > change it to use get_ue_golomb() while at it. And please do it for both > rows and columns. Can be in separate patches if you want the rows one to > explicitly address the fuzzing testcase. > > Can you also confirm my suspicion that the checks should be comparing > the values with sps->ctb_height/weight and not with sps->height/weight?
Ok, so yes, it is. Should be changed in a separate patch as well. > >> >> Thanks >> >> [...] >> >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". >> > _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".