[FFmpeg-devel] [PATCH] MLP/TrueHD decoder

Ian Caulfield ian.caulfield
Tue Dec 4 16:03:47 CET 2007


Hi

Another revision of the patch attached. Hopefully creeping towards
acceptability :)

On Dec 3, 2007 5:34 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Mon, Dec 03, 2007 at 04:05:28PM +0000, Ian Caulfield wrote:
> > On Dec 2, 2007 2:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> >
> > > a memset(0) of the whole context is minimum if you want to keep this design
> > > and want to see it applied to svn, but it would be much better not to set
> > > critical variables to invalid values to begin with or overwrite them
> > > individually with harmless values if they have been filled with something
> > > out of range
> >
> > I'll look at doing this if you'd like.
>
> cleaning up the validity checks is absolutely needed if you want to see this
> in svn, how you implement it is your decission but it must be idiot proof
> that is, code which looks unrelated to a check should be unrelated to the
> check ...

OK, I've gone through the validity checks and made sure they either
check the values before writing to the context or that they zero the
values if they're invalid (except one case checking the filter
denominators are the same - a bug letting that slip through would
sound crap rather than present a security hole).

> > Fair enough, although I would have thought that decoding a downmix
> > with fewer channels would be preferable to just borking in this
> > case...
>
> you can change your code to support more channels ...
> i dont see why you would want to support some max and then do forced
> downmixing to that ...

I've increased the MAX_CHANNELS define to 16, to cover the maximum
value of a 4-bit field.

> so yes you are right, it seems like this case cant overflow
> but its fantastically complicated, such code is simply not fit for svn
> it would take litteraly days to check any single line change anyone
> (who is not the author and doesnt know about these tricks) want to do
> also you will forget about these things a year after you wrote it ...
> and a harmless looking change to any point of the sample_rate path could
> cause access_unit_size_pow2 to pass with a too large value ...
>
>
> >
> > > please check the source of the WHOLE decoder for such buffer overflows!
> > > and add proper checks
> >
> > I'll have a look - would you prefer redundant checks in cases like the above?
>
> id prefer clean code without redundant checks
> if that isnt possibly i prefer redundant checks if its security relevant
> and no redundant checks if its speed critical code

I've checked all the loops in the code, and made sure that the
variables used as limits are checked for validity. No doubt you'll
spot something I've missed though :)

>
> also please always check values for validity soon after you read them for
> example like:
>
> ratebits = get_bits(&gb, 4);
> if((ratebits&7) > 2)
>     return -1;
>
> and support all values which pass these checks

In this case I've left the checks where they were, but checked all the
derived values - the above line is in the parser, which probably
shouldn't fall over just because the decoder won't understand that
part of the metadata - stream copy for example should still work.

Ian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlpdec.patch
Type: text/x-diff
Size: 40601 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071204/75d61316/attachment.patch>



More information about the ffmpeg-devel mailing list