| Message ID | 20190613183236.19353-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Michael Niedermayer > Sent: Friday, June 14, 2019 2:33 AM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: [FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer overflow > with num_tile_rows > > 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@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); I think the machine code generated here should be the same, right? So you just tell fuzzer "I am doing subtraction between unsigned numbers", to make it happy? > ret = AVERROR_INVALIDDATA; > goto err; > } > -- > 2.21.0 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sat, Jun 15, 2019 at 03:07:13PM +0000, Song, Ruiling wrote: > > -----Original Message----- > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > > Of Michael Niedermayer > > Sent: Friday, June 14, 2019 2:33 AM > > To: FFmpeg development discussions and patches <ffmpeg- > > devel@ffmpeg.org> > > Subject: [FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer overflow > > with num_tile_rows > > > > 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@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); > I think the machine code generated here should be the same, right? > So you just tell fuzzer "I am doing subtraction between unsigned numbers", to make it happy? its likely the same machine code, yes. A compiler might produce different code that break in case of the overflow though ... thx [...]
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Michael Niedermayer > Sent: Sunday, June 16, 2019 6:07 AM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer > overflow with num_tile_rows > > On Sat, Jun 15, 2019 at 03:07:13PM +0000, Song, Ruiling wrote: > > > -----Original Message----- > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > Behalf > > > Of Michael Niedermayer > > > Sent: Friday, June 14, 2019 2:33 AM > > > To: FFmpeg development discussions and patches <ffmpeg- > > > devel@ffmpeg.org> > > > Subject: [FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer > overflow > > > with num_tile_rows > > > > > > 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@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); > > I think the machine code generated here should be the same, right? > > So you just tell fuzzer "I am doing subtraction between unsigned numbers", > to make it happy? > > its likely the same machine code, yes. A compiler might produce different > code > that break in case of the overflow though ... Ok, it seems num_tile_columns also need such kind of change. > > thx > > [...] > -- > Michael GnuPG fingerprint: > 9FF2128B147EF6730BADF133611EC787040B0FAB > > When the tyrant has disposed of foreign enemies by conquest or treaty, and > there is nothing more to fear from them, then he is always stirring up > some war or other, in order that the people may require a leader. -- Plato
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@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. The minimum allowed value for num_tile_{rows,cols}_minus1 is 0. > ret = AVERROR_INVALIDDATA; > goto err; > } >
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@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". 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. > The > minimum allowed value for num_tile_{rows,cols}_minus1 is 0. yes [...]
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@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-request@ffmpeg.org with subject "unsubscribe". >
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@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 [...]
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@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. [...]
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@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-request@ffmpeg.org with subject "unsubscribe". >
On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote: > 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@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. what would be your feeling/oppinon about making the field uint16_t ? this would make it unsigned in the struct but avoid the problems with unsigned int ? Thanks [...]
On 6/19/2019 3:13 PM, Michael Niedermayer wrote: > On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote: >> 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@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. > > what would be your feeling/oppinon about making the field uint16_t ? > this would make it unsigned in the struct but avoid the problems with > unsigned int ? That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also change it to use get_ue_golomb() while at it. And please do it for both rows and columns. Can be in separate patches if you want the rows one to explicitly address the fuzzing testcase. Can you also confirm my suspicion that the checks should be comparing the values with sps->ctb_height/weight and not with sps->height/weight? > > Thanks > > [...] > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 6/19/2019 3:59 PM, James Almer wrote: > On 6/19/2019 3:13 PM, Michael Niedermayer wrote: >> On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote: >>> 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@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. >> >> what would be your feeling/oppinon about making the field uint16_t ? >> this would make it unsigned in the struct but avoid the problems with >> unsigned int ? > > That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also > change it to use get_ue_golomb() while at it. And please do it for both > rows and columns. Can be in separate patches if you want the rows one to > explicitly address the fuzzing testcase. > > Can you also confirm my suspicion that the checks should be comparing > the values with sps->ctb_height/weight and not with sps->height/weight? Ok, so yes, it is. Should be changed in a separate patch as well. > >> >> Thanks >> >> [...] >> >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >> >
On Wed, Jun 19, 2019 at 03:59:47PM -0300, James Almer wrote: > On 6/19/2019 3:13 PM, Michael Niedermayer wrote: > > On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote: > >> 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@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. > > > > what would be your feeling/oppinon about making the field uint16_t ? > > this would make it unsigned in the struct but avoid the problems with > > unsigned int ? > > That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also > change it to use get_ue_golomb() while at it. And please do it for both > rows and columns. Can be in separate patches if you want the rows one to > explicitly address the fuzzing testcase. will do but this requires intermediate int, ill post a new patch get_ue_golomb() can return negative error codes. storing this in an 8bit variable would truncate the error code and could fold it into a valid value. [...]
On Thu, Jun 20, 2019 at 01:04:45AM -0300, James Almer wrote: > On 6/19/2019 3:59 PM, James Almer wrote: > > On 6/19/2019 3:13 PM, Michael Niedermayer wrote: > >> On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote: > >>> 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@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. > >> > >> what would be your feeling/oppinon about making the field uint16_t ? > >> this would make it unsigned in the struct but avoid the problems with > >> unsigned int ? > > > > That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also > > change it to use get_ue_golomb() while at it. And please do it for both > > rows and columns. Can be in separate patches if you want the rows one to > > explicitly address the fuzzing testcase. > > > > Can you also confirm my suspicion that the checks should be comparing > > the values with sps->ctb_height/weight and not with sps->height/weight? > > Ok, so yes, it is. Should be changed in a separate patch as well. will post it together with the other updated patch thanks [...]
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); ret = AVERROR_INVALIDDATA; goto err; }
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@niedermayer.cc> --- libavcodec/hevc_ps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)