diff mbox

[FFmpeg-devel,4/4] avcodec/h264_slice: Fix overflow in recovery_frame computation

Message ID 20180608221130.12644-4-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer June 8, 2018, 10:11 p.m. UTC 
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(-)

Comments

James Almer June 8, 2018, 11:12 p.m. UTC | #1 
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);
>
James Almer June 9, 2018, 2:23 a.m. UTC | #2 
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);
>>
>
James Almer June 9, 2018, 2:34 a.m. UTC | #3 
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);
>>>
>>
>
Michael Niedermayer June 10, 2018, 3:06 p.m. UTC | #4 
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 mbox

Patch

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);