Message ID | 20180608221130.12644-4-michael@niedermayer.cc |
---|---|
State | New |
Headers | show |
On 6/8/2018 7:11 PM, Michael Niedermayer wrote: > Fixes: signed integer overflow: 15 + 2147483646 cannot be represented in type 'int' > Fixes: 8381/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_H264_fuzzer-6225533137321984 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/h264_sei.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c > index 9defcb80b9..2f16d95f56 100644 > --- a/libavcodec/h264_sei.c > +++ b/libavcodec/h264_sei.c > @@ -261,10 +261,16 @@ static int decode_unregistered_user_data(H264SEIUnregistered *h, GetBitContext * > return 0; > } > > -static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb) > +static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb, void *logctx) > { > - h->recovery_frame_cnt = get_ue_golomb_long(gb); > + unsigned recovery_frame_cnt = get_ue_golomb_long(gb); > > + if (recovery_frame_cnt > (1<<16)) { Maybe move MAX_LOG2_MAX_FRAME_NUM out of h264_ps.c and into h264_ps.h, then use it here? > + av_log(logctx, AV_LOG_ERROR, "recovery_frame_cnt %d is out of range\n", recovery_frame_cnt); It's unsigned, so %u. Some pedantic compilers (Like djgpp) may complain or downright fail otherwise. > + return AVERROR_INVALIDDATA; > + } > + > + h->recovery_frame_cnt = recovery_frame_cnt; > /* 1b exact_match_flag, > * 1b broken_link_flag, > * 2b changing_slice_group_idc */ > @@ -429,7 +435,7 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, > ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size); > break; > case H264_SEI_TYPE_RECOVERY_POINT: > - ret = decode_recovery_point(&h->recovery_point, gb); > + ret = decode_recovery_point(&h->recovery_point, gb, logctx); > break; > case H264_SEI_TYPE_BUFFERING_PERIOD: > ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx); >
On 6/8/2018 8:12 PM, James Almer wrote: > On 6/8/2018 7:11 PM, Michael Niedermayer wrote: >> Fixes: signed integer overflow: 15 + 2147483646 cannot be represented in type 'int' >> Fixes: 8381/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_H264_fuzzer-6225533137321984 >> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/h264_sei.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c >> index 9defcb80b9..2f16d95f56 100644 >> --- a/libavcodec/h264_sei.c >> +++ b/libavcodec/h264_sei.c >> @@ -261,10 +261,16 @@ static int decode_unregistered_user_data(H264SEIUnregistered *h, GetBitContext * >> return 0; >> } >> >> -static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb) >> +static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb, void *logctx) >> { >> - h->recovery_frame_cnt = get_ue_golomb_long(gb); >> + unsigned recovery_frame_cnt = get_ue_golomb_long(gb); >> >> + if (recovery_frame_cnt > (1<<16)) { > > Maybe move MAX_LOG2_MAX_FRAME_NUM out of h264_ps.c and into h264_ps.h, > then use it here? And it should be "(1 << MAX_LOG2_MAX_FRAME_NUM) - 1", for that matter. Or alternatively use sps->log2_max_frame_num from the active sps instead. > >> + av_log(logctx, AV_LOG_ERROR, "recovery_frame_cnt %d is out of range\n", recovery_frame_cnt); > > It's unsigned, so %u. Some pedantic compilers (Like djgpp) may complain > or downright fail otherwise. > >> + return AVERROR_INVALIDDATA; >> + } >> + >> + h->recovery_frame_cnt = recovery_frame_cnt; >> /* 1b exact_match_flag, >> * 1b broken_link_flag, >> * 2b changing_slice_group_idc */ >> @@ -429,7 +435,7 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, >> ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size); >> break; >> case H264_SEI_TYPE_RECOVERY_POINT: >> - ret = decode_recovery_point(&h->recovery_point, gb); >> + ret = decode_recovery_point(&h->recovery_point, gb, logctx); >> break; >> case H264_SEI_TYPE_BUFFERING_PERIOD: >> ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx); >> >
On 6/8/2018 11:23 PM, James Almer wrote: > On 6/8/2018 8:12 PM, James Almer wrote: >> On 6/8/2018 7:11 PM, Michael Niedermayer wrote: >>> Fixes: signed integer overflow: 15 + 2147483646 cannot be represented in type 'int' >>> Fixes: 8381/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_H264_fuzzer-6225533137321984 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/h264_sei.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c >>> index 9defcb80b9..2f16d95f56 100644 >>> --- a/libavcodec/h264_sei.c >>> +++ b/libavcodec/h264_sei.c >>> @@ -261,10 +261,16 @@ static int decode_unregistered_user_data(H264SEIUnregistered *h, GetBitContext * >>> return 0; >>> } >>> >>> -static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb) >>> +static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb, void *logctx) >>> { >>> - h->recovery_frame_cnt = get_ue_golomb_long(gb); >>> + unsigned recovery_frame_cnt = get_ue_golomb_long(gb); >>> >>> + if (recovery_frame_cnt > (1<<16)) { >> >> Maybe move MAX_LOG2_MAX_FRAME_NUM out of h264_ps.c and into h264_ps.h, >> then use it here? > > And it should be "(1 << MAX_LOG2_MAX_FRAME_NUM) - 1", for that matter. > Or alternatively use sps->log2_max_frame_num from the active sps instead. Or maybe not. Guess this is already handled by h264_slice.c, so probably just use the aforementioned constant. > >> >>> + av_log(logctx, AV_LOG_ERROR, "recovery_frame_cnt %d is out of range\n", recovery_frame_cnt); >> >> It's unsigned, so %u. Some pedantic compilers (Like djgpp) may complain >> or downright fail otherwise. >> >>> + return AVERROR_INVALIDDATA; >>> + } >>> + >>> + h->recovery_frame_cnt = recovery_frame_cnt; >>> /* 1b exact_match_flag, >>> * 1b broken_link_flag, >>> * 2b changing_slice_group_idc */ >>> @@ -429,7 +435,7 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, >>> ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size); >>> break; >>> case H264_SEI_TYPE_RECOVERY_POINT: >>> - ret = decode_recovery_point(&h->recovery_point, gb); >>> + ret = decode_recovery_point(&h->recovery_point, gb, logctx); >>> break; >>> case H264_SEI_TYPE_BUFFERING_PERIOD: >>> ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx); >>> >> >
On Fri, Jun 08, 2018 at 11:34:02PM -0300, James Almer wrote: > On 6/8/2018 11:23 PM, James Almer wrote: > > On 6/8/2018 8:12 PM, James Almer wrote: > >> On 6/8/2018 7:11 PM, Michael Niedermayer wrote: > >>> Fixes: signed integer overflow: 15 + 2147483646 cannot be represented in type 'int' > >>> Fixes: 8381/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_H264_fuzzer-6225533137321984 > >>> > >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavcodec/h264_sei.c | 12 +++++++++--- > >>> 1 file changed, 9 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c > >>> index 9defcb80b9..2f16d95f56 100644 > >>> --- a/libavcodec/h264_sei.c > >>> +++ b/libavcodec/h264_sei.c > >>> @@ -261,10 +261,16 @@ static int decode_unregistered_user_data(H264SEIUnregistered *h, GetBitContext * > >>> return 0; > >>> } > >>> > >>> -static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb) > >>> +static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb, void *logctx) > >>> { > >>> - h->recovery_frame_cnt = get_ue_golomb_long(gb); > >>> + unsigned recovery_frame_cnt = get_ue_golomb_long(gb); > >>> > >>> + if (recovery_frame_cnt > (1<<16)) { > >> > >> Maybe move MAX_LOG2_MAX_FRAME_NUM out of h264_ps.c and into h264_ps.h, > >> then use it here? > > > > And it should be "(1 << MAX_LOG2_MAX_FRAME_NUM) - 1", for that matter. > > > Or alternatively use sps->log2_max_frame_num from the active sps instead. > > Or maybe not. Guess this is already handled by h264_slice.c, so probably > just use the aforementioned constant. will apply with these changes after basic testing thanks [...]
diff --git a/libavcodec/h264_sei.c b/libavcodec/h264_sei.c index 9defcb80b9..2f16d95f56 100644 --- a/libavcodec/h264_sei.c +++ b/libavcodec/h264_sei.c @@ -261,10 +261,16 @@ static int decode_unregistered_user_data(H264SEIUnregistered *h, GetBitContext * return 0; } -static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb) +static int decode_recovery_point(H264SEIRecoveryPoint *h, GetBitContext *gb, void *logctx) { - h->recovery_frame_cnt = get_ue_golomb_long(gb); + unsigned recovery_frame_cnt = get_ue_golomb_long(gb); + if (recovery_frame_cnt > (1<<16)) { + av_log(logctx, AV_LOG_ERROR, "recovery_frame_cnt %d is out of range\n", recovery_frame_cnt); + return AVERROR_INVALIDDATA; + } + + h->recovery_frame_cnt = recovery_frame_cnt; /* 1b exact_match_flag, * 1b broken_link_flag, * 2b changing_slice_group_idc */ @@ -429,7 +435,7 @@ int ff_h264_sei_decode(H264SEIContext *h, GetBitContext *gb, ret = decode_unregistered_user_data(&h->unregistered, gb, logctx, size); break; case H264_SEI_TYPE_RECOVERY_POINT: - ret = decode_recovery_point(&h->recovery_point, gb); + ret = decode_recovery_point(&h->recovery_point, gb, logctx); break; case H264_SEI_TYPE_BUFFERING_PERIOD: ret = decode_buffering_period(&h->buffering_period, gb, ps, logctx);
Fixes: signed integer overflow: 15 + 2147483646 cannot be represented in type 'int' Fixes: 8381/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_H264_fuzzer-6225533137321984 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/h264_sei.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)