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 <mich...@niedermayer.cc>
> ---
> 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".
