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