From b6e78c1d291f065a047871936ddef4a27d93ca22 Mon Sep 17 00:00:00 2001 From: ospab Date: Mon, 22 Jun 2026 14:57:20 +0300 Subject: [PATCH] Fix TUN no-internet: terminate helper cleanly and harden bypass routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The helper logged "exiting" but never terminated: the WinTun blocking receive runs on a thread that task.abort() cannot cancel, so it kept the ostp_tun adapter (and its metric-0 default route) alive and hung the tokio runtime as a zombie. The next connect then faced two competing default routes and failed to install the per-server /32 bypass, so the client's own handshake packets looped back into the dead tunnel — every OSTP handshake timed out and there was no internet. - ostp-tun-helper: std::process::exit(0) after run_server returns so the kernel reclaims the adapter and all routes bound to it. - ostp-tun/windows_route: dedupe bypass IPs, purge any stale /32 for the dest before adding (enumerate + delete), and log add failures at warn! instead of debug! so the cause is visible in the INFO-level helper log. - ostp-tun/windows: keep .destination() LUID default route (reliable capture) alongside the racy friendly-name route; retry create() through the transient ERROR_INVALID_PARAMETER window. - ostp-client: wire BridgeMetrics.connection_state through runner and inbounds so the GUI reflects connecting/connected/disconnected. - ostp-gui: parse JSONC config (strip // and /* */) in the settings view. Co-Authored-By: Claude Opus 4.8 --- Cargo.lock | 4 +- ostp-client/src/runner.rs | 12 ++++- .../src/tunnel/inbounds/local_proxy.rs | 6 +++ ostp-client/src/tunnel/inbounds/tun.rs | 10 +++- ostp-core/src/protocol.rs | 2 +- ostp-gui/src-tauri/Cargo.lock | 1 + ostp-gui/src/main.js | 35 +++++++++++-- ostp-tun-helper/src/main.rs | 14 ++++- ostp-tun/src/windows.rs | 51 ++++++++++++++++--- ostp-tun/src/windows_route.rs | 43 ++++++++++++++-- 10 files changed, 159 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0ea09fd..4c780de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1591,12 +1591,13 @@ name = "ostp-tun-helper" version = "0.3.12" dependencies = [ "anyhow", - "chrono", + "hex", "ostp-client", "portable-atomic", "serde", "serde_json", "tokio", + "tracing", "winres", ] @@ -2589,6 +2590,7 @@ dependencies = [ "sharded-slab", "smallvec", "thread_local", + "time", "tracing", "tracing-core", "tracing-log", diff --git a/ostp-client/src/runner.rs b/ostp-client/src/runner.rs index 7dbfc5a..f697f76 100644 --- a/ostp-client/src/runner.rs +++ b/ostp-client/src/runner.rs @@ -9,12 +9,18 @@ use crate::tunnel::router::Router; pub async fn run_client_core( config: ClientConfig, - _metrics: Arc, + metrics: Arc, mut shutdown_rx_ext: watch::Receiver, _config_rx: Option>, ) -> Result<()> { + use portable_atomic::Ordering; tracing::info!("starting client core"); + // Report "connecting" until an inbound has successfully bound. Each inbound + // flips this to 2 (connected) once it is ready; if they all fail, the + // select! below returns and we reset to 0 (disconnected). + metrics.connection_state.store(1, Ordering::Relaxed); + let router = Arc::new(Router::new(config.routing.clone())); let balancer = Arc::new(Balancer::new(&config)); @@ -29,6 +35,7 @@ pub async fn run_client_core( let outbound_manager_clone = outbound_manager.clone(); let shutdown_rx = shutdown_rx_ext.clone(); let config_clone = config.clone(); + let metrics_clone = metrics.clone(); match inbound.clone() { InboundConfig::Tun { .. } => { @@ -39,6 +46,7 @@ pub async fn run_client_core( router_clone, outbound_manager_clone, shutdown_rx, + metrics_clone, ).await { tracing::error!("TUN inbound failed: {}", e); } @@ -52,6 +60,7 @@ pub async fn run_client_core( router_clone, outbound_manager_clone, shutdown_rx, + metrics_clone, ).await { tracing::error!("SOCKS inbound failed: {}", e); } @@ -69,5 +78,6 @@ pub async fn run_client_core( } } + metrics.connection_state.store(0, Ordering::Relaxed); Ok(()) } diff --git a/ostp-client/src/tunnel/inbounds/local_proxy.rs b/ostp-client/src/tunnel/inbounds/local_proxy.rs index 7c32cba..b02a8cd 100644 --- a/ostp-client/src/tunnel/inbounds/local_proxy.rs +++ b/ostp-client/src/tunnel/inbounds/local_proxy.rs @@ -13,7 +13,9 @@ pub async fn run_socks_inbound( router: Arc, outbound_manager: Arc, mut shutdown: watch::Receiver, + metrics: Arc, ) -> Result<()> { + use portable_atomic::Ordering; let InboundConfig::LocalProxy { tag, protocol, listen, port, set_system_proxy } = inbound_config else { return Err(anyhow!("Invalid config for LocalProxy inbound")); }; @@ -30,6 +32,10 @@ pub async fn run_socks_inbound( let listener = TcpListener::bind(&bind_addr).await?; + // Listener bound successfully — the proxy is ready to accept connections. + metrics.connection_state.store(2, Ordering::Relaxed); + tracing::info!("{} proxy inbound ready on {}, connection state = connected", protocol, bind_addr); + loop { tokio::select! { _ = shutdown.changed() => { diff --git a/ostp-client/src/tunnel/inbounds/tun.rs b/ostp-client/src/tunnel/inbounds/tun.rs index 3d6a9f9..b04b292 100644 --- a/ostp-client/src/tunnel/inbounds/tun.rs +++ b/ostp-client/src/tunnel/inbounds/tun.rs @@ -13,9 +13,11 @@ pub async fn run_tun_inbound( router: Arc, outbound_manager: Arc, mut shutdown: watch::Receiver, + metrics: Arc, ) -> Result<()> { - + use netstack_smoltcp::StackBuilder; + use portable_atomic::Ordering; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use futures::{StreamExt, SinkExt}; @@ -179,6 +181,11 @@ pub async fn run_tun_inbound( } }; + // TUN device is up and the default route has been installed inside + // OstpTunInterface::create — the tunnel is now carrying traffic. + metrics.connection_state.store(2, Ordering::Relaxed); + tracing::info!("TUN inbound ready, connection state = connected"); + // ── TCP Handler ── let outbound_manager_tcp = outbound_manager.clone(); let router_tcp = router.clone(); @@ -296,6 +303,7 @@ pub async fn run_tun_inbound( _router: Arc, _outbound_manager: Arc, _shutdown: watch::Receiver, + _metrics: Arc, ) -> Result<()> { Err(anyhow!("TUN is only supported on Windows and Linux")) } diff --git a/ostp-core/src/protocol.rs b/ostp-core/src/protocol.rs index da68665..62491c1 100644 --- a/ostp-core/src/protocol.rs +++ b/ostp-core/src/protocol.rs @@ -265,7 +265,7 @@ impl ProtocolMachine { noise_len, raw_vec.len() - 6 ))); } - tracing::info!("handle_inbound: raw_vec.len()={}, noise_len={}, raw_vec[0..6]={:?}", raw_vec.len(), noise_len, &raw_vec[0..6]); + tracing::debug!("handle_inbound: raw_vec.len()={}, noise_len={}, raw_vec[0..6]={:?}", raw_vec.len(), noise_len, &raw_vec[0..6]); let mut read_out = vec![0_u8; 1024]; let n = self.noise.read_handshake(&raw_vec[6..6 + noise_len], &mut read_out).map_err(|e| { diff --git a/ostp-gui/src-tauri/Cargo.lock b/ostp-gui/src-tauri/Cargo.lock index bcc30e2..7691f72 100644 --- a/ostp-gui/src-tauri/Cargo.lock +++ b/ostp-gui/src-tauri/Cargo.lock @@ -4544,6 +4544,7 @@ dependencies = [ "sharded-slab", "smallvec", "thread_local", + "time", "tracing", "tracing-core", "tracing-log", diff --git a/ostp-gui/src/main.js b/ostp-gui/src/main.js index 40f8696..90e67b9 100644 --- a/ostp-gui/src/main.js +++ b/ostp-gui/src/main.js @@ -6,6 +6,33 @@ if (window.__TAURI__?.core) { invoke = window.__TAURI__.core.invoke; } +// ── JSONC parsing ───────────────────────────────────────────────────────────── +// config.json is JSONC: save_config prepends a `// OSTP Configuration` header +// and the default template carries comments. Strip // line and /* */ block +// comments before JSON.parse, while preserving them inside string literals +// (config values contain URLs like https:// and paths like ./wintun.dll). +function parseJsonc(raw) { + let out = ''; + let inStr = false, inLine = false, inBlock = false, escaped = false; + for (let i = 0; i < raw.length; i++) { + const c = raw[i], n = raw[i + 1]; + if (inLine) { if (c === '\n') { inLine = false; out += c; } continue; } + if (inBlock) { if (c === '*' && n === '/') { inBlock = false; i++; } continue; } + if (inStr) { + out += c; + if (escaped) escaped = false; + else if (c === '\\') escaped = true; + else if (c === '"') inStr = false; + continue; + } + if (c === '"') { inStr = true; out += c; continue; } + if (c === '/' && n === '/') { inLine = true; i++; continue; } + if (c === '/' && n === '*') { inBlock = true; i++; continue; } + out += c; + } + return JSON.parse(out); +} + // ── State ──────────────────────────────────────────────────────────────────── let appState = 'disconnected'; // 'disconnected' | 'connecting' | 'connected' let pollTimer = null; @@ -384,7 +411,7 @@ async function handleToggle() { if (appState === 'disconnected') { try { const raw = await invoke('get_config'); - const cfg = JSON.parse(raw); + const cfg = parseJsonc(raw); serverAddr = cfg.server || ''; } catch { serverAddr = ''; } @@ -434,7 +461,7 @@ function showScreen(name) { async function loadConfigIntoForm() { try { const raw = await invoke('get_config'); - rawConfig = JSON.parse(raw); + rawConfig = parseJsonc(raw); const c = rawConfig.mode === 'client' ? rawConfig : null; if (!c) return; @@ -695,7 +722,7 @@ window.addEventListener('DOMContentLoaded', async () => { // Auto-connect on startup try { const raw = await invoke('get_config'); - rawConfig = JSON.parse(raw); + rawConfig = parseJsonc(raw); if (rawConfig?.gui?.autoconnect) { setTimeout(() => { if (appState === 'disconnected') handleToggle(); @@ -716,7 +743,7 @@ window.addEventListener('DOMContentLoaded', async () => { try { const raw = await invoke('get_config'); - rawConfig = JSON.parse(raw); + rawConfig = parseJsonc(raw); } catch { showToast('Please save your config first', 'error'); return; diff --git a/ostp-tun-helper/src/main.rs b/ostp-tun-helper/src/main.rs index b06657e..4a00ba0 100644 --- a/ostp-tun-helper/src/main.rs +++ b/ostp-tun-helper/src/main.rs @@ -74,7 +74,19 @@ async fn main() -> Result<()> { tracing::error!("fatal: {}", e); } tracing::info!("helper exiting"); - Ok(()) + + // The WinTun blocking `receive` runs on a thread that `task.abort()` cannot + // cancel, so it keeps the adapter handle — and the default route bound to it + // — alive and prevents the tokio runtime from shutting down. Without this the + // process lingers as a zombie: `ostp_tun` stays Up, its metric-0 default + // route competes with the physical one, and the NEXT connect fails to install + // the server bypass route, so traffic loops back into a dead tunnel (no + // internet). The GUI launches a fresh helper for every connect, so this + // process has no more work once run_server returns. Give the synchronous + // route/firewall teardown a moment to finish, then force the process to exit + // so the kernel reclaims the adapter and every route bound to it. + tokio::time::sleep(Duration::from_millis(800)).await; + std::process::exit(0); } async fn run_server(expected_token: String, port: u16) -> Result<()> { diff --git a/ostp-tun/src/windows.rs b/ostp-tun/src/windows.rs index 78a0917..dc15048 100644 --- a/ostp-tun/src/windows.rs +++ b/ostp-tun/src/windows.rs @@ -68,36 +68,73 @@ pub async fn create(opts: OstpTunOptions) -> Result { .tun_name("ostp_tun") .address((10, 1, 0, 2)) .netmask((255, 255, 255, 0)) + // `.destination()` makes the `tun` crate install the default route via + // the adapter's LUID (available immediately after creation). This is + // the RELIABLE default route that captures traffic — the manual + // add_ipv4_route below depends on a friendly-name interface-index + // lookup that races on a freshly created adapter. Keep both: the LUID + // route guarantees connectivity even when the index lookup is slow. + // The retry loop around tun::create absorbs the transient + // ERROR_INVALID_PARAMETER (os error 87) this call can hit during the + // post-creation registration window. .destination((10, 1, 0, 1)) .mtu(opts.mtu) .up(); - let dev = tun::create(&tun_cfg).map_err(|e| anyhow!("Failed to create TUN device: {}", e))?; + // The IpHelper calls the `tun` crate performs right after Adapter::create + // (set address / mtu) can transiently fail with ERROR_INVALID_PARAMETER + // (os error 87) when the freshly created interface is not yet registered + // in the IP stack. Retry a few times; on retry the crate reuses the + // existing adapter via Adapter::open. + let dev = { + let mut attempt = 0; + loop { + attempt += 1; + match tun::create(&tun_cfg) { + Ok(d) => break d, + Err(e) if attempt < 5 => { + tracing::warn!( + "TUN device creation attempt {}/5 failed: {} — retrying in 300ms", + attempt, e + ); + tokio::time::sleep(std::time::Duration::from_millis(300)).await; + } + Err(e) => return Err(anyhow!("Failed to create TUN device after {} attempts: {}", attempt, e)), + } + } + }; let dev = tun::AsyncDevice::new(dev).map_err(|e| anyhow!("TUN device async failed: {}", e))?; tracing::info!("TUN device 'ostp_tun' created."); let current_exe = std::env::current_exe()?.to_string_lossy().into_owned(); + // A freshly created WinTun adapter can take several seconds to appear in + // GetAdaptersAddresses (it only shows up once it has an operational IPv4 + // binding). The default route via the TUN is what actually captures + // traffic, so this lookup is critical — give it a generous window + // (~15s) before giving up rather than the previous 2s. let mut tun_index = None; - for _ in 0..20 { + for _ in 0..75 { if let Some(idx) = windows_route::sys::get_interface_index("ostp_tun") { tun_index = Some(idx); break; } - tokio::time::sleep(std::time::Duration::from_millis(100)).await; + tokio::time::sleep(std::time::Duration::from_millis(200)).await; } if let Some(idx) = tun_index { - let _ = windows_route::sys::add_ipv4_route( + match windows_route::sys::add_ipv4_route( std::net::Ipv4Addr::new(0, 0, 0, 0), std::net::Ipv4Addr::new(0, 0, 0, 0), std::net::Ipv4Addr::new(10, 1, 0, 1), idx, 5, - ); - tracing::info!("Default route via TUN (if_index={idx}, metric=5) added."); + ) { + Ok(()) => tracing::info!("Default route via TUN (if_index={idx}, metric=5) added."), + Err(e) => tracing::error!("Failed to add default route via TUN (if_index={idx}): {e} — traffic will NOT be captured."), + } } else { - tracing::warn!("Could not find ostp_tun index in routing table — traffic may not be captured."); + tracing::error!("Could not find ostp_tun index in routing table after 15s — traffic will NOT be captured."); } let exe1 = current_exe.clone(); diff --git a/ostp-tun/src/windows_route.rs b/ostp-tun/src/windows_route.rs index 13ca88f..659a1db 100644 --- a/ostp-tun/src/windows_route.rs +++ b/ostp-tun/src/windows_route.rs @@ -168,6 +168,34 @@ pub mod sys { } } + /// Delete every routing-table entry whose destination is `dest`/`mask`, + /// regardless of its gateway or interface. Used to purge stale bypass routes + /// left by a previous session (possibly pointing at an old gateway after a + /// network change) so a fresh, correct one can be installed. + pub fn delete_routes_for_dest(dest: Ipv4Addr, mask: Ipv4Addr) { + unsafe { + let mut size: ULONG = 0; + if GetIpForwardTable(ptr::null_mut(), &mut size, 0) != ERROR_INSUFFICIENT_BUFFER { + return; + } + let mut buf: Vec = vec![0; size as usize]; + let table = buf.as_mut_ptr() as *mut MIB_IPFORWARDTABLE; + if GetIpForwardTable(table, &mut size, 0) != NO_ERROR { + return; + } + let want_dest = ipv4_to_dword(dest); + let want_mask = ipv4_to_dword(mask); + let entries = + std::slice::from_raw_parts_mut((*table).table.as_mut_ptr(), (*table).dwNumEntries as usize); + for row in entries { + if row.dwForwardDest == want_dest && row.dwForwardMask == want_mask { + // Delete the exact existing row (its own nexthop/ifindex). + let _ = DeleteIpForwardEntry(row); + } + } + } + } + /// Add bypass routes for a list of resolved IP addresses (typically from exclusion config). /// Each IP gets a /32 host route via the physical gateway so it bypasses the TUN. /// Returns list of (ip, gw, if_index) that were successfully added, for later cleanup. @@ -178,15 +206,24 @@ pub mod sys { metric: u32, ) -> Vec<(Ipv4Addr, Ipv4Addr, u32)> { let mut added = Vec::new(); + let mut seen = std::collections::HashSet::new(); + let mask = Ipv4Addr::new(255, 255, 255, 255); for &ip in ips { - let mask = Ipv4Addr::new(255, 255, 255, 255); + // The server IP is passed both as server_ip and inside bypass_ips, so + // dedupe to avoid a guaranteed "already exists" failure on the second add. + if !seen.insert(ip) { + continue; + } + // Purge any pre-existing /32 for this dest (e.g. a stale route via an + // old gateway from a previous session) so CreateIpForwardEntry below + // installs the correct one instead of failing with ERROR_OBJECT_ALREADY_EXISTS. + delete_routes_for_dest(ip, mask); match add_ipv4_route(ip, mask, gw, if_index, metric) { Ok(()) => { added.push((ip, gw, if_index)); } Err(e) => { - // 87 = ERROR_INVALID_PARAMETER (route may already exist) - tracing::debug!("bypass route add {ip}/32 via {gw}: {e}"); + tracing::warn!("bypass route add {ip}/32 via {gw} (if {if_index}) failed: {e}"); } } }