feat(sysex): splicing payload data

Author: BenLeadbetter

fuzz/Cargo.toml

@@ -38,8 +38,8 @@ doc = false
 bench = false
 
 [[bin]]
-name = "generic_sysex_inserting_payloads"
-path = "./fuzz_targets/generic_sysex_inserting_payloads.rs"
+name = "generic_sysex_splicing_payloads"
+path = "./fuzz_targets/generic_sysex_splicing_payloads.rs"
 test = false
 doc = false
 bench = false

…gets/generic_sysex_inserting_payloads.rs …rgets/generic_sysex_splicing_payloads.rsfuzz/fuzz_targets/generic_sysex_inserting_payloads.rs renamed to fuzz/fuzz_targets/generic_sysex_splicing_payloads.rs fuzz/fuzz_targets/generic_sysex_inserting_payloads.rs renamed to fuzz/fuzz_targets/generic_sysex_splicing_payloads.rs

@@ -59,12 +59,13 @@ impl IntoByte<u8> for u8 {
     }
 }
 
-fn test_case<B, M>(data: &InputData, mut message: M, index: usize)
+fn test_case<B, M, R>(data: &InputData, mut message: M, range: R)
 where
     B: midi2::buffer::Buffer + midi2::buffer::BufferTryResize + midi2::buffer::BufferMut,
     M: midi2::Sysex<B>,
     <M as Sysex<B>>::Byte: Eq + core::fmt::Debug,
     u8: IntoByte<<M as Sysex<B>>::Byte>,
+    R: core::ops::RangeBounds<usize> + Clone,
 {
     let Ok(()) = message.try_set_payload(data.initial_data.iter().map(u8::byte)) else {
         return;
@@ -78,14 +79,16 @@ where
         );
     }
 
