From 8f28ec275e412dd3af4f3cda460605512faf332c Mon Sep 17 00:00:00 2001
From: Ruud van Asseldonk <dev@veniogames.com>
Date: Thu, 23 Aug 2018 20:01:40 +0200
Subject: [PATCH] Fix bug in decoding residuals

A partition order could occur, such that the block size was not a
multiple of 2^order. Computation of the number of samples per partition
did not account for this case, rounding down due to the bit shift. This
meant that we would not fill the entire decode buffer.

Claxon does not zero the decode buffer because it is (should be)
overwritten anyway, and in the case of a format error, where the buffer
might be only partially full, the buffer is not exposed again.
Furthermore, the way decoding works in most places, is that we fill the
entire buffer, just by looping to fill it. If the input bitstream does
not contain enough data to fill the buffer, then that's a format error.
In a few places though, we need to slice up the buffer before decoding
into it: for decoding individual channels, and also for decoding
residuals, which are split into partitions.

This particular format error was especially nasty because it did not
cause a format error down the line. Instead, it caused the buffer to be
sliced in a way where the slices together did not cover the entire
buffer, and so parts of uninitialized memory could remain in the buffer.

Thanks a lot to Sergey "Shnatsel" Davidoff for reporting this bug,
together with elaborate steps to reproduce that allowed me to pinpoint
the cause quickly.
---
 src/subframe.rs | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/subframe.rs b/src/subframe.rs
index 01056bd..9d948b3 100644
--- a/src/subframe.rs
+++ b/src/subframe.rs
@@ -254,12 +254,25 @@ fn decode_residual<R: ReadBytes>(input: &mut Bitstream<R>,
     // most 2^16 - 1 samples in the block. No values have been marked as
     // invalid by the specification though.
     let n_partitions = 1u32 << order;
-    let n_samples = block_size >> order;
+    let n_samples_per_partition = block_size >> order;
+
+    // The partitions together must fill the block. If the block size is not a
+    // multiple of 2^order; if we shifted off some bits, then we would not fill
+    // the entire block. Such a partition order is invalid for this block size.
+    if block_size & (n_partitions - 1) as u16 != 0 {
+        return fmt_err("invalid partition order")
+    }
+
+    // NOTE: the check above checks that block_size is a multiple of n_partitions
+    // (this works because n_partitions is a power of 2). The check below is
+    // equivalent but more expensive.
+    debug_assert_eq!(n_partitions * n_samples_per_partition as u32, block_size as u32);
+
     let n_warm_up = block_size - buffer.len() as u16;
 
     // The partition size must be at least as big as the number of warm-up
     // samples, otherwise the size of the first partition is negative.
-    if n_warm_up > n_samples {
+    if n_warm_up > n_samples_per_partition {
         return fmt_err("invalid residual");
     }
 
@@ -267,22 +280,22 @@ fn decode_residual<R: ReadBytes>(input: &mut Bitstream<R>,
     match partition_type {
         RicePartitionType::Rice => {
             let mut start = 0;
-            let mut len = n_samples - n_warm_up;
+            let mut len = n_samples_per_partition - n_warm_up;
             for _ in 0..n_partitions {
                 let slice = &mut buffer[start..start + len as usize];
                 try!(decode_rice_partition(input, slice));
                 start = start + len as usize;
-                len = n_samples;
+                len = n_samples_per_partition;
             }
         }
         RicePartitionType::Rice2 => {
             let mut start = 0;
-            let mut len = n_samples - n_warm_up;
+            let mut len = n_samples_per_partition - n_warm_up;
             for _ in 0..n_partitions {
                 let slice = &mut buffer[start..start + len as usize];
                 try!(decode_rice2_partition(input, slice));
                 start = start + len as usize;
-                len = n_samples;
+                len = n_samples_per_partition;
             }
         }
     }