From cd190337f6b5139c805b1f7a076e32b278947562 Mon Sep 17 00:00:00 2001 From: EJ Campbell Date: Thu, 1 Oct 2026 00:24:45 -0700 Subject: [PATCH] podman run refuses two --publish mappings that claim one host address and port fcvm podman run --publish 8080:80,8080:443 was accepted. Routed and rootless networking then failed the second bind after the VM's network existed, and bridged networking installed both DNAT rules and delivered only the first, so guest port 443 was unreachable with no error. snapshot run --publish already refuses such a list (PortMapping::require_distinct_host_sockets). podman run now parses its --publish specs in one place, publish_mappings, before anything else reads them, and applies the same check there. The check that already ran at that point, a published guest port that --forward-localhost also claims, moves into the function and uses the parsed mappings, where it parsed each spec again and skipped the ones that did not parse. A malformed spec now fails the run before the kernel, the rootfs and the image are looked up. It was already refused later on both paths: by the launch path on a cache miss, and on a cache hit by the restore, which parses the run's specs strictly. Red, with the check removed from publish_mappings: make _test-unit FILTER="-p fcvm --lib -E 'test(/^commands::podman::tests::publish_/)'" Summary [ 5.017s] 1 test run: 0 passed, 1 failed, 731 skipped commands::podman::tests::publish_specs_are_parsed_strictly_for_a_run called `Result::unwrap_err()` on an `Ok` value: [PortMapping { host_ip: None, host_port: 8080, guest_port: 80, proto: Tcp }, PortMapping { host_ip: None, host_port: 8080, guest_port: 443, proto: Tcp }] Green on this commit: the same command Summary [ 0.011s] 1 test run: 1 passed, 731 skipped make test-unit Summary [ 71.285s] 1376 tests run: 1376 passed (1 slow), 0 skipped make lint rc 0 make _test-root FILTER=--no-run rc 0 (compiles the root-only tests, runs none) --- src/commands/podman/mod.rs | 118 ++++++++++++++++++++++++------------- src/network/types.rs | 4 +- 2 files changed, 80 insertions(+), 42 deletions(-) diff --git a/src/commands/podman/mod.rs b/src/commands/podman/mod.rs index 178631adc..e01372c2c 100644 --- a/src/commands/podman/mod.rs +++ b/src/commands/podman/mod.rs @@ -965,6 +965,52 @@ async fn export_localhost_image_disk( Ok(disk_path) } +/// The `--publish` mappings of a run, parsed before anything else reads the specs. +/// +/// A malformed spec fails the run here, before the kernel, the rootfs and the image are +/// looked up. So do two mappings that claim one host address and port: left to network +/// setup, routed and rootless networking fail the second bind after the VM's network +/// exists, and bridged networking installs both DNAT rules and delivers only the first. +fn publish_mappings(args: &RunArgs) -> Result> { + let port_mappings: Vec = args + .publish + .iter() + .map(|s| PortMapping::parse(s)) + .collect::>>() + .context("parsing port mappings")?; + + // --publish now DNATs each published guest port to 127.0.0.1 inside the guest, + // and --forward-localhost BINDS 127.0.0.1: there as a relay to the host. + // Overlap turns the published port into a reflector: an external client reaches + // the HOST's service on that port and never touches the guest. It returns a + // successful response from the wrong machine rather than an error, so it cannot + // be left to be discovered at runtime. The overlap is on the GUEST port, which + // is the second field of --publish. + let forwarded: std::collections::HashSet = + args.forward_localhost.iter().copied().collect(); + for (spec, pm) in args.publish.iter().zip(&port_mappings) { + // TCP only: published_guest_ports carries TCP mappings alone, and the + // --forward-localhost relay is a TCP listener, so `--publish H:G/udp` + // alongside `--forward-localhost G` shares a port NUMBER without ever + // sharing a socket. Rejecting it would regress a valid combination. + if pm.proto == crate::network::Protocol::Tcp && forwarded.contains(&pm.guest_port) { + bail!( + "--publish {spec} and --forward-localhost {} both claim guest port {}. \ + --publish makes that port reach the guest's 127.0.0.1:{}, which is exactly \ + where --forward-localhost binds its relay to the host — so the published \ + port would answer with the HOST's service instead of the guest's. \ + Use different ports.", + pm.guest_port, + pm.guest_port, + pm.guest_port + ); + } + } + PortMapping::require_distinct_host_sockets(&port_mappings)?; + + Ok(port_mappings) +} + async fn prepare_vm_for_lifecycle( mut args: RunArgs, lifecycle: PodmanLifecycle, @@ -1001,38 +1047,7 @@ async fn prepare_vm_for_lifecycle( ); } - // --publish now DNATs each published guest port to 127.0.0.1 inside the guest, - // and --forward-localhost BINDS 127.0.0.1: there as a relay to the host. - // Overlap turns the published port into a reflector: an external client reaches - // the HOST's service on that port and never touches the guest. It returns a - // successful response from the wrong machine rather than an error, so it cannot - // be left to be discovered at runtime. The overlap is on the GUEST port, which - // is the second field of --publish. - { - let forwarded: std::collections::HashSet = - args.forward_localhost.iter().copied().collect(); - for spec in &args.publish { - let Ok(pm) = crate::network::PortMapping::parse(spec) else { - continue; // invalid specs are reported by the parser itself - }; - // TCP only: published_guest_ports carries TCP mappings alone, and the - // --forward-localhost relay is a TCP listener, so `--publish H:G/udp` - // alongside `--forward-localhost G` shares a port NUMBER without ever - // sharing a socket. Rejecting it would regress a valid combination. - if pm.proto == crate::network::Protocol::Tcp && forwarded.contains(&pm.guest_port) { - bail!( - "--publish {spec} and --forward-localhost {} both claim guest port {}. \ - --publish makes that port reach the guest's 127.0.0.1:{}, which is exactly \ - where --forward-localhost binds its relay to the host — so the published \ - port would answer with the HOST's service instead of the guest's. \ - Use different ports.", - pm.guest_port, - pm.guest_port, - pm.guest_port - ); - } - } - } + let port_mappings = publish_mappings(&args)?; // Disallow --setup when running as root // Root users should run `fcvm setup` explicitly @@ -1345,14 +1360,6 @@ async fn prepare_vm_for_lifecycle( let vm_id = generate_vm_id(); let vm_name = args.name.clone(); - // Parse port mappings - let port_mappings: Vec = args - .publish - .iter() - .map(|s| PortMapping::parse(s)) - .collect::>>() - .context("parsing port mappings")?; - // Parse volume mappings (HOST:GUEST[:ro]) let volume_mappings: Vec = args .map @@ -2930,6 +2937,37 @@ mod tests { } } + /// A malformed --publish fails the run by name, and so does a published guest port + /// that --forward-localhost also claims. + #[test] + fn publish_specs_are_parsed_strictly_for_a_run() { + let mut args = test_args(); + args.publish = vec!["8080:80".to_string(), "[::1:9090:90".to_string()]; + let error = format!("{:#}", publish_mappings(&args).unwrap_err()); + assert!(error.contains("[::1:9090:90"), "{error}"); + + args.publish = vec!["8080:80".to_string(), "127.0.0.1:5300:53/udp".to_string()]; + let written: Vec = publish_mappings(&args) + .unwrap() + .iter() + .map(|mapping| mapping.to_string()) + .collect(); + assert_eq!(written, ["8080:80/tcp", "127.0.0.1:5300:53/udp"]); + + args.forward_localhost = vec![80]; + let error = format!("{:#}", publish_mappings(&args).unwrap_err()); + assert!(error.contains("--forward-localhost 80"), "{error}"); + + // One host address and port cannot go to two guest ports. + args.forward_localhost = vec![]; + args.publish = vec!["8080:80".to_string(), "8080:443".to_string()]; + let error = format!("{:#}", publish_mappings(&args).unwrap_err()); + assert!( + error.contains("8080:80/tcp") && error.contains("8080:443/tcp"), + "{error}" + ); + } + /// A cache hit restores through `snapshot run`, where a clone chooses where it /// publishes. The restore has to publish where THIS run asked. #[test] diff --git a/src/network/types.rs b/src/network/types.rs index ac636ce80..030ab8064 100644 --- a/src/network/types.rs +++ b/src/network/types.rs @@ -78,8 +78,8 @@ impl std::fmt::Display for PortMapping { impl PortMapping { /// Parse port mappings leniently, skipping invalid values with a warning. - /// Used for cache key computation where invalid mappings are caught - /// later during actual network setup in `podman/mod.rs`. + /// Used for cache key computation. `podman run` has parsed the same specs strictly + /// by then (`publish_mappings` in `podman/mod.rs`), so nothing is skipped there. pub fn parse_all_lenient(specs: &[String]) -> Vec { specs .iter()