[FFmpeg-devel] [PATCH] MLP/TrueHD decoder
Michael Niedermayer
michaelni
Tue Jan 8 01:25:46 CET 2008
On Mon, Jan 07, 2008 at 04:39:11PM +0000, Ian Caulfield wrote:
> 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...
[...]
> +/** Data table used for TrueHD noise generation function */
> +
> +static int8_t noise_table[256] = {
static const
[...]
> +/** Write the audio data into the output buffer.
> + */
> +
> +static int output_data_internal(MLPDecodeContext *m, unsigned int substr,
> + uint8_t *data, unsigned int *data_size, int is32)
> +{
> + unsigned int i, ch = 0;
> + int32_t *data_32 = (int32_t*) data;
> + int16_t *data_16 = (int16_t*) data;
> +
> + if (*data_size < m->max_channel[substr] * m->blockpos[substr]
> + * (is32 ? 4 : 2))
> + return -1;
> +
> + for (i = 0; i < m->blockpos[substr]; i++) {
> + for (ch = 0; ch <= m->max_channel[substr]; ch++) {
ideg ...
lets try this differently
please actually TEST ALL security checks in the decoder, that is set variables
manually to the max which passes the tests
ahh and i want to see the test code, attach it!
and
go over the whole code and check that there are no other max vs. max+1 errors
and
run -absf noise -acodec copy over a long (>1hour) stream and decode that
afterwards if it crashes or ends in an infinite loop, fix your code and try
again
[...]
> +/** XOR together all the bytes of a buffer.
> + * Does this belong in dspcontext?
> + */
> +
> +static uint8_t calculate_parity(const uint8_t *buf, unsigned int buf_size)
> +{
> + uint32_t scratch = 0;
> + const uint8_t *buf_end = buf + buf_size;
> +
> + for (; buf < buf_end - 3; buf += 4)
> + scratch ^= *((const uint32_t*)buf);
> +
> + scratch ^= scratch >> 16;
> + scratch ^= scratch >> 8;
> + scratch &= 0xff;
unnneeded
[...]
> +/**
> + * Read an access unit from the stream.
> + * Returns -1 on error, 0 if not enough data is present in the input stream
> + * otherwise returns the number of bytes consumed.
> + */
> +
> +static int read_access_unit(AVCodecContext *avctx, void* data, int *data_size,
> + uint8_t *buf, int buf_size)
> +{
> + MLPDecodeContext *m = avctx->priv_data;
> + GetBitContext gb;
> + unsigned int length, substr;
> + unsigned int substream_start;
> + unsigned int header_size;
> + uint8_t substream_parity_present[MAX_SUBSTREAMS];
> + uint16_t substream_data_len[MAX_SUBSTREAMS];
every use of substream_data_len either has a *2 or a *16 after it
multiplying this by *2 when its set would simplify the code
[...]
> + skip_bits(&gb, (-get_bits_count(&gb)) & 15);
> + if ((substream_data_len[substr] * 16) - get_bits_count(&gb) >= 48 &&
superflouse ()
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I wish the Xiph folks would stop pretending they've got something they
do not. Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/20080108/686449e2/attachment.pgp>
More information about the ffmpeg-devel
mailing list