kad: potentially wrong returned value when calling Addresses::remove with a non present address

[stormshield-frb]

Summary

There is an incoherence between the documentation of protocols::kad::src::addresses.rs::Addresses::remove() and its implementation.

Indeed, the documentation states that it returns Err(()) if the address is the last remaining address, which cannot be removed. However, when calling remove and there is only one address left, Err(()) is returned no matter what, event if the provided address was not in the self.addrs field.

rust-libp2p/protocols/kad/src/addresses.rs

Lines 69 to 82 in dbfda10

	pub fn remove(&mut self, addr: &Multiaddr) -> Result<(), ()> {
	if self.addrs.len() == 1 {
	return Err(());
	}
	if let Some(pos) = self.addrs.iter().position(|a| a == addr) {
	self.addrs.remove(pos);
	if self.addrs.len() <= self.addrs.inline_size() {
	self.addrs.shrink_to_fit();
	}
	}
	Ok(())
	}

As we can see at line 70, there is no check to determine if the last address is indeed the one provided.

Expected behavior

let addr1 = "/ip4/127.0.0.1/tcp/1234".parse::<Multiaddr>().unwrap();
let addr2 = "/ip4/127.0.0.1/tcp/4321".parse::<Multiaddr>().unwrap();

let mut addresses = Addresses::new(addr1.clone());

// addr2 is not present so calling remove should return Ok(()) and be a no-op as mentioned in the doc:
// "Returns `Ok(())` if the address is either not in the list or was found and removed"
assert_eq!(Ok(()), addresses.remove(&addr2));

// addr1 is present but is the last one so it should return Err(()) and be a no-op as mentioned in the doc:
// "Returns `Err(())` if the address is the last remaining address, which cannot be removed."
assert_eq!(Err(()), addresses.remove(&addr1));

Actual behavior

let addr1 = "/ip4/127.0.0.1/tcp/1234".parse::<Multiaddr>().unwrap();
let addr2 = "/ip4/127.0.0.1/tcp/4321".parse::<Multiaddr>().unwrap();

let mut addresses = Addresses::new(addr1.clone());

// addr2 is not present so calling remove should return Ok(()) and be a no-op as mentioned in the doc:
// "Returns `Ok(())` if the address is either not in the list or was found and removed"
// However, it currently returns Err(()) because there is no check to assert the last remaining address
// does match the provided one
assert_eq!(Err(()), addresses.remove(&addr2));

Relevant log output

No response

Possible Solution

No response

Version

libp2p-kad: 0.45.0 & master

Would you like to work on fixing this bug ?

Yes