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.
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
