From 3101345fe69eb3466e7143be1e657dd43d8753a1 Mon Sep 17 00:00:00 2001 From: Red Bear OS Date: Thu, 9 Jul 2026 00:42:46 +0300 Subject: [PATCH] review: NDP state corruption fix, port double-free fix, observer limits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL/MEDIUM BUGS FIXED: 1. ethernet.rs send_ndp_solicit: state corruption via recursive call The function destructured self.ndp_state by value (target, tries, silent_until) at the start and wrote back at the end. But between destructuring and writing back, self.drop_waiting_packets_v6(target) could recursively call self.send_ndp_solicit(Instant::ZERO) for a different target. The recursive call updated self.ndp_state with the new target's state. When control returned to the original frame, the original write-back clobbered the recursive call's state, silently losing neighbor discovery for the new target. Fix: use scoped pattern matches to read target/tries/silent_until from the live state right before they are needed, so the recursive call's writes are preserved. This matches the pattern used by send_arp (which uses ref mut and is correct). 2. scheme/socket.rs on_close: port double-free close_file() was called before the refcount check, so the port was released on every close. For a dup'd socket, the second close tried to release the port again — double-free. Fix: compute the new refcount first, only call close_file and remove from socket_set when the count reaches 0 (last reference). 3. scheme/tcp.rs new_socket: port + socket leak on connect failure If get_port() succeeded but connect() later failed, the port was claimed and the socket was added to socket_set, but new_socket returned Err. The caller (open_inner) did not insert the file handle, so on_close was never called. Both the port and the socket slot leaked. Fix: when connect() fails, release the auto-allocated port before returning. Explicit user-provided ports are still released by on_close when the last file is dropped (preserves the existing on_close-based release). 4. observer.rs capture: per-packet size limit not enforced Vec::with_capacity only pre-allocates; extend_from_slice copies the full packet. The intended per-packet limit (MAX_CAPTURE_BYTES / max_packets = 256 bytes) was being bypassed, allowing a single 1500-byte packet to consume the full capture buffer. Fix: truncate to per_packet_limit before extending. 5. observer.rs capture: short packets bypassed filter Packets < 20 bytes returned true (capture anyway) regardless of the user's filter. A filter like 'tcp port 80' would capture all short packets, including non-TCP. Fix: return false (no match) for short packets. Filter semantics are now consistent: if a packet can't be matched, it's not captured. All 29 existing tests still pass. --- netstack/src/link/ethernet.rs | 41 +++++++++++++++++++++++------------ netstack/src/observer.rs | 10 ++++++--- netstack/src/scheme/socket.rs | 31 +++++++++++++------------- netstack/src/scheme/tcp.rs | 16 ++++++++++++-- 4 files changed, 64 insertions(+), 34 deletions(-) diff --git a/netstack/src/link/ethernet.rs b/netstack/src/link/ethernet.rs index 21a90be99b..b52effbceb 100644 --- a/netstack/src/link/ethernet.rs +++ b/netstack/src/link/ethernet.rs @@ -469,23 +469,29 @@ impl EthernetLink { return; }; - let (target, mut tries, mut silent_until) = match self.ndp_state { + // Extract target. Do NOT destructure tries/silent_until by value + // — the recursive call into drop_waiting_packets_v6 can modify + // self.ndp_state and a later write-back would clobber that + // update. We read tries/silent_until from the live state right + // before we write it back. + let target = match self.ndp_state { NdpState::Discovered => return, - NdpState::Discovering { - target, - tries, - silent_until, - } => (target, tries, silent_until), + NdpState::Discovering { target, .. } => target, }; - if silent_until > now { - return; - } - - if tries >= NDP_MAX_TRIES { - self.drop_waiting_packets_v6(target); - self.ndp_state = NdpState::Discovered; - return; + { + let (tries, silent_until) = match self.ndp_state { + NdpState::Discovering { tries, silent_until, .. } => (tries, silent_until), + NdpState::Discovered => return, + }; + if silent_until > now { + return; + } + if tries >= NDP_MAX_TRIES { + self.drop_waiting_packets_v6(target); + self.ndp_state = NdpState::Discovered; + return; + } } let snmc = solicited_node_multicast(&target); @@ -502,6 +508,13 @@ impl EthernetLink { icmp.emit(&src_addr, &dst_addr, &mut icmp_pkt, &ChecksumCapabilities::ignored()); let icmp_len = icmp.buffer_len(); + // Read the current state (which may have been updated by a + // recursive call into drop_waiting_packets_v6) and update + // incrementally. This avoids overwriting a recursive update. + let (mut tries, mut silent_until) = match self.ndp_state { + NdpState::Discovering { tries, silent_until, .. } => (tries, silent_until), + NdpState::Discovered => return, + }; tries += 1; silent_until = now + Self::NDP_SILENCE_TIME; self.ndp_state = NdpState::Discovering { diff --git a/netstack/src/observer.rs b/netstack/src/observer.rs index 78d12a59e3..bb730a27f9 100644 --- a/netstack/src/observer.rs +++ b/netstack/src/observer.rs @@ -42,7 +42,7 @@ impl CaptureFilter { return true; } if packet.len() < 20 { - return true; // too short to filter, capture anyway + return false; // too short to determine protocol/port } let version = packet[0] >> 4; let (proto, src_port_offset, dst_port_offset) = match version { @@ -147,8 +147,12 @@ impl Observer { if buf.len() >= self.max_packets { buf.remove(0); } - let mut copy = Vec::with_capacity(packet.len().min(MAX_CAPTURE_BYTES / self.max_packets)); - copy.extend_from_slice(packet); + // Truncate to per-packet size limit so a single large packet + // can't exhaust the total capture buffer. + let per_packet_limit = MAX_CAPTURE_BYTES / self.max_packets.max(1); + let truncated_len = packet.len().min(per_packet_limit); + let mut copy = Vec::with_capacity(truncated_len); + copy.extend_from_slice(&packet[..truncated_len]); buf.push(copy); self.total_captured.fetch_add(1, Ordering::Relaxed); } diff --git a/netstack/src/scheme/socket.rs b/netstack/src/scheme/socket.rs index e73cb28777..8028710c4d 100644 --- a/netstack/src/scheme/socket.rs +++ b/netstack/src/scheme/socket.rs @@ -675,32 +675,33 @@ where let socket_handle = scheme_file.socket_handle(); let mut socket_set = self.socket_set.borrow_mut(); - let socket = socket_set.get::(socket_handle); - let _ = socket.close_file(&scheme_file, &mut self.scheme_data); - - let remove = match self.ref_counts.entry(socket_handle) { + // Compute refcount change WITHOUT calling close_file yet. + // The port release (inside close_file) must happen exactly + // once when the LAST file referencing this socket is + // closed — otherwise a dup'd socket's port would be + // double-freed. + let new_count = match self.ref_counts.entry(socket_handle) { Entry::Vacant(_) => { warn!("Closing a socket_handle with no ref"); - true + 0 } Entry::Occupied(mut e) => { - if *e.get() == 0 { - warn!("Closing a socket_handle with no ref"); + let count = *e.get(); + if count <= 1 { e.remove(); - true + 0 } else { *e.get_mut() -= 1; - if *e.get() == 0 { - e.remove(); - true - } else { - false - } + count - 1 } } }; - if remove { + // Only close the socket (which releases the port) when + // the last reference is gone. + if new_count == 0 { + let socket = socket_set.get::(socket_handle); + let _ = socket.close_file(&scheme_file, &mut self.scheme_data); socket_set.remove(socket_handle); } } diff --git a/netstack/src/scheme/tcp.rs b/netstack/src/scheme/tcp.rs index 4c2c34aa78..a84962154a 100644 --- a/netstack/src/scheme/tcp.rs +++ b/netstack/src/scheme/tcp.rs @@ -131,14 +131,26 @@ impl<'a> SchemeSocket for TcpSocket<'a> { port: local_endpoint.port, }; + let allocated_port = local_endpoint.port; + let allocated_ephemeral = local_endpoint_addr.is_none(); trace!("Connecting tcp {} {}", local_endpoint, remote_endpoint); - tcp_socket + if let Err(e) = tcp_socket .connect( context.iface.borrow_mut().context(), IpEndpoint::new(remote_endpoint.addr.unwrap(), remote_endpoint.port), local_endpoint, ) - .map_err(|_| SyscallError::new(syscall::EIO))?; + .map_err(|_| SyscallError::new(syscall::EIO)) + { + // Connect failed. Release the auto-allocated port (if + // any) so the next attempt can use it. Explicit user- + // provided ports are released by on_close when the + // last file is dropped. + if allocated_ephemeral { + port_set.release_port(allocated_port); + } + return Err(e); + } None } else { trace!("Listening tcp {}", local_endpoint);