Repository: sharksforarms/deku (rust)

Issue:
Offset in from_bytes() causes panic in reader.rs if the bytes are greater than 128
At some point after 0.16 my code started crashing when I was using an offset in from_bytes() where the input bits total more than 128 bytes. So if I have something like this, for example:

```rust
 match AccuracyDescriptionRecord::from_bytes((&data, 1032)) {
    Ok((_remaining_data, acc_desc_record)) => acc_desc_record.print(),
    Err(error) => panic!("Error getting Accuracy Description Record from data: {error}"),
}
``` 

The error looks like:

> thread 'main' panicked at /Users/justin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deku-0.18.1/src/reader.rs:258:63:
> range end index 434 out of range for slice of length 128

This seems to be happening because from_bytes() is limiting the input offset to the MAX_BITS_AMT in the reader.

Pull request description:
Seek first bytes lengths before seek bits for __deku_input
* For bits input into from_bytes, first seek the byte amount until we can seek the bits amount

Closes #500

Patch:
diff --git a/deku-derive/src/macros/deku_read.rs b/deku-derive/src/macros/deku_read.rs
index a5d2ee4..53788aa 100644
--- a/deku-derive/src/macros/deku_read.rs
+++ b/deku-derive/src/macros/deku_read.rs
@@ -42,7 +42,8 @@ fn emit_struct(input: &DekuData) -> Result<TokenStream, syn::Error> {
             {
                 use ::#crate_::no_std_io::Seek;
                 use ::#crate_::no_std_io::SeekFrom;
-                if let Err(e) = __deku_reader.seek(SeekFrom::Current(i64::try_from(#num).unwrap())) {
+                let seek_amt = i64::try_from(#num).expect("could not convert into i64");
+                if let Err(e) = __deku_reader.seek(SeekFrom::Current(seek_amt)) {
                     return Err(::#crate_::DekuError::Io(e.kind()));
                 }
             }
@@ -52,7 +53,8 @@ fn emit_struct(input: &DekuData) -> Result<TokenStream, syn::Error> {
             {
                 use ::#crate_::no_std_io::Seek;
                 use ::#crate_::no_std_io::SeekFrom;
-                if let Err(e) = __deku_reader.seek(SeekFrom::End(i64::try_from(#num).unwrap())) {
+                let seek_amt = i64::try_from(#num).expect("could not convert into i64");
+                if let Err(e) = __deku_reader.seek(SeekFrom::End(seek_amt)) {
                     return Err(::#crate_::DekuError::Io(e.kind()));
                 }
             }
@@ -62,7 +64,8 @@ fn emit_struct(input: &DekuData) -> Result<TokenStream, syn::Error> {
             {
                 use ::#crate_::no_std_io::Seek;
                 use ::#crate_::no_std_io::SeekFrom;
-                if let Err(e) = __deku_reader.seek(SeekFrom::Start(u64::try_from(#num).unwrap())) {
+                let seek_amt = u64::try_from(#num).expect("could not convert into u64");
+                if let Err(e) = __deku_reader.seek(SeekFrom::Start(seek_amt)) {
                     return Err(::#crate_::DekuError::Io(e.kind()));
                 }
             }
diff --git a/deku-derive/src/macros/deku_write.rs b/deku-derive/src/macros/deku_write.rs
index 2d19c21..cd917a3 100644
--- a/deku-derive/src/macros/deku_write.rs
+++ b/deku-derive/src/macros/deku_write.rs
@@ -33,7 +33,8 @@ fn emit_struct(input: &DekuData) -> Result<TokenStream, syn::Error> {
             {
                 use ::#crate_::no_std_io::Seek;
                 use ::#crate_::no_std_io::SeekFrom;
-                if let Err(e) = __deku_writer.seek(SeekFrom::Current(i64::try_from(#num).unwrap())) {
+                let seek_amt = i64::try_from(#num).expect("could not convert into i64");
+                if let Err(e) = __deku_writer.seek(SeekFrom::Current(seek_amt)) {
                     return Err(::#crate_::DekuError::Io(e.kind()));
                 }
             }
@@ -43,7 +44,8 @@ fn emit_struct(input: &DekuData) -> Result<TokenStream, syn::Error> {
             {
                 use ::#crate_::no_std_io::Seek;
                 use ::#crate_::no_std_io::SeekFrom;
-                if let Err(e) = __deku_writer.seek(SeekFrom::End(i64::try_from(#num).unwrap())) {
+                let seek_amt = i64::try_from(#num).expect("could not convert into i64");
+                if let Err(e) = __deku_writer.seek(SeekFrom::End(seek_amt)) {
                     return Err(::#crate_::DekuError::Io(e.kind()));
                 }
             }
@@ -53,7 +55,8 @@ fn emit_struct(input: &DekuData) -> Result<TokenStream, syn::Error> {
             {
                 use ::#crate_::no_std_io::Seek;
                 use ::#crate_::no_std_io::SeekFrom;
-                if let Err(e) = __deku_writer.seek(SeekFrom::Start(u64::try_from(#num).unwrap())) {
+                let seek_amt = u64::try_from(#num).expect("could not convert into u64");
+                if let Err(e) = __deku_writer.seek(SeekFrom::Start(seek_amt)) {
                     return Err(::#crate_::DekuError::Io(e.kind()));
                 }
             }
diff --git a/src/impls/cstring.rs b/src/impls/cstring.rs
index d0796b1..606c70c 100644
--- a/src/impls/cstring.rs
+++ b/src/impls/cstring.rs
@@ -31,6 +31,23 @@ where
     ) -> Result<Self, DekuError> {
         let bytes =
             Vec::from_reader_with_ctx(reader, (Limit::from(|b: &u8| *b == 0x00), inner_ctx))?;
+        let value = CString::from_vec_with_nul(bytes).map_err(|e| {
+            DekuError::Parse(Cow::from(format!("Failed to convert Vec to CString: {e}")))
+        })?;
+
+        Ok(value)
+    }
+}
+
+impl<'a, Ctx: Copy> DekuReader<'a, (ByteSize, Ctx)> for CString
+where
+    u8: DekuReader<'a, Ctx>,
+{
+    fn from_reader_with_ctx<R: Read + Seek>(
+        reader: &mut Reader<R>,
+        (byte_size, inner_ctx): (ByteSize, Ctx),
+    ) -> Result<Self, DekuError> {
+        let bytes = Vec::from_reader_with_ctx(reader, (Limit::from(byte_size.0), inner_ctx))?;
 
         let value = CString::from_vec_with_nul(bytes).map_err(|e| {
             DekuError::Parse(Cow::from(format!("Failed to convert Vec to CString: {e}")))
@@ -49,25 +66,59 @@ mod tests {
 
     use super::*;
 
-    #[rstest(input, expected, expected_rest,
+    #[rstest(input, len, expected, expected_rest,
         case(
             &[b't', b'e', b's', b't', b'\0'],
+            Some(5),
             CString::new("test").unwrap(),
             &[],
         ),
+        case(
+            &[b't', b'e', b's', b't', b'\0'],
+            None,
+            CString::new("test").unwrap(),
+            &[],
+        ),
+        case(
+            &[b't', b'e', b's', b't', b'\0', b'a'],
+            Some(5),
+            CString::new("test").unwrap(),
+            &[b'a'],
+        ),
         case(
             &[b't', b'e', b's', b't', b'\0', b'a'],
+            None,
             CString::new("test").unwrap(),
             &[b'a'],
         ),
 
+        #[should_panic(expected = "Parse(\"Failed to convert Vec to CString: data provided is not nul terminated\")")]
+        case(
+            &[b't', b'e', b's', b't'],
+            Some(4),
+            CString::new("test").unwrap(),
+            &[b'a'],
+        ),
+
+        #[should_panic(expected = "Incomplete(NeedSize { bits: 8 })")]
+        case(&[b't', b'e', b's', b't'], Some(5), CString::new("test").unwrap(), &[]),
+
         #[should_panic(expected = "Incomplete(NeedSize { bits: 8 })")]
-        case(&[b't', b'e', b's', b't'], CString::new("test").unwrap(), &[]),
+        case(&[b't', b'e', b's', b't'], None, CString::new("test").unwrap(), &[]),
     )]
-    fn test_cstring(input: &[u8], expected: CString, expected_rest: &[u8]) {
+    fn test_cstring_count(
+        input: &[u8],
+        len: Option<usize>,
+        expected: CString,
+        expected_rest: &[u8],
+    ) {
         let mut cursor = Cursor::new(input);
         let mut reader = Reader::new(&mut cursor);
-        let res_read = CString::from_reader_with_ctx(&mut reader, ()).unwrap();
+        let res_read = if let Some(len) = len {
+            CString::from_reader_with_ctx(&mut reader, (ByteSize(len), ())).unwrap()
+        } else {
+            CString::from_reader_with_ctx(&mut reader, ()).unwrap()
+        };
         assert_eq!(expected, res_read);
         let mut buf = vec![];
         cursor.read_to_end(&mut buf).unwrap();
diff --git a/src/reader.rs b/src/reader.rs
index 8252823..80d7e82 100644
--- a/src/reader.rs
+++ b/src/reader.rs
@@ -174,8 +174,24 @@ impl<'a, R: Read + Seek> Reader<'a, R> {
         {
             #[cfg(feature = "logging")]
             log::trace!("skip_bits: {amt}");
+
+            let bytes_amt = amt / 8;
+            let bits_amt = amt % 8;
+
+            // first, seek with bytes
+            if bytes_amt != 0 {
+                self.seek(SeekFrom::Current(
+                    i64::try_from(bytes_amt).expect("could not convert seek usize into i64"),
+                ))
+                .map_err(|e| DekuError::Io(e.kind()))?;
+            }
+
+            // Unlike normal seek not couting as bits_read, this one does
+            // to keep from_bytes returns
+            self.bits_read += bytes_amt * 8;
+
             // Save, and keep the leftover bits since the read will most likely be less than a byte
-            self.read_bits(amt)?;
+            self.read_bits(bits_amt)?;
         }
 
         #[cfg(not(feature = "bits"))]


Test patch:
diff --git a/tests/test_cstring.rs b/tests/test_cstring.rs
new file mode 100644
index 0000000..10d7636
--- /dev/null
+++ b/tests/test_cstring.rs
@@ -0,0 +1,29 @@
+use deku::prelude::*;
+
+use std::ffi::CString;
+
+#[test]
+fn test_cstring_no_ctx() {
+    #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
+    pub struct Data {
+        s: CString,
+    }
+
+    let bytes = &[b't', b'e', b's', b't', b'\0'];
+    let (_, d) = Data::from_bytes((bytes, 0)).unwrap();
+    assert_eq!(d.s, CString::new("test").unwrap());
+}
+
+#[test]
+fn test_cstring_bytes() {
+    #[derive(PartialEq, Debug, DekuRead, DekuWrite)]
+    pub struct Data {
+        len: u8,
+        #[deku(bytes = "*len as usize")]
+        s: CString,
+    }
+
+    let bytes = &[0x05, b't', b'e', b's', b't', b'\0'];
+    let (_, d) = Data::from_bytes((bytes, 0)).unwrap();
+    assert_eq!(d.s, CString::new("test").unwrap());
+}
diff --git a/tests/test_from_bytes.rs b/tests/test_from_bytes.rs
index aac1f4d..cb43076 100644
--- a/tests/test_from_bytes.rs
+++ b/tests/test_from_bytes.rs
@@ -54,3 +54,30 @@ fn test_from_bytes_enum() {
     assert_eq!(6, i);
     assert_eq!(0b0101_1010u8, rest[0]);
 }
+
+#[cfg(feature = "bits")]
+#[test]
+fn test_from_bytes_long() {
+    #[derive(Debug, PartialEq, DekuRead, DekuWrite)]
+    #[deku(id_type = "u8", bits = 4)]
+    enum TestDeku {
+        #[deku(id = "0b0110")]
+        VariantA(#[deku(bits = 4)] u8),
+        #[deku(id = "0b0101")]
+        VariantB(#[deku(bits = 2)] u8),
+    }
+
+    let mut test_data = vec![0x00; 200];
+    test_data.extend([0b0110_0110u8, 0b0101_1010u8].to_vec());
+
+    let ((rest, i), ret_read) = TestDeku::from_bytes((&test_data, 200 * 8)).unwrap();
+    assert_eq!(TestDeku::VariantA(0b0110), ret_read);
+    assert_eq!(1, rest.len());
+    assert_eq!(0, i);
+
+    let ((rest, i), ret_read) = TestDeku::from_bytes((rest, i)).unwrap();
+    assert_eq!(TestDeku::VariantB(0b10), ret_read);
+    assert_eq!(1, rest.len());
+    assert_eq!(6, i);
+    assert_eq!(0b0101_1010u8, rest[0]);
+}