On Wed, Jan 6, 2016 at 12:40 AM, Ganesh Ajjanagadde <gajja...@mit.edu> wrote: > On Tue, Jan 5, 2016 at 3:28 PM, Hendrik Leppkes <h.lepp...@gmail.com> wrote: >> On Tue, Jan 5, 2016 at 10:46 PM, Andreas Cadhalpun >> <andreas.cadhal...@googlemail.com> wrote: >>> On 05.01.2016 21:38, foo86 wrote: >>>> On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: >>>>> On 03.01.2016 18:49, foo86 wrote: >>>>>> +// 5.3.1 - Bit stream header >>>>>> +static int parse_frame_header(DCA2CoreDecoder *s) >>>>>> +{ >>>>> [...] >>>>>> + // Source PCM resolution >>>>>> + s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = >>>>>> get_bits(&s->gb, 3)]; >>>>> >>>>> This can cause an out-of-bounds read if get_bits returns 7, because >>>>> ff_dca_bits_per_sample >>>>> only has 7 elements. >>>> >>>> Fixed locally, thanks. >>> >>> Thanks. >>> >>>> P.S. To avoid resending this huge patch, I've put the fixes accumulated >>>> so far in a private dcadec2 branch on github [1] (will be rebased >>>> frequently against FFmpeg master). >>>> >>>> [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 >>> >>> OK. This decoder seems to be quite robust in handling fuzzed samples, >>> so from a security point of view it should be fine to replace the >>> old dca decoder with this one. >>> >> >> >> So that leaves us with a bunch of positive comments, on this side >> anyway, and noone opposed yet. >> >> Arguments for a switch include: >> - Nearly complete coverage of all DTS features, well tested and >> confirmed bit-exact (only DTS Express is missing, which is technically >> its own little codec using DTS EXSS headers) >> - Slightly faster (~5%) >> - Active maintainer >> - Andreas seal of security approval ;) >> >> If anyone thinks we should not replace our decoder, speak now or >> forever hold your peace (and bring proper arguments). >> I will try to do a code review to the best of my abilities in the upcoming >> days. > > For now, I definitely think we should replace our decoder. > Just a clarification: in the long run, isn't it a good idea to get > this into the repo and not use an external library? For instance, if > and when asm code gets written for this, isn't it easier to work > within FFmpeg's codebase, which has some infrastructure for writing > asm? >
The whole idea of this patch is to integrate the decoder fully into our codebase, and no longer use the external library. PS: What I forgot to mention in my last mail, foo86 if you have any questions about ripping out the old dca decoder, feel free to contact me. - Hendrik _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel