diff --git a/Cargo.lock b/Cargo.lock index e907032d..dbc34892 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2605,8 +2605,9 @@ dependencies = [ [[package]] name = "running-process-core" -version = "3.1.0" -source = "git+https://github.com/zackees/running-process?rev=ff9d7972504f7a0dcee0f410274daf3b02e4fcc2#ff9d7972504f7a0dcee0f410274daf3b02e4fcc2" +version = "3.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "03f35c478beb7dff57f999628b301d5e18306e6fc80f90cf6c6c221904df9b56" dependencies = [ "libc", "portable-pty", diff --git a/Cargo.toml b/Cargo.toml index cd8fd34b..7c35f89a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,12 +71,8 @@ tracing-test = "0.2" # Process containment: all subprocess spawns the daemon performs (compilers, # esptool, qemu, simavr, node, npm, …) and any grandchildren they fork must -# die with the daemon. Pinned to a pre-release rev that exposes the -# `Containment` enum / `spawn_with_containment` API. The published -# `running-process-core` 3.4.x dropped those in favor of -# `ContainedProcessGroup` — fbuild will need a containment-layer -# refactor before it can move to crates.io. See FastLED/fbuild#32. -running-process-core = { git = "https://github.com/zackees/running-process", rev = "ff9d7972504f7a0dcee0f410274daf3b02e4fcc2", package = "running-process-core" } +# die with the daemon. See FastLED/fbuild#32. +running-process-core = "3.4" [profile.dev] debug = 0 diff --git a/crates/fbuild-core/src/containment.rs b/crates/fbuild-core/src/containment.rs index dbdf2763..7ad410f4 100644 --- a/crates/fbuild-core/src/containment.rs +++ b/crates/fbuild-core/src/containment.rs @@ -56,10 +56,7 @@ use std::process::{Child, Command}; use std::sync::OnceLock; -#[cfg(unix)] -use running_process_core::ContainedProcessGroup; -#[cfg(windows)] -use running_process_core::{ContainedChild, ContainedProcessGroup, Containment}; +use running_process_core::{ContainedProcessGroup, ORIGINATOR_ENV_VAR}; /// Global process-wide containment group. Initialised once by the /// daemon; remains `None` in non-daemon contexts (CLI binary, tests). @@ -101,35 +98,48 @@ pub fn is_initialised() -> bool { /// Falls back to uncontained `Command::spawn` when no global group has /// been initialised (non-daemon binaries). /// -/// **Windows**: delegates to `ContainedProcessGroup::spawn` which -/// assigns the child to the Job Object — safe and stateless. +/// **Windows**: spawn directly, then assign the child handle to a Job +/// Object with `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` +/// ([`windows_job::assign`]) — the same containment mechanism the +/// pre-publication `running-process-core` rev used internally, +/// reimplemented locally since the published 3.4 API no longer exposes +/// `spawn_with_containment(_, Containment::Contained)`. /// /// **Unix**: installs a per-child `pre_exec` hook that creates a new /// process group (`setpgid(0, 0)`) and, on Linux, requests -/// `PR_SET_PDEATHSIG(SIGKILL)`. This sidesteps -/// [`ContainedProcessGroup::spawn_with_containment`]'s shared-pgid -/// behaviour, which fails with EPERM on the second spawn after the -/// first child (the pgid leader) has exited — the root cause of +/// `PR_SET_PDEATHSIG(SIGKILL)`. We deliberately do not call +/// `ContainedProcessGroup::spawn` here because the per-child pgid +/// approach sidesteps an EPERM race that hit the second spawn after the +/// first child (the pgid leader) had exited — the root cause of /// FastLED/fbuild#129. The Linux kernel's `PR_SET_PDEATHSIG` still /// enforces the "child dies with daemon" contract; macOS relies on /// process-group-leader death alone. pub fn spawn_contained(command: &mut Command) -> std::io::Result { #[cfg(windows)] { - match GLOBAL_GROUP.get() { - Some(group) => { - let ContainedChild { child, .. } = - group.spawn_with_containment(command, Containment::Contained)?; - Ok(child) - } - None => command.spawn(), + let Some(group) = GLOBAL_GROUP.get() else { + return command.spawn(); + }; + inject_originator_env(command, group); + let mut child = command.spawn()?; + use std::os::windows::io::AsRawHandle; + if let Err(e) = windows_job::assign(child.as_raw_handle()) { + // The atomic spawn+assign that `ContainedProcessGroup::spawn_with_containment` + // used to provide is gone in 3.4. If assign fails after spawn + // succeeds, kill the orphan so the caller can't leak an + // uncontained child by accident. + let _ = child.kill(); + let _ = child.wait(); + return Err(e); } + Ok(child) } #[cfg(unix)] { - if GLOBAL_GROUP.get().is_none() { + let Some(group) = GLOBAL_GROUP.get() else { return command.spawn(); - } + }; + inject_originator_env(command, group); unix_install_pre_exec(command); command.spawn() } @@ -141,28 +151,39 @@ pub fn spawn_contained(command: &mut Command) -> std::io::Result { pub fn spawn_detached(command: &mut Command) -> std::io::Result { #[cfg(windows)] { - match GLOBAL_GROUP.get() { - Some(group) => { - let ContainedChild { child, .. } = - group.spawn_with_containment(command, Containment::Detached)?; - Ok(child) - } - None => command.spawn(), + // Detached: no Job Object assignment so the child survives + // when the daemon's job handle closes. We still inject the + // originator env var for cross-process correlation. + if let Some(group) = GLOBAL_GROUP.get() { + inject_originator_env(command, group); } + command.spawn() } #[cfg(unix)] { // Detached: create a new session so the child survives the // daemon thread that spawned it. Matches the upstream behaviour // but without joining any shared pgid. - if GLOBAL_GROUP.get().is_none() { + let Some(group) = GLOBAL_GROUP.get() else { return command.spawn(); - } + }; + inject_originator_env(command, group); unix_install_detached_pre_exec(command); command.spawn() } } +/// Mirror of `ContainedProcessGroup::inject_originator_env`: stamp +/// `RUNNING_PROCESS_ORIGINATOR=TOOL:PID` onto the command's env. We do +/// this manually because the published 3.4 API only exposes it via +/// `ContainedProcessGroup::spawn` (which returns its own +/// `SpawnedChild`, not a `std::process::Child` — see #32). +fn inject_originator_env(command: &mut Command, group: &ContainedProcessGroup) { + if let Some(value) = group.originator_value() { + command.env(ORIGINATOR_ENV_VAR, value); + } +} + /// Install a `pre_exec` hook that puts the child in a fresh process /// group (Unix) and, on Linux, asks the kernel to SIGKILL the child /// when the spawning thread exits. @@ -240,8 +261,11 @@ pub mod tokio_spawn { pub fn spawn_contained( command: &mut tokio::process::Command, ) -> std::io::Result { - if !super::is_initialised() { + let Some(group) = super::GLOBAL_GROUP.get() else { return command.spawn(); + }; + if let Some(value) = group.originator_value() { + command.env(super::ORIGINATOR_ENV_VAR, value); } configure(command); let child = command.spawn()?; diff --git a/crates/fbuild-core/src/subprocess.rs b/crates/fbuild-core/src/subprocess.rs index 162eb88a..2c8be27c 100644 --- a/crates/fbuild-core/src/subprocess.rs +++ b/crates/fbuild-core/src/subprocess.rs @@ -12,15 +12,18 @@ //! * strip MSYS/MSYS2 env vars that would otherwise poison native //! Windows toolchain binaries. //! -//! Containment is honoured via `ProcessConfig::containment = Some(...)` -//! when the daemon has installed the global containment group. CLI -//! binaries and unit tests run uncontained just as before. +//! On Windows, containment is applied post-spawn by +//! [`crate::containment::windows_job::assign`] when the daemon has +//! installed the global containment group; CLI binaries and unit tests +//! run uncontained just as before. (Earlier code used a `containment:` +//! field on `ProcessConfig` from a pre-release `running-process-core`; +//! the published 3.4 API does not expose that field — see #32.) use std::path::Path; use std::time::Duration; use running_process_core::{ - CommandSpec, Containment, NativeProcess, ProcessConfig, ProcessError, StderrMode, StdinMode, + CommandSpec, NativeProcess, ProcessConfig, ProcessError, StderrMode, StdinMode, }; use crate::{FbuildError, Result}; @@ -153,11 +156,6 @@ fn build_config( create_process_group: false, stdin_mode, nice: None, - containment: if crate::containment::is_initialised() { - Some(Containment::Contained) - } else { - None - }, }) }