src: codec: Fix SKIP_CRC_VALIDATION panic and hygiene

.gitignore

@@ -1 +1,2 @@
 /target
+/dhat-heap.json

src/codec.rs

@@ -10,6 +10,17 @@ use crate::{
     Packet,
 };
 
+/// MAVLink packet codec whose behavior is selected at compile time through
+/// const-generic toggles.
+///
+/// The toggles are, in order:
+///
+/// * `ACCEPT_V1` -- accept MAVLink v1 frames.
+/// * `ACCEPT_V2` -- accept MAVLink v2 frames.
+/// * `DROP_INVALID_SYSID` -- reject frames whose system id equals zero.
+/// * `DROP_INVALID_COMPID` -- reject frames whose component id equals zero.
+/// * `SKIP_CRC_VALIDATION` -- skip **only** the CRC computation step.
+/// * `DROP_INCOMPATIBLE` -- reject v2 frames with unsupported incompat flags.
 #[derive(Debug, Default)]
 pub struct MavlinkCodec<
     const ACCEPT_V1: bool,
@@ -71,12 +82,6 @@ impl<
                     trace!("Waitig for STX...");
 
                     if buf.is_empty() {
-                        if ACCEPT_V2 {
-                            // buf.reserve(V2Packet::MAX_PACKET_SIZE);
-                        } else {
-                            // buf.reserve(V1Packet::MAX_PACKET_SIZE);
-                        }
-
                         trace!(
                             "Not enough data, buf.len: {:?}, buf.capacity: {:?}",
                             buf.len(),
@@ -98,8 +103,6 @@ impl<
                 // V1 Codec
                 CodecState::WaitingV1PacketHeader if ACCEPT_V1 => {
                     if buf.len() < V1Packet::HEADER_SIZE {
-                        // buf.reserve(V1Packet::HEADER_SIZE);
-
                         trace!(
                             "Not enough data, buf.len: {:?}, buf.capacity: {:?}",
                             buf.len(),
@@ -113,8 +116,6 @@ impl<
                 }
                 CodecState::ValidatingV1Packet { packet_size } if ACCEPT_V1 => {
                     if buf.len() < packet_size {
-                        // buf.reserve(V1Packet::MAX_PACKET_SIZE);
-
                         trace!(
                             "Not enough data, buf.len: {:?}, buf.capacity: {:?}",
                             buf.len(),
@@ -186,22 +187,7 @@ impl<
                     self.state = CodecState::CopyV1Packet { packet_size };
                 }
                 CodecState::CopyV1Packet { packet_size } if ACCEPT_V1 => {
-                    let buf_packet = if SKIP_CRC_VALIDATION {
-                        // Copy the entire packet consuming the source buffer
-                        let mut buf_packet = BytesMut::with_capacity(packet_size);
-                        buf_packet[..packet_size].copy_from_slice(&buf[..packet_size]);
-
-                        // Since it is a non validated packet, there might be other packets within this buffer, so we can only discard this STX
-                        buf.advance(V1Packet::STX_SIZE);
-
-                        buf_packet
-                    } else {
-                        let buf_packet = buf.split_to(packet_size);
-                        // buf.reserve(V1Packet::MAX_PACKET_SIZE);
-
-                        buf_packet
-                    };
-
+                    let buf_packet = buf.split_to(packet_size);
                     let packet = V1Packet {
                         buffer: buf_packet.freeze(),
                     };
@@ -212,8 +198,6 @@ impl<
                 // V2 Codec
                 CodecState::WaitingV2PacketHeader if ACCEPT_V2 => {
                     if buf.len() < V2Packet::HEADER_SIZE {
-                        // buf.reserve(V2Packet::HEADER_SIZE);
-
                         trace!(
                             "Not enough data, buf.len: {:?}, buf.capacity: {:?}",
                             buf.len(),
@@ -225,7 +209,7 @@ impl<
                     if DROP_INCOMPATIBLE {
                         let incompat_flags = *v2::incompat_flags(buf);
                         if incompat_flags & !MAVLINK_SUPPORTED_IFLAGS > 0 {
-                            buf.advance(V1Packet::STX_SIZE); // Discard this STX
+                            buf.advance(V2Packet::STX_SIZE); // Discard this STX
                             self.state = CodecState::WaitingForStx;
 
                             return Ok(Some(Err(DecoderError::Incompatible { incompat_flags })));
@@ -237,8 +221,6 @@ impl<
                 }
                 CodecState::ValidatingV2Packet { packet_size } if ACCEPT_V2 => {
                     if buf.len() < packet_size {
-                        // buf.reserve(V2Packet::MAX_PACKET_SIZE);
-
                         trace!(
                             "Not enough data, buf.len: {:?}, buf.capacity: {:?}",
                             buf.len(),
@@ -310,21 +292,7 @@ impl<
                     self.state = CodecState::CopyV2Packet { packet_size };
                 }
                 CodecState::CopyV2Packet { packet_size } if ACCEPT_V2 => {
-                    let buf_packet = if SKIP_CRC_VALIDATION {
-                        // Copy the entire packet consuming the source buffer
-                        let mut buf_packet = BytesMut::with_capacity(packet_size);
-                        buf_packet[..packet_size].copy_from_slice(&buf[..packet_size]);
-
-                        // Since it is a non validated packet, there might be other packets within this buffer, so we can only discard this STX
-                        buf.advance(V2Packet::STX_SIZE);
-
-                        buf_packet
-                    } else {
-                        let buf_packet = buf.split_to(packet_size);
-                        // buf.reserve(V2Packet::MAX_PACKET_SIZE);
-
-                        buf_packet
-                    };
+                    let buf_packet = buf.split_to(packet_size);
 
                     let packet = V2Packet {
                         buffer: buf_packet.freeze(),

src/v2.rs

@@ -175,9 +175,11 @@ pub(crate) fn packet_size<T: AsRef<[u8]>>(buf: &T) -> usize {
     let header = V2Packet::HEADER_SIZE;
     let payload = *len(buf) as usize;
     let checksum = V2Packet::CHECKSUM_SIZE;
-    let signature = has_signature(buf)
-        .then_some(V2Packet::SIGNATURE_SIZE)
-        .unwrap_or_default();
+    let signature = if has_signature(buf) {
+        V2Packet::SIGNATURE_SIZE
+    } else {
+        Default::default()
+    };
 
     stx + header + payload + checksum + signature
 }

tests/skip_crc_test.rs

@@ -0,0 +1,139 @@
+use bytes::BytesMut;
+use dev_utils::{create_random_v1_raw_message, create_random_v2_raw_message};
+use mavlink_codec::{codec::MavlinkCodec, error::DecoderError, Packet};
+use rand::{rngs::StdRng, SeedableRng};
+use tokio_util::codec::Decoder;
+
+const SEED: u64 = 42;
+
+type SkipV1Codec = MavlinkCodec<true, false, false, false, true, false>;
+type SkipV2Codec = MavlinkCodec<false, true, false, false, true, false>;
+type StrictV1Codec = MavlinkCodec<true, false, false, false, false, false>;
+type StrictV2Codec = MavlinkCodec<false, true, false, false, false, false>;
+
+fn corrupt_crc(buf: &mut [u8]) {
+    let len = buf.len();
+    assert!(len >= 2, "packet must have at least two bytes to corrupt");
+    buf[len - 2] = buf[len - 2].wrapping_add(1);
+    buf[len - 1] = buf[len - 1].wrapping_add(1);
+}
+
+#[test]
+fn skip_crc_v1_decodes_valid_packet() {
+    let mut rng: StdRng = SeedableRng::seed_from_u64(SEED);
+    let raw = create_random_v1_raw_message(&mut rng);
+    let total = raw.raw_bytes().len();
+    let mut buf = BytesMut::from(raw.raw_bytes());
+
+    let mut codec = SkipV1Codec::default();
+    let decoded = codec.decode(&mut buf).unwrap();
+
+    assert!(
+        matches!(decoded, Some(Ok(Packet::V1(_)))),
+        "expected Packet::V1, got {decoded:?}"
+    );
+    assert!(
+        buf.is_empty(),
+        "F3: {} of {total} bytes remained after a single successful decode",
+        buf.len()
+    );
+    assert!(
+        codec.decode(&mut buf).unwrap().is_none(),
+        "F3: decoder emitted a ghost packet from bytes that should already have been consumed"
+    );
+}
+
+#[test]
+fn skip_crc_v2_decodes_valid_packet() {
+    let mut rng: StdRng = SeedableRng::seed_from_u64(SEED);
+    let raw = create_random_v2_raw_message(&mut rng);
+    let total = raw.raw_bytes().len();
+    let mut buf = BytesMut::from(raw.raw_bytes());
+
+    let mut codec = SkipV2Codec::default();
+    let decoded = codec.decode(&mut buf).unwrap();
+
+    assert!(
+        matches!(decoded, Some(Ok(Packet::V2(_)))),
+        "expected Packet::V2, got {decoded:?}"
+    );
+    assert!(
+        buf.is_empty(),
+        "F3: {} of {total} bytes remained after a single successful decode",
+        buf.len()
+    );
+    assert!(
+        codec.decode(&mut buf).unwrap().is_none(),
+        "F3: decoder emitted a ghost packet from bytes that should already have been consumed"
+    );
+}
+
+#[test]
+fn skip_crc_v1_accepts_corrupted_crc() {
+    let mut rng: StdRng = SeedableRng::seed_from_u64(SEED);
+    let raw = create_random_v1_raw_message(&mut rng);
+
+    let mut corrupted: Vec<u8> = raw.raw_bytes().to_vec();
+    corrupt_crc(&mut corrupted);
+
+    // Skip codec must accept despite the broken CRC.
+    {
+        let mut buf = BytesMut::from(corrupted.as_slice());
+        let mut codec = SkipV1Codec::default();
+        let decoded = codec.decode(&mut buf).unwrap();
+        assert!(
+            matches!(decoded, Some(Ok(Packet::V1(_)))),
+            "skip-CRC codec must accept a packet with corrupted CRC, got {decoded:?}"
+        );
+        assert!(
+            buf.is_empty(),
+            "F3: {} bytes remained after decoding a corrupted-CRC packet under SKIP_CRC_VALIDATION",
+            buf.len()
+        );
+    }
+
+    // Non-skip codec must reject with InvalidCRC, proving the toggle's scope.
+    {
+        let mut buf = BytesMut::from(corrupted.as_slice());
+        let mut codec = StrictV1Codec::default();
+        let decoded = codec.decode(&mut buf).unwrap();
+        assert!(
+            matches!(decoded, Some(Err(DecoderError::InvalidCRC { .. }))),
+            "strict codec must reject corrupted CRC, got {decoded:?}"
+        );
+    }
+}
+
+#[test]
+fn skip_crc_v2_accepts_corrupted_crc() {
+    let mut rng: StdRng = SeedableRng::seed_from_u64(SEED);
+    let raw = create_random_v2_raw_message(&mut rng);
+
+    let mut corrupted: Vec<u8> = raw.raw_bytes().to_vec();
+    corrupt_crc(&mut corrupted);
+
+    {
+        let mut buf = BytesMut::from(corrupted.as_slice());
+        let mut codec = SkipV2Codec::default();
+        let decoded = codec.decode(&mut buf).unwrap();
+        assert!(
+            matches!(decoded, Some(Ok(Packet::V2(_)))),
+            "skip-CRC codec must accept a packet with corrupted CRC, got {decoded:?}"
+        );
+        assert!(
+            buf.is_empty(),
+            "F3: {} bytes remained after decoding a corrupted-CRC packet under SKIP_CRC_VALIDATION",
+            buf.len()
+        );
+    }
+
+    {
+        let mut buf = BytesMut::from(corrupted.as_slice());
+        let mut codec = StrictV2Codec::default();
+        let decoded = codec.decode(&mut buf).unwrap();
+        assert!(
+            matches!(decoded, Some(Err(DecoderError::InvalidCRC { .. }))),
+            "strict codec must reject corrupted CRC, got {decoded:?}"
+        );
+    }
+}