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

[Ramiro Polla]

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 on the code anyways. coded values which pass the checksum are trusted.

> about other codecs
> mpeg & h26* are designed so 23 zero bits can never occur in a valid bitstream
> that way the 64 zero bit at the end are guranteed to trigger some kind of
> error.

Oh, so they're guaranteed to be zeroes? I'll have to take another look 
at the code with this new information in mind. MLP doesn't seem to be so 
well designed anyways.

> I definitly do not want the code to be salted with input buffer checks in
> every second line!

Too much salt is bad for our health =)

Ramiro Polla
-------------- next part --------------
A non-text attachment was scrubbed...
Name: revert_1.diff
Type: text/x-diff
Size: 704 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080627/29c7b160/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: revert_2.diff
Type: text/x-diff
Size: 1629 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-soc/attachments/20080627/29c7b160/attachment-0001.diff>