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. > > [...] > > > _______________________________________________ > 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".
