From 32a117d763483658212409c04d9188f0f2f74001 Mon Sep 17 00:00:00 2001 From: "claude[bot]" Date: Sat, 7 Feb 2026 22:30:33 +0000 Subject: [PATCH] fix: precise IP matching, safe NAT cleanup, atomic full snapshot replace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use exact "inet /" pattern in is_ip_in_use_on_veth to prevent substring false positives (e.g., 10.1.1.1 matching 10.1.1.10) - Remove unsafe sysctl ip_forward=0 from cleanup_global_nat_if_unused to avoid breaking Docker/K8s/VPNs on the host - Apply rename-swap technique to full snapshot path (matching diff snapshot path) to prevent data loss on crash between remove and rename 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/commands/common.rs | 22 +++++++++++++++------- src/network/bridged.rs | 5 ++++- src/network/portmap.rs | 9 ++++----- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/commands/common.rs b/src/commands/common.rs index e7b13fc6..037f2547 100644 --- a/src/commands/common.rs +++ b/src/commands/common.rs @@ -1031,16 +1031,24 @@ pub async fn create_snapshot_core( .await .context("writing snapshot config")?; - // Atomic rename from temp to final location - // If final exists (e.g., from previous snapshot with same name), remove it first + // Atomic replace: rename old out of the way, then rename new into place. + // Same technique as the diff snapshot path above — avoids a window where + // no snapshot exists if we crash between remove and rename. if snapshot_dir.exists() { - tokio::fs::remove_dir_all(snapshot_dir) + let old_snapshot_dir = snapshot_dir.with_extension("old"); + let _ = tokio::fs::remove_dir_all(&old_snapshot_dir).await; + tokio::fs::rename(snapshot_dir, &old_snapshot_dir) + .await + .context("moving old snapshot out of the way")?; + tokio::fs::rename(&temp_snapshot_dir, snapshot_dir) + .await + .context("renaming temp snapshot to final location")?; + let _ = tokio::fs::remove_dir_all(&old_snapshot_dir).await; + } else { + tokio::fs::rename(&temp_snapshot_dir, snapshot_dir) .await - .context("removing existing snapshot directory")?; + .context("renaming temp snapshot to final location")?; } - tokio::fs::rename(&temp_snapshot_dir, snapshot_dir) - .await - .context("renaming temp snapshot to final location")?; info!( snapshot = %snapshot_config.name, diff --git a/src/network/bridged.rs b/src/network/bridged.rs index 92a53585..4a4e5f53 100644 --- a/src/network/bridged.rs +++ b/src/network/bridged.rs @@ -37,8 +37,11 @@ async fn is_ip_in_use_on_veth(ip: &str) -> bool { }; let stdout = String::from_utf8_lossy(&output.stdout); + // Match "inet /" exactly to avoid substring false positives + // (e.g., checking 10.1.1.1 should not match 10.1.1.10 or 210.1.1.1) + let inet_pattern = format!("inet {}/", ip); for line in stdout.lines() { - if line.contains(ip) && line.contains("veth0-") { + if line.contains(&inet_pattern) && line.contains("veth0-") { return true; } } diff --git a/src/network/portmap.rs b/src/network/portmap.rs index 4037c294..6cb213ca 100644 --- a/src/network/portmap.rs +++ b/src/network/portmap.rs @@ -383,11 +383,10 @@ pub async fn cleanup_global_nat_if_unused() { } } - // Disable IP forwarding - let _ = Command::new("sysctl") - .args(["-w", "net.ipv4.ip_forward=0"]) - .output() - .await; + // Note: we intentionally do NOT disable ip_forward here. + // Other services on the host (Docker, Kubernetes, VPNs, etc.) may depend on + // IP forwarding being enabled. Since we can't know whether fcvm was the one + // that enabled it, the safe default is to leave it on. debug!("global NAT cleanup complete"); }