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

[James Almer]

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. 
> 
> I dont think "unsigned int" is wise to use as type here, the semantics 
> of unsigned ints are unexpected to many
> especially making random subsets of "normal" fields unsigned will make
> the codebase slowly "interresting".

If you make the actual unsigned values as per the spec be unsigned, it
will not be a random subset of values... And i see plenty of unsigned
and uint*_t fields in hevc_ps.h. These for some reason were given the
wrong type.

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

By making these two fields unsigned you can remove the
"pps->num_tile_{rows,cols} <= 0" checks, leaving only the
"pps->num_tile_{rows,cols} >= sps->{height,width}" checks. Just add an
av_assert0(pps->num_tile_{rows,cols} > 0) before them and the av_log()
calls, which is also before the loop you quoted, if you want to be sure
no change in the future introduces issues.


> 
> 
>> The
>> minimum allowed value for num_tile_{rows,cols}_minus1 is 0.
> 
> yes
> 
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>