From d4bad389082765975bdc4bf9233be86bb4cf5e45 Mon Sep 17 00:00:00 2001 From: Juergen Kunz Date: Mon, 6 Apr 2026 10:15:37 +0000 Subject: [PATCH] fix(server): clean up bridge and hybrid shutdown handling --- changelog.md | 8 +++ rust/src/server.rs | 86 +++++++++++++++++++++++++++++--- rust/src/wireguard.rs | 6 ++- ts/00_commitinfo_data.ts | 2 +- ts/smartvpn.classes.vpnserver.ts | 22 +++++++- 5 files changed, 111 insertions(+), 13 deletions(-) diff --git a/changelog.md b/changelog.md index cacb50c..e214f1f 100644 --- a/changelog.md +++ b/changelog.md @@ -1,5 +1,13 @@ # Changelog +## 2026-04-06 - 1.19.2 - fix(server) +clean up bridge and hybrid shutdown handling + +- persist bridge teardown metadata so stop() can restore host IP configuration and remove the bridge in bridge and hybrid modes +- use separate shutdown channels for hybrid socket and bridge engines to stop both forwarding paths correctly +- avoid IP pool leaks when client registration fails and ignore unspecified IPv4 addresses when selecting WireGuard peer addresses +- make daemon bridge stop await nftables cleanup and process exit, and cap effective tunnel MTU to the link MTU + ## 2026-04-01 - 1.19.1 - fix(rust) clean up unused Rust warnings in bridge, network, and server modules diff --git a/rust/src/server.rs b/rust/src/server.rs index c81a05c..fc9c938 100644 --- a/rust/src/server.rs +++ b/rust/src/server.rs @@ -173,6 +173,14 @@ pub enum ForwardingEngine { Testing, } +/// Info needed to tear down bridge infrastructure on stop(). +pub struct BridgeCleanupInfo { + pub physical_iface: String, + pub bridge_name: String, + pub host_ip: Ipv4Addr, + pub host_prefix: u8, +} + /// Shared server state. pub struct ServerState { pub config: ServerConfig, @@ -189,6 +197,10 @@ pub struct ServerState { pub tun_routes: RwLock>>>, /// Shutdown signal for the forwarding background task (TUN reader or NAT engine). pub tun_shutdown: mpsc::Sender<()>, + /// Shutdown signal for the bridge engine (bridge/hybrid modes only). + pub bridge_shutdown: Option>, + /// Bridge teardown info (bridge/hybrid modes only). + pub bridge_cleanup: Option, } /// The VPN server. @@ -267,6 +279,9 @@ impl VpnServer { Testing, } + let mut bridge_cleanup_info: Option = None; + let mut bridge_shut_tx: Option> = None; + let (setup, fwd_shutdown_tx) = match mode { "tun" => { let tun_config = TunConfig { @@ -310,6 +325,13 @@ impl VpnServer { info!("Bridge {} created: TAP={}, physical={}, IP={}/{}", bridge_name, tap_name, phys_iface, host_ip, host_prefix); + bridge_cleanup_info = Some(BridgeCleanupInfo { + physical_iface: phys_iface, + bridge_name: bridge_name.to_string(), + host_ip, + host_prefix, + }); + let (packet_tx, packet_rx) = mpsc::channel::>(4096); let (tx, rx) = mpsc::channel::<()>(1); (ForwardingSetup::Bridge { packet_tx, packet_rx, tap_device, shutdown_rx: rx }, tx) @@ -319,7 +341,7 @@ impl VpnServer { // Socket engine setup let (s_tx, s_rx) = mpsc::channel::>(4096); - let (_s_shut_tx, s_shut_rx) = mpsc::channel::<()>(1); + let (s_shut_tx, s_shut_rx) = mpsc::channel::<()>(1); // Bridge engine setup let phys_iface = match &config.bridge_physical_interface { @@ -347,14 +369,20 @@ impl VpnServer { info!("Hybrid mode: socket + bridge (TAP={}, physical={}, IP={}/{})", tap_name, phys_iface, host_ip, host_prefix); - // We use s_shut_tx as the main shutdown (it will trigger both) - let _ = b_shut_tx; // bridge shutdown handled separately - let (tx, _) = mpsc::channel::<()>(1); + bridge_cleanup_info = Some(BridgeCleanupInfo { + physical_iface: phys_iface, + bridge_name: bridge_name.to_string(), + host_ip, + host_prefix, + }); + bridge_shut_tx = Some(b_shut_tx); + + // Socket engine uses fwd_shutdown_tx (stored in state.tun_shutdown) (ForwardingSetup::Hybrid { socket_tx: s_tx, socket_rx: s_rx, socket_shutdown_rx: s_shut_rx, bridge_tx: b_tx, bridge_rx: b_rx, bridge_shutdown_rx: b_shut_rx, tap_device, routing_table, - }, tx) + }, s_shut_tx) } _ => { info!("Forwarding disabled (testing/monitoring mode)"); @@ -365,7 +393,7 @@ impl VpnServer { // Compute effective MTU from overhead let overhead = TunnelOverhead::default_overhead(); - let mtu_config = MtuConfig::new(overhead.effective_tun_mtu(1500).max(link_mtu)); + let mtu_config = MtuConfig::new(overhead.effective_tun_mtu(1500).min(link_mtu)); // Build client registry from config let registry = ClientRegistry::from_entries( @@ -385,6 +413,8 @@ impl VpnServer { forwarding_engine: Mutex::new(ForwardingEngine::Testing), tun_routes: RwLock::new(HashMap::new()), tun_shutdown: fwd_shutdown_tx, + bridge_shutdown: bridge_shut_tx, + bridge_cleanup: bridge_cleanup_info, }); // Spawn the forwarding background task and set the engine @@ -588,6 +618,43 @@ impl VpnServer { let _ = state.tun_shutdown.send(()).await; *state.forwarding_engine.lock().await = ForwardingEngine::Testing; } + "bridge" => { + let _ = state.tun_shutdown.send(()).await; + *state.forwarding_engine.lock().await = ForwardingEngine::Testing; + // Restore host networking: move IP back and remove bridge + if let Some(ref cleanup) = state.bridge_cleanup { + if let Err(e) = crate::bridge::restore_host_ip( + &cleanup.physical_iface, &cleanup.bridge_name, + cleanup.host_ip, cleanup.host_prefix, + ).await { + warn!("Failed to restore host IP: {}", e); + } + if let Err(e) = crate::bridge::remove_bridge(&cleanup.bridge_name).await { + warn!("Failed to remove bridge: {}", e); + } + } + } + "hybrid" => { + // Shut down socket (NAT) engine + let _ = state.tun_shutdown.send(()).await; + // Shut down bridge engine + if let Some(ref bridge_shut) = state.bridge_shutdown { + let _ = bridge_shut.send(()).await; + } + *state.forwarding_engine.lock().await = ForwardingEngine::Testing; + // Restore host networking: move IP back and remove bridge + if let Some(ref cleanup) = state.bridge_cleanup { + if let Err(e) = crate::bridge::restore_host_ip( + &cleanup.physical_iface, &cleanup.bridge_name, + cleanup.host_ip, cleanup.host_prefix, + ).await { + warn!("Failed to restore host IP: {}", e); + } + if let Err(e) = crate::bridge::remove_bridge(&cleanup.bridge_name).await { + warn!("Failed to remove bridge: {}", e); + } + } + } _ => {} } @@ -807,8 +874,11 @@ impl VpnServer { vlan_id: partial.get("vlanId").and_then(|v| v.as_u64()).map(|v| v as u16), }; - // Add to registry - state.client_registry.write().await.add(entry.clone())?; + // Add to registry — release IP on failure to avoid pool leak + if let Err(e) = state.client_registry.write().await.add(entry.clone()) { + state.ip_pool.lock().await.release(&assigned_ip); + return Err(e); + } // Register WG peer with the running WG listener (if active) if self.wg_command_tx.is_some() { diff --git a/rust/src/wireguard.rs b/rust/src/wireguard.rs index ac4b4b6..4ab4a04 100644 --- a/rust/src/wireguard.rs +++ b/rust/src/wireguard.rs @@ -319,10 +319,12 @@ fn extract_peer_vpn_ip(allowed_ips: &[AllowedIp]) -> Option { } } } - // Fallback: use the first IPv4 address from any prefix length + // Fallback: use the first non-unspecified IPv4 address from any prefix length for aip in allowed_ips { if let IpAddr::V4(v4) = aip.addr { - return Some(v4); + if !v4.is_unspecified() { + return Some(v4); + } } } None diff --git a/ts/00_commitinfo_data.ts b/ts/00_commitinfo_data.ts index 7264704..98422dc 100644 --- a/ts/00_commitinfo_data.ts +++ b/ts/00_commitinfo_data.ts @@ -3,6 +3,6 @@ */ export const commitinfo = { name: '@push.rocks/smartvpn', - version: '1.19.1', + version: '1.19.2', description: 'A VPN solution with TypeScript control plane and Rust data plane daemon' } diff --git a/ts/smartvpn.classes.vpnserver.ts b/ts/smartvpn.classes.vpnserver.ts index 8b5bf84..7ba844e 100644 --- a/ts/smartvpn.classes.vpnserver.ts +++ b/ts/smartvpn.classes.vpnserver.ts @@ -333,17 +333,35 @@ export class VpnServer extends plugins.events.EventEmitter { /** * Stop the daemon bridge. */ - public stop(): void { + public async stop(): Promise { // Clean up nftables rules if (this.nftHealthInterval) { clearInterval(this.nftHealthInterval); this.nftHealthInterval = undefined; } if (this.nft) { - this.nft.cleanup().catch(() => {}); // best-effort cleanup + try { + await this.nft.cleanup(); + } catch (e) { + console.warn(`[smartvpn] nftables cleanup failed: ${e}`); + } this.nft = undefined; } + + // Wait for bridge process to exit (with timeout) + const exitPromise = new Promise((resolve) => { + if (!this.bridge.running) { + resolve(); + return; + } + const timeout = setTimeout(() => resolve(), 5000); + this.bridge.once('exit', () => { + clearTimeout(timeout); + resolve(); + }); + }); this.bridge.stop(); + await exitPromise; } /**