[FFmpeg-devel] [PATCH] MLP/TrueHD decoder
Ian Caulfield
ian.caulfield
Mon Jan 7 17:39:11 CET 2008
Hi,
New version attached
On Jan 6, 2008 10:17 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> On Tue, Dec 18, 2007 at 11:38:09AM +0000, Ian Caulfield wrote:
> >
> > +typedef struct MLPDecodeContext {
> > + AVCodecContext *avctx;
> > +
> > + uint8_t params_valid;
> > + uint8_t max_decoded_substream;
> > + int access_unit_size;
> > + int access_unit_size_pow2;
> > + uint8_t restart_seen[MAX_SUBSTREAMS];
> > +
> > + uint8_t num_substreams;
> > +
> > + //@{
> > + /** Restart header data */
>
> > + uint16_t restart_sync[MAX_SUBSTREAMS];
>
> shouldnt this rather be called sync_word ?
I've changed it to restart_sync_word (to distinguish it from the major
sync word if that ever needs to be stored in the context)
> > +} MLPDecodeContext;
>
> some comments explaing what each var is, would be nice
I've put comments for most of the variables (I thought some were
sufficiently obvious to not require explanation - will fill those in
too if you like).
> > +
> > + if (codebook > 0)
> > + result = get_vlc2(gbp, huff_vlc[codebook-1].table,
> > + VLC_BITS, (9 + VLC_BITS - 1) / VLC_BITS) - 7;
>
> the -7 can be merged into sign_huff_offset
Done
> > +static void generate_noise_2(MLPDecodeContext *m, unsigned int substr)
> > +{
> > + unsigned int i;
> > + uint32_t seed = m->noisegen_seed[substr];
> > +
> > + for (i = 0; i < m->access_unit_size_pow2; i++) {
>
> > + uint8_t tmp = seed >> 15;
>
> please name this seed_shr15
Done
> > + if (*data_size < m->avctx->channels * m->access_unit_size
> > + * (m->avctx->sample_fmt == SAMPLE_FMT_S32 ? 4 : 2))
> > + return -1;
>
> its nice that you check that, it would be nicer i you did after setting
> the variables, like that it is just exploitable
Moved to output_data_internal
>
> [...]
> > + skip_bits(&gb, (-get_bits_count(&gb)) & 15);
> > + if (show_bits_long(&gb, 32) == 0xd234d234 ||
> > + show_bits_long(&gb, 18) == 0xd234e) {
> > + skip_bits(&gb, 18);
> > + av_log(m->avctx, AV_LOG_INFO, "End of stream indicated\n");
> > + if (get_bits1(&gb))
> > + m->blockpos[substr] -= get_bits(&gb, 13);
> > + else
> > + skip_bits(&gb, 13);
>
> exploitable, blockpos overflows and is later used as array limit
> (for exmple in rematrix_channels)
> iam starting to be scared by your patches, can you _please_ take this more
> serious, and check the variables used to decide where to write data to?
> all of them, double check your code, if i find another such issue i will not
> review this patch again and permanently reject it!
Sorry about that - I was very careful and checked all loops to make
sure the bounds were checked properly. I was then given a new sample
that decoded incorrectly and added the above code to handle it without
thinking fully. I'll avoid making any non-review changes until
reviewing is over, and try to be more paranoid in general. Hopefully
there aren't any holes left...
Ian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mlpdec.patch
Type: text/x-diff
Size: 41044 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080107/4535117a/attachment.patch>
More information about the ffmpeg-devel
mailing list