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

[Michael Niedermayer]

On Wed, Feb 18, 2009 at 03:29:01PM +0100, Ivan Schreter wrote:
> 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.

yes, ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090218/1954d710/attachment.pgp>