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