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

[Andreas Cadhalpun]

Hi,

On 29.12.2015 19:46, Ronald S. Bultje wrote:
> On Tue, Dec 29, 2015 at 11:49 AM, Andreas Cadhalpun <
> andreas.cadhalpun at googlemail.com> wrote:
>> Do you have a sample causing overflows in the vp9 decoder?
> 
> 
> Nope, but I'm going to assume they're not hard to create, just use a few
> high same-sign quantized coefficients (e.g. SHORT_MAX) and a high quantizer
> (qidx=255, dq_ac=1828). Both dequant into int16_t (1828*SHORT_MAX doesn't
> fit in 16 bits) as well as the idct results themselves (because you're
> adding and subtracting from an already near-edge value which should fit in
> 16bits) will overflow.

I guess hard depends on who you ask. I've fuzzed the vp9 decoder and
haven't yet come across a sample that triggers a signed integer overflow there.
In the case of this daala decoder, 3/4 of the fuzzed samples do...

>> (Overflows in dsp code are typically not a security concern.)
>>
>> Well, the overflows in the imdct calculation of the aac_fixed decoder
>> ultimately
>> caused crashes.
> 
> 
> I would prefer if for video codecs, unless specifically proven otherwise
> for that codec with specific content, we assume by default that overflows
> in performance-sensitive dsp code (specifically idct) are OK. ubsan may not
> like us on fuzzed content, but so be it.

I prefer that input for such performance-sensitive dsp code gets sanitized,
so that overflows are (nearly) impossible.

The daala decoder uses fixed point approximations for multiplication
with floats like:
    /* 13573/32768 ~= Tan[pi/8] ~= 0.414213562373095 */ \
    t0 += (t1*13573 + 16384) >> 15; \

For these multiplications with up to 2^15 to fit into int32_t the input must
be in the +/- 2^16 range, so it should be clipped to that range, e.g. with:
--- a/libavcodec/daaladec.c
+++ b/libavcodec/daaladec.c
@@ -393,6 +393,19 @@ static inline void decode_block_pvq(DaalaContext *s, int x, int y, int p,
 
     daala_coding_to_raster(&d[boffset], aw, pred, n);
 
+    for (int i = 0; i < 1 << (bsize + 2); i += 1) {
+        for (int j = 0; j < 1 << (bsize + 2); j += 1) {
+            int idx = boffset + i * aw + j;
+            if (s->dcoef[p][idx] > UINT16_MAX) {
+                av_log(NULL, AV_LOG_WARNING, "dcoef %d too large\n", s->dcoef[p][idx]);
+                s->dcoef[p][idx] = UINT16_MAX;
+            } else if (s->dcoef[p][idx] < -UINT16_MAX) {
+                av_log(NULL, AV_LOG_WARNING, "dcoef %d too small\n", s->dcoef[p][idx]);
+                s->dcoef[p][idx] = -UINT16_MAX;
+            }
+        }
+    }
+
     /* IDCT */
     if (s->dsp.idct[bsize])
         s->dsp.idct[bsize]((uint8_t *)(s->ccoef[p] + boffset), aw,

This prevents most of the overflows in the idct and prints warnings instead of
silent failure. If these happen with valid input, the idct has to be changed,
and otherwise the clipping should be harmless. 

Best regards,
Andreas