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

[Michael Niedermayer]

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


> 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 think so, though if its security relevant please check it explicitly
(segfault through reads isnt security relevant for us though)


> 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.

MLP seemed to be VERY poorly designed last time i had to review a patch
related to it ...


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

:)


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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-soc/attachments/20080628/0e23dca7/attachment.pgp>