From e6a57baee6c0973abfb09b1df61bf1eafcf0c5d1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 11 Mar 2020 11:50:40 +0100 Subject: [PATCH 1/5] protocols/kad: Add test to reproduce right shift overflow panic --- protocols/kad/src/behaviour/test.rs | 45 ++++++++++++++++++++++++++++- protocols/kad/src/handler.rs | 9 ++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 274e5e17af5..0615457ae6d 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -22,8 +22,9 @@ use super::*; +use crate::handler::KademliaRequestId; use crate::K_VALUE; -use crate::kbucket::Distance; +use crate::kbucket::{Distance, Key}; use crate::record::store::MemoryStore; use futures::{ prelude::*, @@ -31,6 +32,7 @@ use futures::{ future::poll_fn, }; use libp2p_core::{ + connection::ConnectionId, PeerId, Transport, identity, @@ -682,3 +684,44 @@ fn exceed_jobs_max_queries() { }) ) } + +#[test] +// Within `Behaviour::record_received` the exponentially decreasing expiration +// based on the distance to the target for a record is calculated as following: +// +// 1. Calculate the amount of nodes between us and the record key beyond the k +// replication constant as `n`. +// +// 2. Shift the configured record time-to-live `n` times to the right to +// calculate an exponentially decreasing expiration. +// +// The configured record time-to-live is a u64. If `n` is larger or equal to 64 +// the right shift will lead to an overflow which panics in debug mode. +fn record_received_no_right_shift_overflow() { + // This will likely generate enough keys to have equal to or more than 64 + // peers between the record key and the node's local key. + let mut keys = (0..100000).map(|_| PeerId::random()).collect::>(); + keys.sort_by(|a, b| a.as_ref().cmp(b.as_ref())); + + let local_id = keys.remove(0); + let target = keys.pop().unwrap(); + + let store = MemoryStore::new(local_id.clone()); + let mut kad = Kademlia::new(local_id, store); + + for key in keys.into_iter() { + kad.add_address( + &key, + "/ip4/127.0.0.1/tcp/1234".parse::().unwrap(), + ); + } + + let connection_id = ConnectionId::new(1234); + let request_id = KademliaRequestId::default(); + let record = Record::new( + record::Key::from(target.clone().into_bytes()), + vec![], + ); + + kad.record_received(target, connection_id, request_id, record); +} diff --git a/protocols/kad/src/handler.rs b/protocols/kad/src/handler.rs index ac2430f0ae1..660c66ce275 100644 --- a/protocols/kad/src/handler.rs +++ b/protocols/kad/src/handler.rs @@ -364,6 +364,15 @@ pub struct KademliaRequestId { connec_unique_id: UniqueConnecId, } +#[cfg(test)] +impl Default for KademliaRequestId { + fn default() -> Self { + KademliaRequestId { + connec_unique_id: UniqueConnecId(0), + } + } +} + /// Unique identifier for a connection. #[derive(Debug, Copy, Clone, PartialEq, Eq)] struct UniqueConnecId(u64); From aaaec36187408c3abc4cd5014a26ea7febefa130 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 11 Mar 2020 11:58:31 +0100 Subject: [PATCH 2/5] protocols/kad: Fix right shift overflow panic in record_received Within `Behaviour::record_received` the exponentially decreasing expiration based on the distance to the target for a record is calculated as following: 1. Calculate the amount of nodes between us and the record key beyond the k replication constant as `n`. 2. Shift the configured record time-to-live `n` times to the right to calculate an exponentially decreasing expiration. The configured record time-to-live is a u64. If `n` is larger or equal to 64 the right shift will lead to an overflow which panics in debug mode. This patch uses a checked right shift instead, defaulting to 0 (`now + 0`) for the expiration on overflow. --- protocols/kad/src/behaviour.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 0c04e5da224..eea936cf912 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -957,7 +957,7 @@ where let k = self.queries.config().replication_factor.get(); let num_beyond_k = (usize::max(k, num_between) - k) as u32; let expiration = self.record_ttl.map(|ttl| - now + Duration::from_secs(ttl.as_secs() >> num_beyond_k) + now + Duration::from_secs(ttl.as_secs().checked_shr(num_beyond_k).unwrap_or(0)) ); // The smaller TTL prevails. Only if neither TTL is set is the record // stored "forever". @@ -1911,4 +1911,3 @@ impl QueryInfo { } } } - From a3271c2543ca3ad027125fae54b440921536df12 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 12 Mar 2020 14:46:57 +0100 Subject: [PATCH 3/5] protocols/kad: Put attribute below comment --- protocols/kad/src/behaviour/test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index 0615457ae6d..c1dac0b289e 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -685,7 +685,6 @@ fn exceed_jobs_max_queries() { ) } -#[test] // Within `Behaviour::record_received` the exponentially decreasing expiration // based on the distance to the target for a record is calculated as following: // @@ -697,6 +696,7 @@ fn exceed_jobs_max_queries() { // // The configured record time-to-live is a u64. If `n` is larger or equal to 64 // the right shift will lead to an overflow which panics in debug mode. +#[test] fn record_received_no_right_shift_overflow() { // This will likely generate enough keys to have equal to or more than 64 // peers between the record key and the node's local key. From 9e27e253a9363378021fcf6dc1978be2a17e4f35 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 16 Mar 2020 12:16:28 +0100 Subject: [PATCH 4/5] protocols/kad: Extract shifting logic and rework test Extract right shift into isolated function and replace complex regression test with small isolated one. --- protocols/kad/src/behaviour.rs | 15 ++++++---- protocols/kad/src/behaviour/test.rs | 45 +++++------------------------ protocols/kad/src/handler.rs | 9 ------ 3 files changed, 17 insertions(+), 52 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index eea936cf912..11b847c5aa4 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -946,8 +946,6 @@ where return } - let now = Instant::now(); - // Calculate the expiration exponentially inversely proportional to the // number of nodes between the local node and the closest node to the key // (beyond the replication factor). This ensures avoiding over-caching @@ -956,9 +954,7 @@ where let num_between = self.kbuckets.count_nodes_between(&target); let k = self.queries.config().replication_factor.get(); let num_beyond_k = (usize::max(k, num_between) - k) as u32; - let expiration = self.record_ttl.map(|ttl| - now + Duration::from_secs(ttl.as_secs().checked_shr(num_beyond_k).unwrap_or(0)) - ); + let expiration = exp_decr_expiration(self.record_ttl, num_beyond_k); // The smaller TTL prevails. Only if neither TTL is set is the record // stored "forever". record.expires = record.expires.or(expiration).min(expiration); @@ -1030,6 +1026,15 @@ where } } +/// Calculate exponentially decreasing expiration from a default time-to-live by a factor. +fn exp_decr_expiration(default_ttl: Option, factor: u32) -> Option { + default_ttl.map(|ttl| Instant::now() + Duration::from_secs( + ttl.as_secs() + .checked_shr(factor) + .unwrap_or(0), + )) +} + impl NetworkBehaviour for Kademlia where for<'a> TStore: RecordStore<'a>, diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index c1dac0b289e..d6911029009 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -22,9 +22,8 @@ use super::*; -use crate::handler::KademliaRequestId; use crate::K_VALUE; -use crate::kbucket::{Distance, Key}; +use crate::kbucket::Distance; use crate::record::store::MemoryStore; use futures::{ prelude::*, @@ -32,7 +31,6 @@ use futures::{ future::poll_fn, }; use libp2p_core::{ - connection::ConnectionId, PeerId, Transport, identity, @@ -685,43 +683,14 @@ fn exceed_jobs_max_queries() { ) } -// Within `Behaviour::record_received` the exponentially decreasing expiration -// based on the distance to the target for a record is calculated as following: -// -// 1. Calculate the amount of nodes between us and the record key beyond the k -// replication constant as `n`. -// -// 2. Shift the configured record time-to-live `n` times to the right to -// calculate an exponentially decreasing expiration. -// -// The configured record time-to-live is a u64. If `n` is larger or equal to 64 -// the right shift will lead to an overflow which panics in debug mode. #[test] -fn record_received_no_right_shift_overflow() { - // This will likely generate enough keys to have equal to or more than 64 - // peers between the record key and the node's local key. - let mut keys = (0..100000).map(|_| PeerId::random()).collect::>(); - keys.sort_by(|a, b| a.as_ref().cmp(b.as_ref())); - - let local_id = keys.remove(0); - let target = keys.pop().unwrap(); - - let store = MemoryStore::new(local_id.clone()); - let mut kad = Kademlia::new(local_id, store); - - for key in keys.into_iter() { - kad.add_address( - &key, - "/ip4/127.0.0.1/tcp/1234".parse::().unwrap(), - ); +fn exp_decr_expiration_overflow() { + fn prop_no_panic(ttl: Option, factor: u32) { + exp_decr_expiration(ttl, factor); } - let connection_id = ConnectionId::new(1234); - let request_id = KademliaRequestId::default(); - let record = Record::new( - record::Key::from(target.clone().into_bytes()), - vec![], - ); + // Right shifting a u64 by >63 results in a panic. + prop_no_panic(KademliaConfig::default().record_ttl, 64); - kad.record_received(target, connection_id, request_id, record); + quickcheck(prop_no_panic as fn(_, _)) } diff --git a/protocols/kad/src/handler.rs b/protocols/kad/src/handler.rs index 660c66ce275..ac2430f0ae1 100644 --- a/protocols/kad/src/handler.rs +++ b/protocols/kad/src/handler.rs @@ -364,15 +364,6 @@ pub struct KademliaRequestId { connec_unique_id: UniqueConnecId, } -#[cfg(test)] -impl Default for KademliaRequestId { - fn default() -> Self { - KademliaRequestId { - connec_unique_id: UniqueConnecId(0), - } - } -} - /// Unique identifier for a connection. #[derive(Debug, Copy, Clone, PartialEq, Eq)] struct UniqueConnecId(u64); From 36f6b9372c2288adfcc3620f98359f9eb679e245 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 17 Mar 2020 19:15:04 +0100 Subject: [PATCH 5/5] protocols/kad/src/behaviour: Refactor exp_decr_expiration --- protocols/kad/src/behaviour.rs | 14 ++++++-------- protocols/kad/src/behaviour/test.rs | 6 +++--- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 11b847c5aa4..be51c3dcf09 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -946,6 +946,8 @@ where return } + let now = Instant::now(); + // Calculate the expiration exponentially inversely proportional to the // number of nodes between the local node and the closest node to the key // (beyond the replication factor). This ensures avoiding over-caching @@ -954,7 +956,7 @@ where let num_between = self.kbuckets.count_nodes_between(&target); let k = self.queries.config().replication_factor.get(); let num_beyond_k = (usize::max(k, num_between) - k) as u32; - let expiration = exp_decr_expiration(self.record_ttl, num_beyond_k); + let expiration = self.record_ttl.map(|ttl| now + exp_decrease(ttl, num_beyond_k)); // The smaller TTL prevails. Only if neither TTL is set is the record // stored "forever". record.expires = record.expires.or(expiration).min(expiration); @@ -1026,13 +1028,9 @@ where } } -/// Calculate exponentially decreasing expiration from a default time-to-live by a factor. -fn exp_decr_expiration(default_ttl: Option, factor: u32) -> Option { - default_ttl.map(|ttl| Instant::now() + Duration::from_secs( - ttl.as_secs() - .checked_shr(factor) - .unwrap_or(0), - )) +/// Exponentially decrease the given duration (base 2). +fn exp_decrease(ttl: Duration, exp: u32) -> Duration { + Duration::from_secs(ttl.as_secs().checked_shr(exp).unwrap_or(0)) } impl NetworkBehaviour for Kademlia diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index d6911029009..224368118c0 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -685,12 +685,12 @@ fn exceed_jobs_max_queries() { #[test] fn exp_decr_expiration_overflow() { - fn prop_no_panic(ttl: Option, factor: u32) { - exp_decr_expiration(ttl, factor); + fn prop_no_panic(ttl: Duration, factor: u32) { + exp_decrease(ttl, factor); } // Right shifting a u64 by >63 results in a panic. - prop_no_panic(KademliaConfig::default().record_ttl, 64); + prop_no_panic(KademliaConfig::default().record_ttl.unwrap(), 64); quickcheck(prop_no_panic as fn(_, _)) }