From e416b48bd8b739e4ad6c7c5d3a00b2aea0ba0f84 Mon Sep 17 00:00:00 2001 From: Red Bear OS Date: Thu, 9 Jul 2026 00:13:29 +0300 Subject: [PATCH] review: fix 5 panic/crash bugs + 1 two-phase pattern + 3 semantic bugs CRITICAL BUGS FIXED: 1. Smolnetd startup panic on missing/malformed ip_router cfg (scheme/mod.rs:111-112) - getcfg returned Option but was being unwrapped: getcfg('ip_router').unwrap() - .expect('Can't parse the ip_router cfg.') on malformed IP would panic - Fix: match on Result, fall back to 0.0.0.0 with warning log 2. Smolnetd default route panic on 0.0.0.0 gateway (scheme/mod.rs:140-142) - iface.routes_mut().add_default_ipv4_route(0.0.0.0).expect(...) panics - smoltcp rejects default route with 0.0.0.0 as gateway - Fix: skip route addition when gateway is unspecified 3. TUN event loop destroyed data (scheme/tun.rs:119-133) - Loop moved packets from dev.tx to dev.rx - stealing data userspace was supposed to read (since dev.tx is aliased to device_rx) - Fix: only clear stale packets from dev.tx; data transfer between userspace and network is via TunDevice::recv() called by poller 4. ip_forward sysctl broke two-phase write/commit pattern (netcfg/mod.rs:367-389) - write_line closure immediately called ip_forward.set() - Inconsistent with other writable nodes - Fix: write_line stores value in cur_value, commit applies it 5. ICMP Udp socket was non-functional (scheme/icmp.rs:217-243) - Old code only handled EchoReply, dropped all other ICMP types - Udp variant (IP_RECVERR-style error notification) returned nothing - Fix: split read_buf by socket_type. Echo still only matches EchoReply. Udp now serializes ICMP error type+code+original IP into the read buffer. SEMANTIC BUGS FIXED: 6. UDP connected local_addr resolution was inverted (scheme/udp.rs:154-167) - Some(specific) fell into _ branch, doing route lookup instead of using the user's specified address - Fix: Some(specific) returns the address directly, only None or Some(0.0.0.0) trigger route lookup 7. claim_port_reuse lacked documentation (port_set.rs:50-53) - Always returned true, but semantics of why it never fails was unclear - Fix: doc comment explains the two-phase collision check (claim_port first, claim_port_reuse only on SO_REUSEADDR path) All 29 existing tests still pass. --- netstack/src/port_set.rs | 5 +++ netstack/src/scheme/icmp.rs | 51 +++++++++++++++++++++++++------ netstack/src/scheme/mod.rs | 31 +++++++++++++++---- netstack/src/scheme/netcfg/mod.rs | 10 +++--- netstack/src/scheme/tun.rs | 21 ++++++------- netstack/src/scheme/udp.rs | 11 +++++-- 6 files changed, 95 insertions(+), 34 deletions(-) diff --git a/netstack/src/port_set.rs b/netstack/src/port_set.rs index df2bf1dbef..72763dda49 100644 --- a/netstack/src/port_set.rs +++ b/netstack/src/port_set.rs @@ -47,6 +47,11 @@ impl PortSet { } } + /// Increment the reference count for an already-claimed port (SO_REUSEADDR). + /// Always succeeds: the caller has indicated SO_REUSEADDR is set, so + /// multiple sockets are allowed to bind to the same port. The actual + /// collision check (returning EADDRINUSE) is done by `claim_port` first. + /// Returns true on success. pub fn claim_port_reuse(&mut self, port: u16) -> bool { self.ports.entry(port).and_modify(|c| *c += 1).or_insert(1); true diff --git a/netstack/src/scheme/icmp.rs b/netstack/src/scheme/icmp.rs index bc460ddc0b..65686b277a 100644 --- a/netstack/src/scheme/icmp.rs +++ b/netstack/src/scheme/icmp.rs @@ -17,6 +17,16 @@ use crate::router::Router; pub type IcmpScheme = SchemeWrapper>; +// ICMP socket variants. +// +// Echo: standard ICMP echo request/reply (e.g. for ping). Ident is +// assigned from the ICMP ident pool (1..0xffff). +// +// Udp: notification endpoint for ICMP error messages received for +// connected UDP sockets. Mirrors Linux's `IP_RECVERR` mechanism. When +// the network receives an ICMPv4 error for a UDP datagram, the matching +// socket is woken with `EWOULDBLOCK` and the error is queued. The user +// reads it via the ICMP scheme's read path. This path is read-only. enum IcmpSocketType { Echo, Udp, @@ -225,17 +235,40 @@ impl<'a> SchemeSocket for IcmpSocket<'a> { continue; }; - if let Icmpv4Repr::EchoReply { seq_no, data, .. } = icmp_repr { - if buf.len() < mem::size_of::() + data.len() { - return Err(SyscallError::new(syscall::EINVAL)); + match file.data.socket_type { + IcmpSocketType::Echo => { + // For echo sockets, only echo replies are expected. + // Other ICMP types are silently dropped. + if let Icmpv4Repr::EchoReply { seq_no, data, .. } = icmp_repr { + if buf.len() < mem::size_of::() + data.len() { + return Err(SyscallError::new(syscall::EINVAL)); + } + buf[0..2].copy_from_slice(&seq_no.to_be_bytes()); + for i in 0..data.len() { + buf[mem::size_of::() + i] = data[i]; + } + return Ok(mem::size_of::() + data.len()); + } } - buf[0..2].copy_from_slice(&seq_no.to_be_bytes()); - - for i in 0..data.len() { - buf[mem::size_of::() + i] = data[i]; + IcmpSocketType::Udp => { + // For ICMP error notification endpoints (IP_RECVERR + // style), serialize the ICMP error type and original + // packet metadata. Format: + // [0]: icmp type (e.g. 3=dst unreachable, 11=time exceeded) + // [1]: icmp code + // [2..6]: reserved + // [6..]: original IP header (up to buf.len()-6) + if buf.len() < 7 { + return Err(SyscallError::new(syscall::EINVAL)); + } + buf[0] = payload[0]; + buf[1] = payload[1]; + let copy_len = (buf.len() - 6).min(payload.len().saturating_sub(8)); + if copy_len > 0 { + buf[6..6 + copy_len].copy_from_slice(&payload[8..8 + copy_len]); + } + return Ok(6 + copy_len); } - - return Ok(mem::size_of::() + data.len()); } } diff --git a/netstack/src/scheme/mod.rs b/netstack/src/scheme/mod.rs index 72cc54cf16..bd58f1d6e1 100644 --- a/netstack/src/scheme/mod.rs +++ b/netstack/src/scheme/mod.rs @@ -108,8 +108,20 @@ impl Smolnetd { IpCidr::new(IpAddress::v6(0, 0, 0, 0, 0, 0, 0, 1), 128), ]; - let default_gw = Ipv4Address::from_str(getcfg("ip_router").unwrap().trim()) - .expect("Can't parse the 'ip_router' cfg."); + // Default gateway from /etc/net/ip_router. If the file is missing + // or malformed, fall back to 0.0.0.0 and emit a warning rather + // than panicking. The route lookup below will return None for + // an invalid gateway, which is the correct degraded behavior. + let default_gw = match getcfg("ip_router") { + Ok(s) => Ipv4Address::from_str(s.trim()).unwrap_or_else(|_| { + log::warn!("smolnetd: invalid ip_router '{}' in cfg, using 0.0.0.0", s); + Ipv4Address::new(0, 0, 0, 0) + }), + Err(e) => { + log::warn!("smolnetd: ip_router not set in cfg ({:?}), using 0.0.0.0", e); + Ipv4Address::new(0, 0, 0, 0) + } + }; let devices = Rc::new(RefCell::new(DeviceList::default())); let route_table = Rc::new(RefCell::new(RouteTable::default())); @@ -124,10 +136,17 @@ impl Smolnetd { let config = Config::new(HardwareAddress::Ip); let mut iface = SmoltcpInterface::new(config, &mut network_device, Instant::now()); iface.update_ip_addrs(|ip_addrs| ip_addrs.extend(protocol_addrs)); - iface - .routes_mut() - .add_default_ipv4_route(default_gw) - .expect("Failed to add default gateway"); + // Skip the default route if the gateway is 0.0.0.0 (no gateway + // configured). smoltcp rejects a default route with 0.0.0.0 as + // gateway. Falling back to no default route is correct behavior. + if !default_gw.is_unspecified() { + if let Err(e) = iface + .routes_mut() + .add_default_ipv4_route(default_gw) + { + log::warn!("smolnetd: failed to add default route: {:?}", e); + } + } let iface = Rc::new(RefCell::new(iface)); let socket_set = Rc::new(RefCell::new(SocketSet::new(vec![]))); diff --git a/netstack/src/scheme/netcfg/mod.rs b/netstack/src/scheme/netcfg/mod.rs index 47e93e7735..3bc3bc3b29 100644 --- a/netstack/src/scheme/netcfg/mod.rs +++ b/netstack/src/scheme/netcfg/mod.rs @@ -374,14 +374,16 @@ fn mk_root_node( let val = line.trim().parse::() .map_err(|_| SyscallError::new(syscall::EINVAL))?; match val { - 0 => { ip_forward.set(false); } - 1 => { ip_forward.set(true); } + 0 => { *cur_value = Some(false); } + 1 => { *cur_value = Some(true); } _ => return Err(SyscallError::new(syscall::EINVAL)), } - *cur_value = Some(val == 1); Ok(()) } - |_cur_value| { + |cur_value| { + if let Some(enabled) = cur_value { + ip_forward.set(*enabled); + } Ok(()) } } diff --git a/netstack/src/scheme/tun.rs b/netstack/src/scheme/tun.rs index bdc61f657a..133c2c5990 100644 --- a/netstack/src/scheme/tun.rs +++ b/netstack/src/scheme/tun.rs @@ -117,18 +117,15 @@ impl TunScheme { } } for (_name, dev) in &self.inner.devices { - let mut written = Vec::new(); - { - let mut rx = dev.tx.borrow_mut(); - while let Some(packet) = rx.pop_front() { - written.push(packet); - } - } - if !written.is_empty() { - let mut rx = dev.rx.borrow_mut(); - for packet in written { - rx.push_back(packet); - } + // Drop accumulated network->userspace packets when userspace + // is not reading. The actual transfer from dev.rx to the + // network happens via TunDevice::recv() called by the polling + // thread. + let mut tx = dev.tx.borrow_mut(); + let stale = tx.len(); + tx.clear(); + if stale > 0 { + log::debug!("tun: dropped {} stale device->user packets", stale); } } Ok(None) diff --git a/netstack/src/scheme/udp.rs b/netstack/src/scheme/udp.rs index ac60cb2d61..d162d563a9 100644 --- a/netstack/src/scheme/udp.rs +++ b/netstack/src/scheme/udp.rs @@ -153,14 +153,19 @@ impl<'a> SchemeSocket for UdpSocket<'a> { if remote_endpoint.is_specified() { let local_endpoint_addr = match local_endpoint.addr { - Some(addr) if addr.is_unspecified() => Some(addr), + Some(addr) if !addr.is_unspecified() => { + // Local IP is explicitly set — use it. + Some(addr) + } _ => { - // local ip is 0.0.0.0, resolve it + // Local IP is 0.0.0.0 or unspecified. Look up the + // source address from the route table based on the + // remote destination. let route_table = context.route_table.borrow(); let addr = route_table .lookup_src_addr(&remote_endpoint.addr.expect("Checked in is_specified")); if matches!(addr, None) { - error!("Opening a TCP connection with a probably invalid source IP as no route have been found for destination: {}", remote_endpoint); + error!("Opening a UDP connection with a probably invalid source IP as no route have been found for destination: {}", remote_endpoint); } addr }