[FFmpeg-devel] [PATCH] H.264 timestamps in h264_parser - complete set

[Ivan Schreter]

Michael Niedermayer wrote:
> On Wed, Feb 18, 2009 at 01:41:27PM +0100, Ivan Schreter wrote:
>   
>> Michael Niedermayer wrote:
>>     
>>> On Wed, Feb 18, 2009 at 12:33:57PM +0100, Ivan Schreter wrote:
>>>   [...]
>>>   
>>>       
>>>>> [...]
>>>>>
>>>>>         
>>>>>           
>>>>>> @@ -6859,6 +6860,37 @@
>>>>>>      return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int decode_buffering_period(H264Context *h){
>>>>>> +    MpegEncContext * const s = &h->s;
>>>>>> +    int sps_id;
>>>>>> +    int sched_sel_idx;
>>>>>> +    SPS *sps;
>>>>>> +
>>>>>> +    sps_id = get_ue_golomb_31(&s->gb);
>>>>>>             
>>>>>>             
>>>>> this is missing a validity check (<32 i suspect but didnt check)
>>>>> also as this would have been possibly exploitable, please be carefull 
>>>>> not to
>>>>> miss such checks
>>>>>         
>>>>>           
>>>> According to docs of get_ue_golomb_31(), it can only return value in 
>>>> range 0..31. SPS ID can be in range 0..31 as well, so no check required. 
>>>> However, looking at get_ue_golomb_31() code, the lookup table contains 
>>>> also return value of 32! So either doc is wrong or the lookup table is 
>>>> wrong. I've added the check to be on the safe side.
>>>>     
>>>>         
>>> fixed doc, and you need to make the check unsigned
>>>   
>>>       
>> Why? The function returns int in range 0..32. It takes 9 bits from current 
>> word and uses this as an index into lookup array containing values 0..32. 
>> So it cannot possibly return negative value.
>>     
>
> the function returns an undefined value if the bitstream is not storing 0..31
>   
Mhm... The current implementation cannot return negative value, but 
nevertheless, you are right in the sense that future (different) 
implementation could.

So I simply changed the variable to unsigned int, then a negative value 
will be mapped to very high positive value, so condition >31 will filter 
it out (instead of adding yet another condition).

Attached is the corrected patch. I suppose, it's OK now.

Regards,

Ivan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: h264_timing_6_buffering_period.patch
Type: text/x-patch
Size: 3498 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090218/536e4d25/attachment.bin>