From 002da560a7780fb4b9d81c51473a225f3b690dbf Mon Sep 17 00:00:00 2001 From: FujiApple Date: Thu, 2 Jan 2025 19:51:21 +0800 Subject: [PATCH] fix(core): tracer panic for non-default minimum-ttl (#1460) --- crates/trippy-core/src/state.rs | 111 ++++++++++++++++-- .../state/non_default_minimum_ttl.toml | 21 ++++ 2 files changed, 121 insertions(+), 11 deletions(-) create mode 100644 crates/trippy-core/tests/resources/state/non_default_minimum_ttl.toml diff --git a/crates/trippy-core/src/state.rs b/crates/trippy-core/src/state.rs index 1303c1b9..6b296fd8 100644 --- a/crates/trippy-core/src/state.rs +++ b/crates/trippy-core/src/state.rs @@ -528,7 +528,7 @@ impl FlowState { mod state_updater { use crate::state::FlowState; use crate::types::Checksum; - use crate::{NatStatus, ProbeStatus, Round}; + use crate::{NatStatus, ProbeStatus, Round, TimeToLive}; use std::time::Duration; /// Update the state of a `FlowState` from a `Round`. @@ -632,16 +632,9 @@ mod state_updater { hop.last_sequence = awaited.sequence.0; if self.forward_loss { hop.total_backward_lost += 1; - } else { - let remaining = &self.round.probes[index..]; - let all_awaited = remaining - .iter() - .skip(1) - .all(|p| matches!(p, ProbeStatus::Awaited(_) | ProbeStatus::Skipped)); - if all_awaited { - hop.total_forward_lost += 1; - self.forward_loss = true; - } + } else if is_forward_loss(self.round.probes, awaited.ttl) { + hop.total_forward_lost += 1; + self.forward_loss = true; } } ProbeStatus::Failed(failed) => { @@ -665,6 +658,28 @@ mod state_updater { } } + /// Determine if forward loss has occurred at a given time-to-live. + /// + /// This is determined by checking if all probes after the awaited probe are all also awaited. + fn is_forward_loss(probes: &[ProbeStatus], awaited_ttl: TimeToLive) -> bool { + // Skip all probes that have a ttl less than or equal to the awaited ttl. What remains + // are the probes we are interested in. + let mut remaining = probes + .iter() + .skip_while(|p| match p { + ProbeStatus::Awaited(a) => a.ttl <= awaited_ttl, + ProbeStatus::Complete(c) => c.ttl <= awaited_ttl, + ProbeStatus::Failed(f) => f.ttl <= awaited_ttl, + ProbeStatus::NotSent | ProbeStatus::Skipped => true, + }) + .peekable(); + let is_empty = remaining.peek().is_none(); + let all_awaited = + remaining.all(|p| matches!(p, ProbeStatus::Awaited(_) | ProbeStatus::Skipped)); + // If there is at least one probe remaining and all are awaited then we have forward loss. + !is_empty && all_awaited + } + /// Determine the NAT detection status. /// /// Returns a tuple of the NAT detection status and the checksum to use for the next hop. @@ -699,8 +714,81 @@ mod state_updater { #[cfg(test)] mod tests { use super::*; + use crate::probe::ProbeFailed; + use crate::{ + Flags, IcmpPacketType, Port, Probe, ProbeComplete, RoundId, Sequence, TimeToLive, + TraceId, + }; + use std::net::{IpAddr, Ipv4Addr}; + use std::time::SystemTime; use test_case::test_case; + #[test_case(false, &[], 1; "no forward loss no probes ttl 1")] + #[test_case(true, &[('a', 1), ('a', 2)], 1; "forward loss AA ttl 1")] + #[test_case(false, &[('a', 1), ('c', 2)], 1; "no forward loss AC ttl 1")] + #[test_case(false, &[('a', 1), ('f', 2)], 1; "no forward loss AF ttl 1")] + #[test_case(false, &[('a', 1), ('n', 2)], 1; "no forward loss AN ttl 1")] + #[test_case(false, &[('a', 1), ('c', 2), ('a', 3), ('a', 4)], 1; "no forward loss ACAA ttl 1")] + #[test_case(true, &[('a', 1), ('c', 2), ('a', 3), ('a', 4)], 3; "forward loss ACAA ttl 3")] + #[test_case(false, &[('a', 1), ('c', 2), ('a', 3), ('a', 4)], 4; "no forward loss ACAA ttl 4")] + #[test_case(false, &[('a', 1), ('f', 2), ('n', 3), ('a', 4)], 4; "no forward loss AFAN ttl 1")] + #[test_case(true, &[('a', 4), ('a', 5)], 4; "forward loss AA non-default minimum ttl 4")] + #[test_case(false, &[('a', 4), ('c', 5)], 4; "no forward loss AC non-default minimum ttl 4")] + #[test_case(false, &[('a', 4), ('c', 5), ('a', 6), ('a', 7)], 4; "no forward loss ACAA non-default minimum ttl 4")] + #[test_case(true, &[('a', 4), ('c', 5), ('a', 6), ('a', 7)], 6; "forward loss ACAA non-default minimum ttl 6")] + fn test_is_forward_loss(expected: bool, probes: &[(char, u8)], awaited_ttl: u8) { + assert!(awaited_ttl > 0); + let probes = probes + .iter() + .map(|(typ, ttl)| { + assert!(matches!(typ, 'n' | 's' | 'f' | 'a' | 'c')); + if *ttl == awaited_ttl { + assert!(matches!(typ, 'a')); + } + match typ { + 'n' => ProbeStatus::NotSent, + 's' => ProbeStatus::Skipped, + 'f' => ProbeStatus::Failed(ProbeFailed { + sequence: Sequence::default(), + identifier: TraceId::default(), + src_port: Port::default(), + dest_port: Port::default(), + ttl: TimeToLive(*ttl), + round: RoundId::default(), + sent: SystemTime::now(), + }), + 'a' => ProbeStatus::Awaited(Probe { + sequence: Sequence::default(), + identifier: TraceId::default(), + src_port: Port::default(), + dest_port: Port::default(), + ttl: TimeToLive(*ttl), + round: RoundId(0), + sent: SystemTime::now(), + flags: Flags::empty(), + }), + 'c' => ProbeStatus::Complete(ProbeComplete { + sequence: Sequence::default(), + identifier: TraceId::default(), + src_port: Port::default(), + dest_port: Port::default(), + ttl: TimeToLive(*ttl), + round: RoundId::default(), + sent: SystemTime::now(), + host: IpAddr::V4(Ipv4Addr::UNSPECIFIED), + received: SystemTime::now(), + icmp_packet_type: IcmpPacketType::NotApplicable, + expected_udp_checksum: None, + actual_udp_checksum: None, + extensions: None, + }), + _ => unreachable!(), + } + }) + .collect::>(); + assert_eq!(is_forward_loss(&probes, TimeToLive(awaited_ttl)), expected); + } + #[test_case(123, 123, None => (NatStatus::NotDetected, 123); "first hop matching checksum")] #[test_case(123, 321, None => (NatStatus::Detected, 321); "first hop non-matching checksum")] #[test_case(123, 123, Some(123) => (NatStatus::NotDetected, 123); "non-first hop matching checksum match previous")] @@ -903,6 +991,7 @@ mod tests { #[test_case(file!("nat.toml"))] #[test_case(file!("minimal.toml"))] #[test_case(file!("floss_bloss.toml"))] + #[test_case(file!("non_default_minimum_ttl.toml"))] fn test_scenario(scenario: Scenario) { let mut trace = State::new(StateConfig { max_flows: 1, diff --git a/crates/trippy-core/tests/resources/state/non_default_minimum_ttl.toml b/crates/trippy-core/tests/resources/state/non_default_minimum_ttl.toml new file mode 100644 index 00000000..a1c33985 --- /dev/null +++ b/crates/trippy-core/tests/resources/state/non_default_minimum_ttl.toml @@ -0,0 +1,21 @@ +largest_ttl = 5 + +[[rounds]] +probes = [ + "4 A 333 10.1.0.1 0 12340 80 0 0", + "5 A 777 10.1.0.2 1 12340 80 0 0", +] + +[[expected.hops]] +ttl = 4 +total_sent = 1 +total_recv = 0 +total_forward_loss = 1 +total_backward_loss = 0 + +[[expected.hops]] +ttl = 5 +total_sent = 1 +total_recv = 0 +total_forward_loss = 0 +total_backward_loss = 1