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

[Michael Niedermayer]

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

no, but it would require very precisse knowledge of the types used in the
spec though. Iam not sure everyone working on the code has this knowledge


> And i see plenty of unsigned
> and uint*_t fields in hevc_ps.h. These for some reason were given the
> wrong type.

uint8/16 is not unsigned in this sense in C. So these are not a problem
at all. Ill cover uint32 below, but first consider uint8/16 and unsigned

int main(){
    uint8_t u8 = 0;
    unsigned u = 0;

    printf("is u-1 negative: %d, is u8-1 negative: %d\n", u-1 < 0, u8-1 < 0);
}
This produces:
is u-1 negative: 0, is u8-1 negative: 1

the uint8_t type is a signed int in C expressions, unsigned is a unsigned int in C

Now about uint32_t, that you probably dont want to hear about but it should
behave platform dependant. On a system that has 32bit ints its unsigned, on a platform
where int is 64bit it should be signed.
We did not run into this yet because platforms with 64bit int are uncommon.
the world choose to keep int 32bit even on 64bit architectures in general.

But because of that use of uint32_t except for array types is probably simply
a bad idea.

Iam not sure today if i like C, normally i like it ...

Thanks


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190619/6b74c8e0/attachment.sig>