-    let Ok(()) = message.try_insert_payload(data.data_to_insert.iter().map(u8::byte), index) else {
+    let Ok(()) =
+        message.try_splice_payload(data.data_to_insert.iter().map(u8::byte), range.clone())
+    else {
         return;
     };
 
     let actual = message.payload().collect::<Vec<_>>();
     let expected = {
         let mut ret = data.initial_data.clone();
-        ret.splice(index..index, data.data_to_insert.clone());
+        ret.splice(range, data.data_to_insert.clone());
         ret.iter().map(u8::byte).collect::<Vec<_>>()
     };
     assert_eq!(actual, expected);
@@ -94,33 +97,41 @@ where
 fuzz_target!(|data: InputData| {
     let mut rng = rand::rngs::StdRng::seed_from_u64(data.seed);
     let fized_size_buffer_size = rng.random_range(4..MAX_BUFFER_SIZE);
-    let index = if data.initial_data.is_empty() {
-        0
-    } else {
-        rng.random_range(0..data.initial_data.len())
+    let range = {
+        if data.initial_data.is_empty() {
+            0..0
+        } else {
+            let lower = rng.random_range(0..data.initial_data.len());
+            if lower == data.initial_data.len() {
+                lower..lower
+            } else {
+                let upper = rng.random_range(lower..data.initial_data.len());
+                lower..upper
+            }
+        }
     };
     test_case(
         &data,
         midi2::sysex8::Sysex8::<FixedSizeBuffer<u32>>::try_new_with_buffer(
             FixedSizeBuffer::<u32>::new(fized_size_buffer_size),
         )
         .unwrap(),
-        index,
+        range.clone(),
     );
     test_case(
         &data,
         midi2::sysex7::Sysex7::<FixedSizeBuffer<u32>>::try_new_with_buffer(
             FixedSizeBuffer::<u32>::new(fized_size_buffer_size),
         )
         .unwrap(),
-        index,
+        range.clone(),
     );
     test_case(
         &data,
         midi2::sysex7::Sysex7::<FixedSizeBuffer<u8>>::try_new_with_buffer(
             FixedSizeBuffer::<u8>::new(fized_size_buffer_size),
         )
         .unwrap(),
-        index,
+        range.clone(),
     );
 });

midi2/src/detail/helpers.rs

@@ -89,16 +89,17 @@ pub fn validate_sysex_group_statuses<
     Ok(())
 }
 
-pub fn try_insert_sysex_data<
+pub fn try_splice_sysex_data<
     B: crate::buffer::Buffer + crate::buffer::BufferMut + crate::buffer::BufferTryResize,
     S: SysexInternal<B>,
     D: core::iter::Iterator<Item = <S as crate::traits::Sysex<B>>::Byte>,
+    R: core::ops::RangeBounds<usize>,
 >(
     sysex: &mut S,
     data: D,
-    before: usize,
+    range: R,
 ) -> core::result::Result<(), crate::error::BufferOverflow> {
-    match detail::try_insert_sysex_data(sysex, data, |s, sz| s.try_resize(sz), before) {
+    match detail::try_splice_sysex_data(sysex, data, |s, sz| s.try_resize(sz), range) {
         Err(e) => {
             // if the write failed we reset the message
             // back to zero data
@@ -111,23 +112,24 @@ pub fn try_insert_sysex_data<
     }
 }
 
-pub fn insert_sysex_data<
+pub fn splice_sysex_data<
     B: crate::buffer::Buffer + crate::buffer::BufferMut + crate::buffer::BufferResize,
     S: SysexInternal<B>,
     D: core::iter::Iterator<Item = <S as crate::traits::Sysex<B>>::Byte>,
+    R: core::ops::RangeBounds<usize>,
 >(
     sysex: &mut S,
     data: D,
-    before: usize,
+    range: R,
 ) {
-    detail::try_insert_sysex_data(
+    detail::try_splice_sysex_data(
         sysex,
         data,
         |s, sz| {
             s.resize(sz);
             Ok(())
         },
-        before,
+        range,
     )
     .expect("Resizable buffers should not fail here")
 }
@@ -137,23 +139,37 @@ mod detail {
 
     use super::*;
 
-    pub fn try_insert_sysex_data<
+    pub fn try_splice_sysex_data<
         B: crate::buffer::Buffer + crate::buffer::BufferMut,
         S: crate::traits::SysexInternal<B>,
         D: core::iter::Iterator<Item = <S as crate::traits::Sysex<B>>::Byte>,
         R: Fn(&mut S, usize) -> core::result::Result<(), SysexTryResizeError>,
+        Rg: core::ops::RangeBounds<usize>,
     >(
         sysex: &mut S,
         data: D,
         resize: R,
-        before: usize,
+        range: Rg,
     ) -> core::result::Result<(), crate::error::BufferOverflow> {
         // reformat first to ensure data is optimally filling the
         // underlying buffer
         sysex.compact();
 
         // get an initial estimate for the size of the data
         let initial_size = sysex.payload_size();
+
+        let splice_begin = match range.start_bound() {
+            core::ops::Bound::Included(&v) => v,
+            core::ops::Bound::Excluded(&v) => v + 1,
+            core::ops::Bound::Unbounded => 0,
+        };
+        let splice_end = match range.end_bound() {
+            core::ops::Bound::Included(&v) => v + 1,
+            core::ops::Bound::Excluded(&v) => v,
+            core::ops::Bound::Unbounded => initial_size,
+        };
+        let splice_size = splice_end - splice_begin;
+
         let mut running_data_size_estimate = match data.size_hint() {
             (_, Some(upper)) => upper,
             // not the optimal case - could lead to additional copying
@@ -163,27 +179,26 @@ mod detail {
         let mut additional_size_for_overflow = 1;
         let mut data = data.peekable();
 
-        // initial buffer resize
-        if let Err(SysexTryResizeError(sz)) =
-            resize(sysex, running_data_size_estimate + initial_size)
-        {
-            // failed. we'll work with what we've got
-            running_data_size_estimate = sz.saturating_sub(initial_size);
-        };
-
-        debug_assert_eq!(
-            sysex.payload_size(),
-            running_data_size_estimate + initial_size
-        );
+        if splice_end < splice_end + running_data_size_estimate - splice_size {
+            // we need to grow
+            // initial buffer resize
+            if let Err(SysexTryResizeError(sz)) = resize(
+                sysex,
+                running_data_size_estimate + initial_size - splice_size,
+            ) {
+                // failed. we'll work with what we've got
+                running_data_size_estimate = sz.saturating_sub(initial_size - splice_size);
+            };
+        }
 
-        let mut tail = before + running_data_size_estimate;
-        sysex.move_payload_tail(before, tail);
+        let mut tail = splice_end + running_data_size_estimate - splice_size;
+        sysex.move_payload_tail(splice_end, tail);
 
         'main: loop {
             while written < running_data_size_estimate {
                 match data.next() {
                     Some(v) => {
-                        sysex.write_datum(v, before + written);
+                        sysex.write_datum(v, splice_begin + written);
                         written += 1;
                     }
                     None => {
@@ -199,28 +214,41 @@ mod detail {
             }
 
             // we underestimated.
-            // resize to make more space
             running_data_size_estimate += additional_size_for_overflow;
-            if let Err(SysexTryResizeError(sz)) =
-                resize(sysex, running_data_size_estimate + initial_size)
+
             {
-                // failed. we'll work with what we've got
-                running_data_size_estimate = sz.saturating_sub(initial_size);
-            };
-            sysex.move_payload_tail(tail, before + running_data_size_estimate);
-            tail = before + running_data_size_estimate;
+                let mut to = splice_begin + running_data_size_estimate;
+                if tail < to {
+                    // we need to grow
+                    // resize to make more space
+                    // and move tail
+
+                    if let Err(SysexTryResizeError(sz)) = resize(
+                        sysex,
+                        running_data_size_estimate + initial_size - splice_size,
+                    ) {
+                        // failed. we'll work with what we've got
+                        running_data_size_estimate = sz.saturating_sub(initial_size - splice_size);
+                        to = splice_begin + running_data_size_estimate - splice_size;
+                    };
+                }
+
+                sysex.move_payload_tail(tail, to);
+                tail = splice_begin + running_data_size_estimate;
+            }
+
             additional_size_for_overflow *= 2;
 
             if written >= running_data_size_estimate {
                 return Err(BufferOverflow);
             }
         }
 
-        if written < running_data_size_estimate {
-            // we shrink the buffer back down to the correct size
-            sysex.move_payload_tail(tail, before + written);
-            resize(sysex, written + initial_size).map_err(|_| crate::error::BufferOverflow)?;
-        }
+        // we ensure the buffer is the correct size and move the tail
+        // to the final position
+        sysex.move_payload_tail(tail, splice_begin + written);
+        resize(sysex, written + initial_size - splice_size)
+            .map_err(|_| crate::error::BufferOverflow)?;
 
         Ok(())
     }

midi2/src/detail/test_support/rubbish_payload_iterator.rs

@@ -1,22 +1,37 @@
 /// This is for testing payload insertion implementation on sysex messages.
 /// The iterator returns no size hints so the optimisation case for these
 /// payload insertion implementations will hit their worst case for mem-copying.
-pub struct RubbishPayloadIterator(u8);
+pub struct RubbishPayloadIterator<I: core::iter::Iterator<Item = u8>>(I);
 
-impl RubbishPayloadIterator {
+const DEFAULT_DATA: [u8; 50] = [
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25,
+    26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49,
+];
+
+impl RubbishPayloadIterator<core::iter::Cloned<core::slice::Iter<'static, u8>>> {
     pub fn new() -> Self {
-        RubbishPayloadIterator(0)
+        RubbishPayloadIterator(DEFAULT_DATA.iter().cloned())
+    }
+}
+
+impl<I: core::iter::Iterator<Item = u8>> core::convert::From<I> for RubbishPayloadIterator<I> {
+    fn from(iter: I) -> Self {
+        RubbishPayloadIterator(iter)
     }
 }
 
-impl core::iter::Iterator for RubbishPayloadIterator {
+impl<I: core::iter::Iterator<Item = u8>> core::iter::Iterator for RubbishPayloadIterator<I> {
     type Item = u8;
     fn next(&mut self) -> Option<Self::Item> {
-        if self.0 == 50 {
-            return None;
-        }
-        let ret = Some(self.0);
-        self.0 += 1;
-        ret
+        self.0.next()
+    }
+}
+
+mod tests {
+    use super::*;
+
+    #[test]
+    fn rubbish_iterator_should_give_worst_case_bounds() {
+        assert_eq!(RubbishPayloadIterator::new().size_hint(), (0, None));
     }
 }