On 13.01.2016 19:18, foo86 wrote: > On Tue, Jan 12, 2016 at 11:58:43PM +0100, Andreas Cadhalpun wrote: >> It's not completely fixed yet, because the check is only done if >> static_fields_present is 1. > > You are right, placing the check there doesn't account for case when > static_fields_present becomes 0 later. I moved the check closer to > problematic get_bits() call for a more complete fix.
OK. >> I think the correct fix would be to set mix_metadata_enabled to 0 if >> static_fields_present is 0, e.g. in the else branch in ff_dca_exss_parse. > > The spec doesn't give many details about this, but when > static_fields_present is 0, values parsed earlier are supposedly > retained for stream duration, so clearing mix_metadata_enabled doesn't > seem correct in this case. I see. >> Thanks to your hints I could increase the coverage above 90%. >> I found a few more issues along the way: >> >> static void get_array(GetBitContext *s, int *array, int size, int n) >> { >> int i; >> >> for (i = 0; i < size; i++) >> array[i] = get_sbits(s, n); >> >> This should be get_sbits_long, because n can be larger than 25. > > Added xbr_bit_allocation[][] range check to make sure get_sbits() > argument doesn't exceed 23. Thanks. >> static int chs_assemble_freq_bands(DCAXllDecoder *s, DCAXllChSet *c) >> { >> int i, ch, nsamples = s->nframesamples, *ptr; >> >> av_assert1(c->nfreqbands > 1); >> >> This assert can be triggered. > > There was supposed to be a check to ensure that all channel sets have > the same number of frequency bands, but apparently it got lost during > porting. Re-added it. That fixed it. >> static void combine_residual_frame(DCAXllDecoder *s, DCACoreDecoder *core, >> DCAXllChSet *c) >> { >> int ch, nsamples = s->nframesamples; >> DCAXllChSet *o; >> >> av_assert0(c->freq == core->output_rate); >> av_assert0(nsamples == core->npcmsamples); >> >> These two, as well. >> Maybe they should be regular error checks instead? > > Hmm, core sample rate and number of samples are already checked to be > compatible with XLL at this point. There must be some bug in upsampling > logic if these asserts are triggering, however it is not obvious for me > where the problem is... Can you provide a sample which triggers these? > It would be helpful. Sure, I'll send them to you privately. >> static void scale_down_mix(DCAXllDecoder *s, DCAXllChSet *o, int band) >> { >> int i, j, nchannels = 0, nsamples = s->nframesamples; >> DCAXllChSet *c; >> >> for (i = 0, c = s->chset; i < s->nactivechsets; i++, c++) { >> if (!c->hier_chset) >> continue; >> for (j = 0; j < c->nchannels; j++) { >> int scale = o->dmix_scale[nchannels++]; >> if (scale != (1 << 15)) { >> s->dcadsp->dmix_scale(c->bands[band].msb_sample_buffer[j], >> scale, nsamples); >> >> c->bands[band].msb_sample_buffer[j] can be NULL here, which causes a crash. > > A check re-added earlier to ensure that all channels sets have the same > number of frequency bands should fix this as well, I think. Yes, it does. >> Additionally there are some rarely happening overflows in dcadsp.c. >> (More precisely in filter0, filter1, dmix_sub_c and dmix_scale_c.) >> It would be nice to avoid those, if that's possible without significant >> speed loss. > > I wouldn't worry much about overflows if they happen only with fuzzed > files... Though this is definitely a problem if normal streams trigger > these. They only happen with fuzzed samples and quite rarely at that. I just wanted to mention it for completeness. Best regards, Andreas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel