Re: [FFmpeg-devel] [PATCH 1/2] avcodec/hevc_ps: Fix integer overflow with num_tile_rows and num_tile_columns
On Wed, Jun 26, 2019 at 10:29:05AM -0300, James Almer wrote:
> On 6/26/2019 9:41 AM, Michael Niedermayer wrote:
> > On Tue, Jun 25, 2019 at 10:30:45AM -0300, James Almer wrote:
> >> On 6/25/2019 5:55 AM, 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
> >>> ---
> >>> libavcodec/hevc_ps.c | 23 +--
> >>> libavcodec/hevc_ps.h | 4 ++--
> >>> 2 files changed, 15 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>> index 80df417e4f..07d220a5c8 100644
> >>> --- a/libavcodec/hevc_ps.c
> >>> +++ b/libavcodec/hevc_ps.c
> >>> @@ -1584,22 +1584,25 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb,
> >>> AVCodecContext *avctx,
> >>> pps->entropy_coding_sync_enabled_flag = get_bits1(gb);
> >>>
> >>> if (pps->tiles_enabled_flag) {
> >>> -pps->num_tile_columns = get_ue_golomb_long(gb) + 1;
> >>> -pps->num_tile_rows= get_ue_golomb_long(gb) + 1;
> >>> -if (pps->num_tile_columns <= 0 ||
> >>> -pps->num_tile_columns >= sps->width) {
> >>> +int num_tile_columns_minus1 = get_ue_golomb(gb);
> >>> +int num_tile_rows_minus1= get_ue_golomb(gb);
> >>> +
> >>> +if (num_tile_columns_minus1 < 0 ||
> >>
> >> num_tile_columns_minus1 can never be < 0 for an int now that you're
> >> using get_ue_golomb(gb). It returns values from 0 to 8190.
> >> Add an av_assert0, at most. A value < 0 would mean there's a huge bug in
> >> golomb.h, and not invalid bitstream data.
> >>
> >> And since you got rid of the "- 1" calculation below, which was your
> >> original concern, you could also just make this unsigned. There's really
> >> no need for an int at all.
> >
> > If you look at get_ue_golomb() in golomb.h
> > static inline int get_ue_golomb(GetBitContext *gb)
> > ...
> > there is a case that does:
> > return AVERROR_INVALIDDATA;
> >
> > That is a negative value an should be checked for before
> > truncating to uint8_t. There is no gurantee that error codes
> > when cast to uint8_t would not map to valid values.
>
> I had not seen that. The doxy for the function mentioned the valid range
> it can return, but made no mention of the chance for an AVERROR* value.
>
> int is ok and the patch should be good, then. Sorry for the noise.
>
> Any opinion about the struct field size? Is uint8_t enough? Your
> original suggestion of uint16_t may be a safer bet, as i mentioned.
> Looking at CBS Mark limited these fields to the max allowed value level
> 6.2 allows for them (20 and 22), but we're not doing that here, so
> num_tile_[cols,rows}_minus1 could in theory and in some extreme cases go
> beyond 255.
ok, will apply with uint16_t
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No snowflake in an avalanche ever feels responsible. -- Voltaire
signature.asc
Description: PGP signature
___
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".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/hevc_ps: Fix integer overflow with num_tile_rows and num_tile_columns
On 6/26/2019 9:41 AM, Michael Niedermayer wrote:
> On Tue, Jun 25, 2019 at 10:30:45AM -0300, James Almer wrote:
>> On 6/25/2019 5:55 AM, 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
>>> ---
>>> libavcodec/hevc_ps.c | 23 +--
>>> libavcodec/hevc_ps.h | 4 ++--
>>> 2 files changed, 15 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
>>> index 80df417e4f..07d220a5c8 100644
>>> --- a/libavcodec/hevc_ps.c
>>> +++ b/libavcodec/hevc_ps.c
>>> @@ -1584,22 +1584,25 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb,
>>> AVCodecContext *avctx,
>>> pps->entropy_coding_sync_enabled_flag = get_bits1(gb);
>>>
>>> if (pps->tiles_enabled_flag) {
>>> -pps->num_tile_columns = get_ue_golomb_long(gb) + 1;
>>> -pps->num_tile_rows= get_ue_golomb_long(gb) + 1;
>>> -if (pps->num_tile_columns <= 0 ||
>>> -pps->num_tile_columns >= sps->width) {
>>> +int num_tile_columns_minus1 = get_ue_golomb(gb);
>>> +int num_tile_rows_minus1= get_ue_golomb(gb);
>>> +
>>> +if (num_tile_columns_minus1 < 0 ||
>>
>> num_tile_columns_minus1 can never be < 0 for an int now that you're
>> using get_ue_golomb(gb). It returns values from 0 to 8190.
>> Add an av_assert0, at most. A value < 0 would mean there's a huge bug in
>> golomb.h, and not invalid bitstream data.
>>
>> And since you got rid of the "- 1" calculation below, which was your
>> original concern, you could also just make this unsigned. There's really
>> no need for an int at all.
>
> If you look at get_ue_golomb() in golomb.h
> static inline int get_ue_golomb(GetBitContext *gb)
> ...
> there is a case that does:
> return AVERROR_INVALIDDATA;
>
> That is a negative value an should be checked for before
> truncating to uint8_t. There is no gurantee that error codes
> when cast to uint8_t would not map to valid values.
I had not seen that. The doxy for the function mentioned the valid range
it can return, but made no mention of the chance for an AVERROR* value.
int is ok and the patch should be good, then. Sorry for the noise.
Any opinion about the struct field size? Is uint8_t enough? Your
original suggestion of uint16_t may be a safer bet, as i mentioned.
Looking at CBS Mark limited these fields to the max allowed value level
6.2 allows for them (20 and 22), but we're not doing that here, so
num_tile_[cols,rows}_minus1 could in theory and in some extreme cases go
beyond 255.
>
> also i belive that returning an error in cases that are
> outside 0 to 8190 is the correct thing to do, so the
> implementation which returns this appears to do the correct
> thing to me.
>
> 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-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".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/hevc_ps: Fix integer overflow with num_tile_rows and num_tile_columns
On Tue, Jun 25, 2019 at 10:30:45AM -0300, James Almer wrote:
> On 6/25/2019 5:55 AM, 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
> > ---
> > libavcodec/hevc_ps.c | 23 +--
> > libavcodec/hevc_ps.h | 4 ++--
> > 2 files changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> > index 80df417e4f..07d220a5c8 100644
> > --- a/libavcodec/hevc_ps.c
> > +++ b/libavcodec/hevc_ps.c
> > @@ -1584,22 +1584,25 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb,
> > AVCodecContext *avctx,
> > pps->entropy_coding_sync_enabled_flag = get_bits1(gb);
> >
> > if (pps->tiles_enabled_flag) {
> > -pps->num_tile_columns = get_ue_golomb_long(gb) + 1;
> > -pps->num_tile_rows= get_ue_golomb_long(gb) + 1;
> > -if (pps->num_tile_columns <= 0 ||
> > -pps->num_tile_columns >= sps->width) {
> > +int num_tile_columns_minus1 = get_ue_golomb(gb);
> > +int num_tile_rows_minus1= get_ue_golomb(gb);
> > +
> > +if (num_tile_columns_minus1 < 0 ||
>
> num_tile_columns_minus1 can never be < 0 for an int now that you're
> using get_ue_golomb(gb). It returns values from 0 to 8190.
> Add an av_assert0, at most. A value < 0 would mean there's a huge bug in
> golomb.h, and not invalid bitstream data.
>
> And since you got rid of the "- 1" calculation below, which was your
> original concern, you could also just make this unsigned. There's really
> no need for an int at all.
If you look at get_ue_golomb() in golomb.h
static inline int get_ue_golomb(GetBitContext *gb)
...
there is a case that does:
return AVERROR_INVALIDDATA;
That is a negative value an should be checked for before
truncating to uint8_t. There is no gurantee that error codes
when cast to uint8_t would not map to valid values.
also i belive that returning an error in cases that are
outside 0 to 8190 is the correct thing to do, so the
implementation which returns this appears to do the correct
thing to me.
Thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The smallest minority on earth is the individual. Those who deny
individual rights cannot claim to be defenders of minorities. - Ayn Rand
signature.asc
Description: PGP signature
___
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".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/hevc_ps: Fix integer overflow with num_tile_rows and num_tile_columns
On 6/25/2019 10:30 AM, James Almer wrote:
> On 6/25/2019 5:55 AM, Michael Niedermayer wrote:
>> +num_tile_columns_minus1 >= sps->width - 1) {
>
> Should be sps->ctb_width
>
> From 7.4.3.3.1:
>
> "num_tile_columns_minus1 plus 1 specifies the number of tile columns
> partitioning the picture. num_tile_columns_minus1 shall be in the range
> of 0 to PicWidthInCtbsY − 1, inclusive. When not present, the value of
> num_tile_columns_minus1 is inferred to be equal to 0."
Ugh, sorry, only now i looked at your second patch. You can ignore this part
Rest of my review still stands.
___
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".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/hevc_ps: Fix integer overflow with num_tile_rows and num_tile_columns
On 6/25/2019 5:55 AM, 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
> ---
> libavcodec/hevc_ps.c | 23 +--
> libavcodec/hevc_ps.h | 4 ++--
> 2 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index 80df417e4f..07d220a5c8 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -1584,22 +1584,25 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb,
> AVCodecContext *avctx,
> pps->entropy_coding_sync_enabled_flag = get_bits1(gb);
>
> if (pps->tiles_enabled_flag) {
> -pps->num_tile_columns = get_ue_golomb_long(gb) + 1;
> -pps->num_tile_rows= get_ue_golomb_long(gb) + 1;
> -if (pps->num_tile_columns <= 0 ||
> -pps->num_tile_columns >= sps->width) {
> +int num_tile_columns_minus1 = get_ue_golomb(gb);
> +int num_tile_rows_minus1= get_ue_golomb(gb);
> +
> +if (num_tile_columns_minus1 < 0 ||
num_tile_columns_minus1 can never be < 0 for an int now that you're
using get_ue_golomb(gb). It returns values from 0 to 8190.
Add an av_assert0, at most. A value < 0 would mean there's a huge bug in
golomb.h, and not invalid bitstream data.
And since you got rid of the "- 1" calculation below, which was your
original concern, you could also just make this unsigned. There's really
no need for an int at all.
> +num_tile_columns_minus1 >= sps->width - 1) {
Should be sps->ctb_width
From 7.4.3.3.1:
"num_tile_columns_minus1 plus 1 specifies the number of tile columns
partitioning the picture. num_tile_columns_minus1 shall be in the range
of 0 to PicWidthInCtbsY − 1, inclusive. When not present, the value of
num_tile_columns_minus1 is inferred to be equal to 0."
> av_log(avctx, AV_LOG_ERROR, "num_tile_columns_minus1 out of
> range: %d\n",
> - pps->num_tile_columns - 1);
> -ret = AVERROR_INVALIDDATA;
> + num_tile_columns_minus1);
> +ret = num_tile_columns_minus1 < 0 ? num_tile_columns_minus1 :
> AVERROR_INVALIDDATA;
Again, can't be < 0.
> goto err;
> }
> -if (pps->num_tile_rows <= 0 ||
> -pps->num_tile_rows >= sps->height) {
> +if (num_tile_rows_minus1 < 0 ||
> +num_tile_rows_minus1 >= sps->height - 1) {
Similarly, sps->ctb_height
"num_tile_rows_minus1 plus 1 specifies the number of tile rows
partitioning the picture. num_tile_rows_minus1 shall be in the range of
0 to PicHeightInCtbsY − 1, inclusive. When not present, the value of
num_tile_rows_minus1 is inferred to be equal to 0."
> av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range:
> %d\n",
> - pps->num_tile_rows - 1);
> -ret = AVERROR_INVALIDDATA;
> + num_tile_rows_minus1);
> +ret = num_tile_rows_minus1 < 0 ? num_tile_rows_minus1 :
> AVERROR_INVALIDDATA;
> goto err;
> }
> +pps->num_tile_columns = num_tile_columns_minus1 + 1;
> +pps->num_tile_rows= num_tile_rows_minus1+ 1;
>
> pps->column_width = av_malloc_array(pps->num_tile_columns,
> sizeof(*pps->column_width));
> pps->row_height = av_malloc_array(pps->num_tile_rows,
> sizeof(*pps->row_height));
> diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
> index bbaa9205ef..44695f09f3 100644
> --- a/libavcodec/hevc_ps.h
> +++ b/libavcodec/hevc_ps.h
> @@ -347,8 +347,8 @@ typedef struct HEVCPPS {
> uint8_t tiles_enabled_flag;
> uint8_t entropy_coding_sync_enabled_flag;
>
> -int num_tile_columns; ///< num_tile_columns_minus1 + 1
> -int num_tile_rows; ///< num_tile_rows_minus1 + 1
> +uint8_t num_tile_columns; ///< num_tile_columns_minus1 + 1
> +uint8_t num_tile_rows; ///< num_tile_rows_minus1 + 1
I'd like Mark to chime in if possible, seeing he used uint8_t for these
in cbs, because i'm not sure just how big sps->ctb_{width,height} can be
in some extreme cases (4k or 8k video with a lot of tiles). uint16_t
like you originally suggested may be a safer bet in those cases after all.
LGTM with the above fixed.
> uint8_t uniform_spacing_flag;
> uint8_t loop_filter_across_tiles_enabled_flag;
>
>
___
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] [PATCH 1/2] avcodec/hevc_ps: Fix integer overflow with num_tile_rows and num_tile_columns
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
---
libavcodec/hevc_ps.c | 23 +--
libavcodec/hevc_ps.h | 4 ++--
2 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
index 80df417e4f..07d220a5c8 100644
--- a/libavcodec/hevc_ps.c
+++ b/libavcodec/hevc_ps.c
@@ -1584,22 +1584,25 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb,
AVCodecContext *avctx,
pps->entropy_coding_sync_enabled_flag = get_bits1(gb);
if (pps->tiles_enabled_flag) {
-pps->num_tile_columns = get_ue_golomb_long(gb) + 1;
-pps->num_tile_rows= get_ue_golomb_long(gb) + 1;
-if (pps->num_tile_columns <= 0 ||
-pps->num_tile_columns >= sps->width) {
+int num_tile_columns_minus1 = get_ue_golomb(gb);
+int num_tile_rows_minus1= get_ue_golomb(gb);
+
+if (num_tile_columns_minus1 < 0 ||
+num_tile_columns_minus1 >= sps->width - 1) {
av_log(avctx, AV_LOG_ERROR, "num_tile_columns_minus1 out of range:
%d\n",
- pps->num_tile_columns - 1);
-ret = AVERROR_INVALIDDATA;
+ num_tile_columns_minus1);
+ret = num_tile_columns_minus1 < 0 ? num_tile_columns_minus1 :
AVERROR_INVALIDDATA;
goto err;
}
-if (pps->num_tile_rows <= 0 ||
-pps->num_tile_rows >= sps->height) {
+if (num_tile_rows_minus1 < 0 ||
+num_tile_rows_minus1 >= sps->height - 1) {
av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range:
%d\n",
- pps->num_tile_rows - 1);
-ret = AVERROR_INVALIDDATA;
+ num_tile_rows_minus1);
+ret = num_tile_rows_minus1 < 0 ? num_tile_rows_minus1 :
AVERROR_INVALIDDATA;
goto err;
}
+pps->num_tile_columns = num_tile_columns_minus1 + 1;
+pps->num_tile_rows= num_tile_rows_minus1+ 1;
pps->column_width = av_malloc_array(pps->num_tile_columns,
sizeof(*pps->column_width));
pps->row_height = av_malloc_array(pps->num_tile_rows,
sizeof(*pps->row_height));
diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
index bbaa9205ef..44695f09f3 100644
--- a/libavcodec/hevc_ps.h
+++ b/libavcodec/hevc_ps.h
@@ -347,8 +347,8 @@ typedef struct HEVCPPS {
uint8_t tiles_enabled_flag;
uint8_t entropy_coding_sync_enabled_flag;
-int num_tile_columns; ///< num_tile_columns_minus1 + 1
-int num_tile_rows; ///< num_tile_rows_minus1 + 1
+uint8_t num_tile_columns; ///< num_tile_columns_minus1 + 1
+uint8_t num_tile_rows; ///< num_tile_rows_minus1 + 1
uint8_t uniform_spacing_flag;
uint8_t loop_filter_across_tiles_enabled_flag;
--
2.22.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-requ...@ffmpeg.org with subject "unsubscribe".
