Repository: pdeljanov/Symphonia (language: rust)

Issue:
`attempt to subtract with overflow` error when parsing id3 crc32
Running [this file](https://mega.nz/file/szRABbDb#1HvTGjQ-IrNGTlCPuSNkQ5BgFctejtW2LEWg02iUtWQ) with symphonia-play results in the following error: `thread 'main' panicked at 'attempt to subtract with overflow', symphonia-metadata/src/id3v2/unsync.rs:21:58`. The error comes from [this line](https://github.com/pdeljanov/Symphonia/blob/87eef4916ef976ea335e3051ac62a1187d1782f7/symphonia-metadata/src/id3v2/unsync.rs#L21) and particularly this bit: `(bit_width - bits_read)`. Most places call this function with a bit width of 28, which works okay because `bits_read` is incremented by 7 each time which divides evenly into 28. However, when this is called with a bit width of 32 on [this line](https://github.com/pdeljanov/Symphonia/blob/87eef4916ef976ea335e3051ac62a1187d1782f7/symphonia-metadata/src/id3v2/mod.rs#L215), it causes the while loop [here](https://github.com/pdeljanov/Symphonia/blob/87eef4916ef976ea335e3051ac62a1187d1782f7/symphonia-metadata/src/id3v2/unsync.rs#L19) to perform the following calculations for `(bit_width - bits_read)`:
```rust
32 - 7
32 - 14
32 - 21
32 - 28
32 - 35
```
Once it gets to the last line it overflows since it goes negative. I was able to fix this by changing the while loop to the following:
```rust
while bits_read < bit_width {
    // Ensure bits_read never exceeds the bit width which will cause an overflow
    bits_read = (bits_read + 7).min(bit_width);
    result |= u32::from(reader.read_u8()? & 0x7f) << (bit_width - bits_read);
}
```
After making this change, symphonia-play prints out all the tags as expected. If this is the correct approach, I'd be happy to make a PR for this. Just wanted to check if this is the correct way to fix it first. Thanks!



Discussion / hints:
Hi @aschey,

Thank you for the thoughtful bug report. I think your solution is correct. According to the [ID3v2.4](https://id3.org/id3v2.4.0-structure) spec. section 6.2:

> Synchsafe integers are integers that keep its highest bit (bit 7) zeroed, making seven bits out of eight available. Thus a 32 bit synchsafe integer can store 28 bits of information.

This function is supposed to hide that complexity. Since the CRC is 32-bits wide, this function should consume 5 bytes and combine the lower 7 bits from the first 4 bytes, and the lower 4 bits from the 5th byte to generate the final 32-bit integer.

I'd be very happy to have a PR to fix this. I would include it in the upcoming 0.5 release (Monday-ish). Could you also add a couple tests?

Thanks!
Thanks for the background info @pdeljanov, that makes sense to me. I'll get a PR together with some tests later today.

Fix patch:
diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 7920bae9..1160292d 100644
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -15,6 +15,7 @@ Thom Chiovoloni <chiovolonit@gmail.com>
 #
 # Please keep this section sorted in ascending order.
 
+aschey [https://github.com/aschey]
 BlackHoleFox [https://github.com/blackholefox]
 darksv [https://github.com/darksv]
 djugei [https://github.com/djugei]
diff --git a/symphonia-metadata/src/id3v2/unsync.rs b/symphonia-metadata/src/id3v2/unsync.rs
index aa5dbb09..82251699 100644
--- a/symphonia-metadata/src/id3v2/unsync.rs
+++ b/symphonia-metadata/src/id3v2/unsync.rs
@@ -10,18 +10,22 @@ use std::io;
 use symphonia_core::errors::Result;
 use symphonia_core::io::{FiniteStream, ReadBytes};
 
-pub fn read_syncsafe_leq32<B: ReadBytes>(reader: &mut B, bit_width: u32) -> Result<u32> {
+pub fn read_syncsafe_leq32<B: ReadBytes>(reader: &mut B, bit_width: u8) -> Result<u32> {
     debug_assert!(bit_width <= 32);
 
     let mut result = 0u32;
     let mut bits_read = 0;
 
     while bits_read < bit_width {
-        bits_read += 7;
-        result |= u32::from(reader.read_u8()? & 0x7f) << (bit_width - bits_read);
+        // Ensure bits_read never exceeds the bit width which will cause an overflow
+        let next_read = (bit_width - bits_read).min(7);
+        bits_read += next_read;
+        // The mask should have of the bits below 2 ^ nex_read set to 1
+        let mask = (1 << next_read) - 1;
+        result |= u32::from(reader.read_u8()? & mask) << (bit_width - bits_read);
     }
 
-    Ok(result & (0xffff_ffff >> (32 - bit_width)))
+    Ok(result)
 }
 
 pub fn decode_unsynchronisation(buf: &mut [u8]) -> &mut [u8] {
@@ -174,3 +178,28 @@ impl<B: ReadBytes + FiniteStream> ReadBytes for UnsyncStream<B> {
         unimplemented!();
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::read_syncsafe_leq32;
+    use symphonia_core::io::BufReader;
+
+    #[test]
+    fn verify_read_syncsafe_leq32() {
+        let mut stream = BufReader::new(&[3, 4, 80, 1, 15]);
+        assert_eq!(101875743, read_syncsafe_leq32(&mut stream, 32).unwrap());
+
+        // Special case: for a bit depth that is not a multiple of 7 such as 32
+        // we need to ensure the mask is correct.
+        // In this case, the final iteration should read 4 bits and have a mask of 0b0000_1111.
+        // 0b0000_1111 has a 0 in 16's place so testing mask & 16 will ensure this is working.
+        let mut stream = BufReader::new(&[16, 16, 16, 16, 16]);
+        assert_eq!(541098240, read_syncsafe_leq32(&mut stream, 32).unwrap());
+
+        let mut stream = BufReader::new(&[3, 4, 80, 1]);
+        assert_eq!(6367233, read_syncsafe_leq32(&mut stream, 28).unwrap());
+
+        let mut stream = BufReader::new(&[3, 4, 80, 1]);
+        assert_eq!(0, read_syncsafe_leq32(&mut stream, 0).unwrap());
+    }
+}


Test patch: