[FFmpeg-devel] [RFC v2 3/3] daaladec: Implement a native Daala decoder

[Ronald S. Bultje]

Hi,

On Tue, Dec 29, 2015 at 11:29 AM, Andreas Cadhalpun <
andreas.cadhalpun at googlemail.com> wrote:

> Hi,
>
> On 29.12.2015 17:15, Ronald S. Bultje wrote:
> > On Tue, Dec 29, 2015 at 11:07 AM, Andreas Cadhalpun <
> > andreas.cadhalpun at googlemail.com> wrote:
> >
> >> On 29.12.2015 16:58, Ronald S. Bultje wrote:
> >>> On Tue, Dec 29, 2015 at 10:55 AM, Andreas Cadhalpun <
> >>> andreas.cadhalpun at googlemail.com> wrote:
> >>>
> >>>>> +static av_always_inline void idct_1D_8(pixel *x, int xstride, const
> >>>> pixel y[16])
> >>>>> +{
> >>>>> +    int t1h, t4h, t6h, t0 = y[0], t1 = y[1], t2 = y[2], t3 = y[3],
> t4
> >> =
> >>>> y[4];
> >>>>> +    int t5 = y[5], t6 = y[6], t7 = y[7];
> >>>>> +    t5 -= (t3*2485 + 4096) >> 13;
> >>>>> +    t3 += (t5*18205 + 16384) >> 15;
> >>>>> +    t5 -= (t3*2485 + 4096) >> 13;
> >>>>> +    t7 -= (t1*3227 + 16384) >> 15;
> >>>>> +    t1 += (t7*6393 + 16384) >> 15;
> >>>>> +    t7 -= (t1*3227 + 16384) >> 15;
> >>>>> +    t1 += t3;
> >>>>
> >>>> These seven lines can overflow.
> >>>
> >>>
> >>> Why do you believe they can overflow?
> >>
> >> Because ubsan told me that.
> >>
> >>> Look at range constraints for the input values.
> >>
> >> Apparently that is not constrained enough, e.g.:
> >> t3 = -1449866
> >> t3*2485 = -3602917010 < INT32_MIN
> >
> >
> > So what type is a pixel?
>
> #define pixel int32_t
>
> > Isn't a pixel uint8_t?
>
> No.
>
> > And coefs are typically constrained alike. Or are we talking 12bpp
> content here?
>
> I don't know.
>
> > In that case, you likely need 64bit integers for 15bit math precision
> (look at
> > vp9 code), or daala needs to reduce precision (as does hevc).
>
> Yes, either the intermediate calculation needs to happen with 64bit
> integers,
> or the input has to be sanitized in some way.


It depends what the purpose and source was. Was this real input, or fuzzed,
or what? vp9 decoder can certainly overflow with garbage input and that is
specifically defined so in libvpx. "Only input generated from a real fdct"
is considered sane and has a defined outcome.

(Overflows in dsp code are typically not a security concern.)

Ronald