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

review: fix 5 panic/crash bugs + 1 two-phase pattern + 3 semantic bugs

Browse Source 
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.
This commit is contained in:
Red Bear OS
2026-07-09 00:13:29 +03:00
parent 08d0fb2ede
commit e416b48bd8
6 changed files with 95 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
netstack/src/port_set.rs
+5
View File
@@ -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
netstack/src/scheme/icmp.rs
+42 -9
View File
@@ -17,6 +17,16 @@ use crate::router::Router;
pub type IcmpScheme = SchemeWrapper<IcmpSocket<'static>>;
// 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::<u16>() + 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::<u16>() + 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::<u16>() + i] = data[i];
}
return Ok(mem::size_of::<u16>() + data.len());
}
}
buf[0..2].copy_from_slice(&seq_no.to_be_bytes());
for i in 0..data.len() {
buf[mem::size_of::<u16>() + 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::<u16>() + data.len());
}
}
netstack/src/scheme/mod.rs
+25 -6
View File
@@ -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![])));
netstack/src/scheme/netcfg/mod.rs
+6 -4
View File
@@ -374,14 +374,16 @@ fn mk_root_node(
let val = line.trim().parse::<u8>()
.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(())
}
}
netstack/src/scheme/tun.rs
+9 -12
View File
@@ -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)
netstack/src/scheme/udp.rs
+8 -3
View File
@@ -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
}