[FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer overflow with num_tile_rows

[James Almer]

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 <michael at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>