[FFmpeg-soc] [soc]: r2578 - mlp/mlpdec.c

[Ramiro Polla]

Michael Niedermayer wrote:
> On Fri, Jun 27, 2008 at 10:33:54PM +0100, Ramiro Polla wrote:
>> Hi,
>>
>> Michael Niedermayer wrote:
>>> On Thu, Jun 26, 2008 at 07:26:20PM +0100, Ramiro Polla wrote:
>>>> Hello,
>>>>
>>>> ramiro wrote:
>>>>> Author: ramiro
>>>>> Date: Thu Jun 26 19:33:28 2008
>>>>> New Revision: 2578
>>>>>
>>>>> Log:
>>>>> More checks to see if there is enough data.
>>>>>
>>>>> Modified:
>>>>>    mlp/mlpdec.c
>>>>>
>>>>> Modified: mlp/mlpdec.c
>>>>> ==============================================================================
>>>>> --- mlp/mlpdec.c	(original)
>>>>> +++ mlp/mlpdec.c	Thu Jun 26 19:33:28 2008
>>>>> @@ -1009,6 +1009,9 @@ static int read_access_unit(AVCodecConte
>>>>>      for (substr = 0; substr < m->num_substreams; substr++) {
>>>>>          int extraword_present, checkdata_present, end;
>>>>>  +        if (bytes_left < 2)
>>>>> +            return -1;
>>>>> +
>>>>>          extraword_present = get_bits1(&gb);
>>>>>          skip_bits1(&gb);
>>>>>          checkdata_present = get_bits1(&gb);
>>>>> @@ -1022,6 +1025,8 @@ static int read_access_unit(AVCodecConte
>>>>>          bytes_left -= 2;
>>>>>           if (extraword_present) {
>>>>> +            if (bytes_left < 2)
>>>>> +                return -1;
>>>>>              skip_bits(&gb, 16);
>>>>>              parity_bits ^= *buf++;
>>>>>              parity_bits ^= *buf++;
>>>> This seems like a long and painful road... While we're still reading 
>>>> headers with easily calculated size, this isn't too hard, and there won't 
>>>> be much overhead in the checks. But on more complicated headers and 
>>>> specially the VLC values it's not easy to see if we still have enough 
>>>> data.
>>>>
>>>> Lots of other codecs I've looked at seem to just trust init_get_bits() 
>>>> with the remaining bytes.
>>>>
>>>> What's the best practice here? Should the header checksum && coded 
>>>> lengths be enough to validate the input size?
>>> All _writes_ must be checked, the reads are not critical.
>>> also dont forget FF_INPUT_BUFFER_PADDING_SIZE
>> Ok. So is it ok to apply these patches?
>> revert_1 just revert the last commit of extra checks.
>> revert_2 removes keeping track of bytes left. the value is never used later 
> 
> ok

Ok, I think the code is ready for another review now.

Ramiro Polla