From c000e0a9da18ddd216507c9d4777cfb3b02afbae Mon Sep 17 00:00:00 2001 From: Patrick Crumley Date: Wed, 12 Jul 2023 18:55:57 -0700 Subject: [PATCH] add unit tests for recovering from parsing errors of sbp (#1347) This PR adds a unit test showing that parsing errors fallsback to an invalid message. Seeing this unit test makes it clear why some of the questions @silverjam was having about invoking this on certain inputs causes it to pass and others cause it to fail. Additionally the round-tripping on arbitrary bytes is not possible yet because both the iter_frame & iter_messages will only return a message if there is something that starts with the prelude byte, and it returns the bytes that exist between that prelude and HEADER_LEN, whatever the length of the payload is on byte 5 + the CRC_LEN. This means that if there are extra bytes sent in the stream for aliasing for instance, the framer ignores those bytes, and they are never even tried to be parsed. @silverjam gave two examples that do not throw an error as the unknown fallback is currently implemented. One of those has been added as a unit test here. But the main takeaway is the invalid-fallback as currently written only works on messages that are properly framed but perhaps wrongly labeled or with a CRC error. --- rust/sbp/src/de.rs | 106 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 88 insertions(+), 18 deletions(-) diff --git a/rust/sbp/src/de.rs b/rust/sbp/src/de.rs index 6e2a20d14..f3e4946a5 100644 --- a/rust/sbp/src/de.rs +++ b/rust/sbp/src/de.rs @@ -11,8 +11,8 @@ use dencode::FramedRead; use futures::StreamExt; use crate::{ - messages::invalid::Invalid, messages::SbpMsgParseError, HandleParseError, Sbp, CRC_LEN, - HEADER_LEN, MAX_FRAME_LEN, PAYLOAD_INDEX, PREAMBLE, + messages::{invalid::Invalid, SbpMsgParseError}, + HandleParseError, Sbp, CRC_LEN, HEADER_LEN, MAX_FRAME_LEN, PAYLOAD_INDEX, PREAMBLE, }; /// Deserialize the IO stream into an iterator of messages. @@ -265,6 +265,7 @@ impl dencode::Decoder for FramerImpl { "payload_index must be less than header len" ); let end = HEADER_LEN + src[PAYLOAD_INDEX] as usize + CRC_LEN; + if src.len() < end { src.reserve(MAX_FRAME_LEN); return Ok(None); @@ -435,7 +436,10 @@ mod tests { io::{Cursor, Write}, }; - use crate::{messages::navigation::MsgBaselineEcef, wire_format::WireFormat}; + use crate::{ + messages::{invalid::Invalid, navigation::MsgBaselineEcef}, + wire_format::WireFormat, + }; use super::*; @@ -497,12 +501,27 @@ mod tests { fn test_parse_bad_message() { // Properly framed data but the payload isn't right given the message type let data: Vec = vec![0x55, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x65, 0x8D]; - let bytes = BytesMut::from(&data[..]); - let mut actual = crate::de::Decoder::new(bytes.reader()); - assert!(matches!( - actual.next().unwrap(), - Err(Error::SbpMsgParseError(_)) - )); + let input = Cursor::new(data.clone()); + let mut msgs = iter_messages(input); + if let Some(Err(Error::SbpMsgParseError(e))) = msgs.next() { + let invalid = Invalid::from(e); + assert_eq!( + invalid, + Invalid { + msg_id: Some(257), + sender_id: Some(257), + crc: Some(36197), + invalid_frame: data + } + ) + } else { + panic!( + "expecting unable to parse error because message is framed \ + but not valid" + ); + } + // msgs should be exhausted + assert_eq!(msgs.count(), 0); } #[test] @@ -511,7 +530,7 @@ mod tests { 0x55u8, 0x0b, 0x02, 0xd3, 0x88, 0x14, 0x28, 0xf4, 0x7a, 0x13, 0x96, 0x62, 0xee, 0xff, 0xbe, 0x40, 0x14, 0x00, 0xf6, 0xa3, 0x09, 0x00, 0x00, 0x00, 0x0e, 0x00, 0xdb, 0xbf, ]; - payload.append(&mut payload.clone()); + payload.extend(payload.clone()); let input = Cursor::new(payload); let mut count = 0; for msg in iter_messages(input) { @@ -521,22 +540,73 @@ mod tests { assert_eq!(count, 2); } + #[test] + fn test_no_message_if_improperly_framed() { + let corrupted_input: Vec = vec![ + 0x55, 0x28, 0x61, 0xb2, 0x99, 0xba, 0xd6, 0x1d, 0x42, 0x15, 0xf7, 0x9f, 0x36, 0xf8, + 0xca, 0x72, 0x97, 0x41, 0xaf, 0x29, 0xb9, 0x0b, 0xf1, 0x0e, 0xd3, 0x1d, 0xef, 0xab, + 0xd4, 0x70, 0xac, 0x1e, 0x02, 0x71, 0xa0, 0x59, 0xc1, 0x78, 0x95, 0x5d, 0xf9, 0xe5, + 0xf9, 0x00, 0x6a, 0x38, 0x06, 0x69, 0x06, 0x8d, 0xf1, 0x80, 0xa3, 0xe2, 0xa1, 0x3c, + 0x6b, 0xab, 0x42, 0xe8, 0x18, 0x0a, 0xf0, 0x70, + ]; + let mut msgs = iter_messages(Cursor::new(corrupted_input.clone())); + match msgs.next() { + None if corrupted_input[PAYLOAD_INDEX] as usize > corrupted_input.len() => { + // in theory some day we want to recover here with a invalid message + // however that will require a change to how framed reads are + // taken in + } + _ => { + panic!( + "Expecting None because the data are not large enough to hold the \ + expected frame" + ); + } + } + } #[test] fn test_parse_crc_error() { - let packet = vec![ - // Start with a mostly valid message, with a single byte error - 0x55, 0x0c, // This byte should be 0x0b, changed to intentionally cause a CRC error - 0x02, 0xd3, 0x88, 0x14, 0x28, 0xf4, 0x7a, 0x13, 0x96, 0x62, 0xee, 0xff, 0xbe, 0x40, - 0x14, 0x00, 0xf6, 0xa3, 0x09, 0x00, 0x00, 0x00, 0x0e, 0x00, 0xdb, 0xbf, 0xde, 0xad, - 0xbe, 0xef, // Include another valid message to properly parse + let valid_message = vec![ 0x55, 0x0b, 0x02, 0xd3, 0x88, 0x14, 0x28, 0xf4, 0x7a, 0x13, 0x96, 0x62, 0xee, 0xff, 0xbe, 0x40, 0x14, 0x00, 0xf6, 0xa3, 0x09, 0x00, 0x00, 0x00, 0x0e, 0x00, 0xdb, 0xbf, 0xde, 0xad, 0xbe, 0xef, ]; + + // A mostly valid message + let invalid_message_bytes = { + let mut invalid = valid_message.clone(); + // add one to one of the bytes in the header to intentionally cause a CRC error + invalid[1] += 1; + invalid + }; + + let packet: Vec<_> = invalid_message_bytes + .iter() + .cloned() + .chain(valid_message.into_iter()) + .collect(); + let mut msgs = iter_messages(Cursor::new(packet)); - let res = msgs.next().unwrap().unwrap_err(); - assert!(matches!(res, Error::CrcError(..))); + let crc_err = if let Some(Err(Error::CrcError(inner_err))) = msgs.next() { + inner_err + } else { + panic!("expecting CrcError because CRC is incorrect.") + }; + + let invalid_msg = Invalid::from(crc_err); + assert_eq!( + invalid_msg, + Invalid { + msg_id: Some(524), + sender_id: Some(35027), + crc: Some(49115), + // note that the message here is missing the values in + // 28..32 (not sure if that is desired behavior but hard + // to fix without more breaking changes) + invalid_frame: invalid_message_bytes[0..28].to_vec(), + } + ); let res = msgs.next().unwrap(); assert!(res.is_ok());