vasilito/RedBear-OS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

review: NDP state corruption fix, port double-free fix, observer limits

Browse Source 
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.
This commit is contained in:
Red Bear OS
2026-07-09 00:42:46 +03:00
parent 52daa468b8
commit 3101345fe6
4 changed files with 64 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
netstack/src/link/ethernet.rs
+27 -14
View File
@@ -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 {
netstack/src/observer.rs
+7 -3
View File
@@ -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);
}
netstack/src/scheme/socket.rs
+16 -15
View File
@@ -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::<SocketT>(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::<SocketT>(socket_handle);
let _ = socket.close_file(&scheme_file, &mut self.scheme_data);
socket_set.remove(socket_handle);
}
}
netstack/src/scheme/tcp.rs
+14 -2
View File
@@ -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);