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?
>
> foo86, could you work on changing the patch to replace the original instead?
>
> After it is merged, we could think about integrating your test-suite
> into the FATE system, but all in good time.
>
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
