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

Ian Caulfield ian.caulfield
Mon Dec 3 17:05:28 CET 2007


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.


> [...]
>
> > +    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...

> [...]
> > +    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...

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

Ian




More information about the ffmpeg-devel mailing list