Skip to content

Commit

Permalink
fix(kad): potentially incorrect return value of Addresses::remove
Browse files Browse the repository at this point in the history
  • Loading branch information
stormshield-frb committed Nov 8, 2023
1 parent dbfda10 commit 3d2d68e
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 1 deletion.
6 changes: 6 additions & 0 deletions protocols/kad/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 0.45.1

- Fix a bug where calling `remove` on `Addresses` with a non present `address` would return an error
potentially causing the removal of a `Peer` from the DHT.
See [PR 4816](https://github.com/libp2p/rust-libp2p/pull/4816)

## 0.45.0

- Remove deprecated `kad::Config::set_connection_idle_timeout` in favor of `SwarmBuilder::idle_connection_timeout`.
Expand Down
48 changes: 47 additions & 1 deletion protocols/kad/src/addresses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl Addresses {
/// otherwise unreachable.
#[allow(clippy::result_unit_err)]
pub fn remove(&mut self, addr: &Multiaddr) -> Result<(), ()> {
if self.addrs.len() == 1 {
if self.addrs.len() == 1 && self.addrs[0] == *addr {
return Err(());
}

Expand Down Expand Up @@ -113,3 +113,49 @@ impl fmt::Debug for Addresses {
f.debug_list().entries(self.addrs.iter()).finish()
}
}

#[cfg(test)]
mod tests {
use super::*;

/// Helper function to create a tcp Multiaddr with a specific port
fn tcp_addr(port: &str) -> Multiaddr {
format!("/ip4/127.0.0.1/tcp/{port}").parse().unwrap()
}

#[test]
fn given_one_address_when_removing_different_one_returns_ok() {
let mut addresses = Addresses::new(tcp_addr("1234"));

assert!(addresses.remove(&tcp_addr("4321")).is_ok());
assert_eq!(addresses.addrs.len(), 1);
}

#[test]
fn given_one_address_when_removing_correct_one_returns_err() {
let mut addresses = Addresses::new(tcp_addr("1234"));

assert!(addresses.remove(&tcp_addr("1234")).is_err());
assert_eq!(addresses.addrs.len(), 1);
}

#[test]
fn given_many_addresses_when_removing_different_one_does_not_remove_and_returns_ok() {
let mut addresses = Addresses::new(tcp_addr("1234"));
addresses.insert(tcp_addr("4321"));
assert_eq!(addresses.addrs.len(), 2);

assert!(addresses.remove(&tcp_addr("5678")).is_ok());
assert_eq!(addresses.addrs.len(), 2);
}

#[test]
fn given_many_addresses_when_removing_correct_one_removes_and_returns_ok() {
let mut addresses = Addresses::new(tcp_addr("1234"));
addresses.insert(tcp_addr("4321"));
assert_eq!(addresses.addrs.len(), 2);

assert!(addresses.remove(&tcp_addr("1234")).is_ok());
assert_eq!(addresses.addrs.len(), 1);
}
}

0 comments on commit 3d2d68e

Please sign in to comment.