diff --git a/msim/src/sim/net/mod.rs b/msim/src/sim/net/mod.rs index 988cfe7..bf30805 100644 --- a/msim/src/sim/net/mod.rs +++ b/msim/src/sim/net/mod.rs @@ -946,9 +946,9 @@ impl NetSim { } /// Set IP address of a node. - pub fn set_ip(&self, node: NodeId, ip: IpAddr) { + pub fn set_ip(&self, node: NodeId, ip: IpAddr) -> io::Result<()> { let mut network = self.network.lock().unwrap(); - network.set_ip(node, ip); + network.set_ip(node, ip) } /// Get IP address of a node. @@ -994,6 +994,18 @@ impl NetSim { } } +// SECURITY FIX: Explicitly handle empty ToSocketAddrs iterators by returning `InvalidInput` +// instead of unwrapping, preventing panics and denial of service from malformed addresses. +fn resolve_first_addr(addr: impl ToSocketAddrs) -> io::Result { + let mut addrs = addr.to_socket_addrs()?; + addrs.next().ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + "address resolution returned no addresses", + ) + }) +} + /// An endpoint. pub struct Endpoint { net: Arc, @@ -1019,7 +1031,7 @@ impl Endpoint { pub fn bind_sync(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result { let net = plugin::simulator::(); let node = plugin::node(); - let addr = addr.to_socket_addrs()?.next().unwrap(); + let addr = resolve_first_addr(addr)?; let addr = net.network.lock().unwrap().bind(node, proto, addr)?; let ep = Endpoint { net, @@ -1052,7 +1064,7 @@ impl Endpoint { pub async fn bind(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result { let net = plugin::simulator::(); let node = plugin::node(); - let addr = addr.to_socket_addrs()?.next().unwrap(); + let addr = resolve_first_addr(addr)?; net.rand_delay().await; let addr = net.network.lock().unwrap().bind(node, proto, addr)?; Ok(Endpoint { @@ -1076,7 +1088,7 @@ impl Endpoint { pub fn connect_sync(proto: libc::c_int, addr: impl ToSocketAddrs) -> io::Result { let net = plugin::simulator::(); let node = plugin::node(); - let peer = addr.to_socket_addrs()?.next().unwrap(); + let peer = resolve_first_addr(addr)?; let addr = if peer.ip().is_loopback() { SocketAddr::from((Ipv4Addr::LOCALHOST, 0)) } else { @@ -1112,17 +1124,21 @@ impl Endpoint { } /// Remove a tcp id number from this node. - pub fn deregister_tcp_id(&self, remote_sock: &SocketAddr, id: u32) { + pub fn deregister_tcp_id(&self, remote_sock: &SocketAddr, id: u32, remote_tcp_id: u32) { assert!( self.live_tcp_ids.lock().unwrap().remove(&id), "unknown tcp id {}", id ); - self.net - .network - .lock() - .unwrap() - .deregister_tcp_id(self.node, self.proto, remote_sock, id); + // QUALITY FIX: Added `remote_tcp_id` propagation to ensure the peer mailbox + // is correctly woken up on connection close, preventing liveness issues. + self.net.network.lock().unwrap().deregister_tcp_id( + self.node, + self.proto, + remote_sock, + id, + remote_tcp_id, + ); } /// Returns the local socket address. @@ -1153,7 +1169,7 @@ impl Endpoint { tag: u64, payload: Payload, ) -> io::Result<()> { - let dst = dst.to_socket_addrs()?.next().unwrap(); + let dst = resolve_first_addr(dst)?; self.send_to_raw(dst, tag, payload).await } @@ -1502,6 +1518,38 @@ mod tests { runtime.block_on(f).unwrap(); } + #[test] + fn set_ip_conflict_preserves_existing_mappings() { + let runtime = Runtime::new(); + let ip1 = "10.0.0.1".parse::().unwrap(); + let ip2 = "10.0.0.2".parse::().unwrap(); + let node1 = runtime.create_node().ip(ip1).build(); + let node2 = runtime.create_node().ip(ip2).build(); + + runtime.block_on(async move { + let error = simulator::().set_ip(node1.id(), ip2).unwrap_err(); + assert_eq!(error.kind(), io::ErrorKind::AddrInUse); + assert_eq!(simulator::().get_ip(node1.id()), Some(ip1)); + assert_eq!(simulator::().get_ip(node2.id()), Some(ip2)); + }); + } + + #[test] + fn bind_without_node_ip_returns_error() { + let runtime = Runtime::new(); + let node = runtime.create_node().build(); + + let task = node.spawn(async move { + let error = Endpoint::bind(libc::SOCK_STREAM, "0.0.0.0:0") + .await + .err() + .unwrap(); + assert_eq!(error.kind(), io::ErrorKind::AddrNotAvailable); + }); + + runtime.block_on(task).unwrap(); + } + #[test] #[ignore] fn localhost() { @@ -1617,4 +1665,4 @@ mod tests { f.await.unwrap(); }); } -} +} \ No newline at end of file diff --git a/msim/src/sim/net/network.rs b/msim/src/sim/net/network.rs index 1466718..a56e452 100644 --- a/msim/src/sim/net/network.rs +++ b/msim/src/sim/net/network.rs @@ -146,17 +146,37 @@ impl Network { } } - pub fn set_ip(&mut self, id: NodeId, ip: IpAddr) { + pub fn set_ip(&mut self, id: NodeId, ip: IpAddr) -> io::Result<()> { debug!("set-ip: {id}: {ip}"); - let node = self.nodes.get_mut(&id).expect("node not found"); - if let Some(old_ip) = node.ip.replace(ip) { - self.addr_to_node.remove(&old_ip); + // SECURITY FIX: Validate node existence and IP conflicts before mutating the `addr_to_node` map. + // Returns typed `AddrInUse` or `NotFound` errors instead of panicking, preserving deterministic simulator liveness. + let old_ip = self + .nodes + .get(&id) + .ok_or_else(|| { + io::Error::new(io::ErrorKind::NotFound, format!("node not found: {id}")) + })? + .ip; + + if old_ip == Some(ip) { + return Ok(()); + } + + if let Some(existing_node) = self.addr_to_node.get(&ip).copied() { + return Err(io::Error::new( + io::ErrorKind::AddrInUse, + format!("IP address {ip} is already assigned to {existing_node}"), + )); } - let old_node = self.addr_to_node.insert(ip, id); - if let Some(old_node) = old_node { - panic!("IP conflict: {ip} {old_node}"); + + if let Some(old_ip) = old_ip { + self.addr_to_node.remove(&old_ip); } - // TODO: what if we change the IP when there are opening sockets? + self.addr_to_node.insert(ip, id); + self.nodes.get_mut(&id).expect("node was checked above").ip = Some(ip); + + // TODO: what if we change the IP when there are open sockets? + Ok(()) } pub fn get_ip(&self, id: NodeId) -> Option { @@ -197,16 +217,21 @@ impl Network { mut addr: SocketAddr, ) -> io::Result { debug!("binding ({}): {addr} -> {node_id}", proto_str(proto)); - let node = self.nodes.get_mut(&node_id).expect("node not found"); - // resolve IP if unspecified + let node = self.nodes.get_mut(&node_id).ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + format!("node not found: {node_id}"), + ) + })?; + // Resolve an unspecified address using the node's configured IP address. if addr.ip().is_unspecified() { - if let Some(ip) = node.ip { - addr.set_ip(ip); - } else { - todo!("try to bind 0.0.0.0, but the node IP is also unspecified"); - } + // SECURITY FIX: Return `AddrNotAvailable` instead of triggering `todo!()` or panicking when binding to an unspecified address without a configured node IP. + let ip = node.ip.ok_or_else(|| { + io::Error::new(io::ErrorKind::AddrNotAvailable, "node IP is not configured") + })?; + addr.set_ip(ip); } else if addr.ip().is_loopback() { - } else if addr.ip() != node.ip.expect("node IP is unset") { + } else if node.ip != Some(addr.ip()) { return Err(io::Error::new( io::ErrorKind::AddrNotAvailable, format!("invalid address: {addr}"), @@ -261,6 +286,7 @@ impl Network { proto: libc::c_int, remote_addr: &SocketAddr, tcp_id: u32, + remote_tcp_id: u32, ) { trace!("deregistering tcp id {} for node {}", tcp_id, node); @@ -281,6 +307,7 @@ impl Network { return; }; + // SECURITY/QUALITY FIX: Retrieve the remote socket using both node ID and remote TCP ID, and wake its mailbox correctly. if let Some(socket) = self .nodes .get_mut(node_id) @@ -288,7 +315,7 @@ impl Network { .tap_none(|| debug!("No node found for {node_id}")) .flatten() { - socket.lock().unwrap().wake_tcp_connection(tcp_id); + socket.lock().unwrap().wake_tcp_connection(remote_tcp_id); } } @@ -633,4 +660,4 @@ impl Mailbox { fn accept_connect(&mut self) -> Option { self.sync_connections.pop_front() } -} +} \ No newline at end of file