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
signature.asc
Description: Digital signature
_______________________________________________ FFmpeg-soc mailing list [email protected] https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-soc
