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

Michael Niedermayer michaelni
Mon Dec 3 18:34:42 CET 2007


On Mon, Dec 03, 2007 at 04:05:28PM +0000, Ian Caulfield wrote:
> Hi,
> 
> On Dec 2, 2007 2:28 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Sat, Dec 01, 2007 at 11:49:17PM +0000, Ian Caulfield wrote:
> > > It appears I attached an older version of the patch in my last email,
> > > sorry. I've attached an up-to-date patch (hopefully) addressing
> > > Diego's comments.
> > >
> > > On Dec 1, 2007 10:08 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > On Thu, Nov 29, 2007 at 10:55:56AM +0000, Ian Caulfield wrote:
> > > > > On Nov 12, 2007 1:25 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > >
> > > > > As well as an error being returned, the "have parameters" flag in the
> > > > > context is cleared, causing all packets to be rejected until we get a
> > > > > (valid) sync packet.
> > > >
> > > > there is no variable, constant or otherwise called "have parameters" or have*
> > >
> > > The variable "has_params" is used (previously called in_sync).
> >
> > rather call it params_valid please
> > because if its 0 they are not, also this design is fragile, every future
> > change to the codec would have to be checked that it doesnt use anything
> > from the context if has_params != 1
> 
> Fair enough, although currently is has_params is zero, frames will be
> skipped until read_major_sync sets it again.
> 
> > 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 ...


> 
> 
> > [...]
> >
> > > +    if (m->max_channel[substr] >= MAX_CHANNELS
> > > +        || m->max_matrix_channel[substr] >= MAX_CHANNELS) {
> > > +        if (substr > 0) {
> > > +            av_log(m->avctx, AV_LOG_INFO,
> > > +                   "Substream %d contains more channels than the maximum "
> > > +                   "supported by this decoder (%d). Only substreams up to %d "
> > > +                   "will be decoded. Please provide a sample of this file "
> > > +                   "to the FFmpeg development list.\n",
> > > +                   substr, MAX_CHANNELS, substr - 1);
> > > +            m->max_decoded_substream = substr - 1;
> > > +            m->avctx->channels = m->max_channel[substr - 1] + 1;
> >
> > could you please, remove this fraile hack and just return -1 if the file
> > contains more channels than the decoder supports
> 
> 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 ...


> 
> > [...]
> > > +    for (i = 0; i < m->access_unit_size_pow2; i++) {
> > > +        uint32_t tmp = seed >> 15;
> > > +        m->noise_buffer[i] = noise_table[tmp];
> > > +        seed = (seed << 8) ^ tmp ^ (tmp << 5);
> > > +        seed &= 0x7fffff;
> > > +    }
> >
> > buffer overflow, access_unit_size_pow2 is not checked for validity
> > and the buffer is smaller than the max this variable can be
> 
> access_unit_size_pow2 is set directly depending on the sample rate,
> and the sample rate _is_ checked for validity _before_ the value is
> set. I don't see any way to put an invalid value into the variable
> without corrupting memory...

in mlp_parser.c: ff_mlp_read_major_sync()
    you read 4 bits
        ratebits = get_bits(&gb, 4);
    initalize group1_samplerate of MLPHeaderInfo based on it
        mh->group1_samplerate = mlp_samplerate(ratebits);
            static int mlp_samplerate(int in)
            {
                if (in == 0xF)
                    return 0;

                return (in & 8 ? 44100 : 48000) << (in & 7) ;
            }
    both the above are done on either side of a branch with various other
    things
    next the access_unit_size* from MLPHeaderInfo get initalized based on it
        mh->access_unit_size = 40 << (ratebits & 7);
        mh->access_unit_size_pow2 = 64 << (ratebits & 7);
    after that and many unrelated things ff_mlp_read_major_sync()
    returns 0 (success)

in mlpdec.c: read_major_sync()
    you check mh.group1_samplerate against 0 and MAX_SAMPLERATE before
    copying access_unit_size* from MLPHeaderInfo to MLPDecodeContext

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


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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-devel/attachments/20071203/be211536/attachment.pgp>



More information about the ffmpeg-devel mailing list