diff --git a/bin/controller/CProcessSpawnerRouter.cc b/bin/controller/CProcessSpawnerRouter.cc index c87a17f063..d701cefedb 100644 --- a/bin/controller/CProcessSpawnerRouter.cc +++ b/bin/controller/CProcessSpawnerRouter.cc @@ -149,7 +149,15 @@ CProcessSpawnerRouter::CProcessSpawnerRouter(const TStrVec& permittedProcessPath : m_LegacySpawner{permittedProcessPaths}, m_SandboxedProcessPaths{sandboxedProcessPaths}, m_ConfinementFn{confinementFn ? std::move(confinementFn) : TConfinementFn{[] { return sandbox::hostConfinement(); - }}} { + }}}, + m_ChildIpcReaper{std::make_shared()} { + m_LegacySpawner.setChildIpcDirectoryCallbacks( + [reaper = m_ChildIpcReaper](core::CProcess::TPid pid) { + reaper->onChildExited(pid); + }, + [reaper = m_ChildIpcReaper](core::CProcess::TPid pid, const std::string& root) { + reaper->noteSpawn(pid, root); + }); } const std::string& CProcessSpawnerRouter::lastSpawnFailureReason() const { @@ -252,14 +260,25 @@ bool CProcessSpawnerRouter::spawn(ERoute route, // Derived exactly once per spawn() call, before either backend runs, so // the sandbox2_launch signal below reports the same childId the - // dispatch decision was taken against - see deriveDeploymentId()'s comment for why a - // post-spawn second derivation is not equivalent. Skipped entirely for - // processes that can never emit the signal, so unrelated permitted - // processes (autodetect etc.) pay no ::realpath() cost. + // dispatch decision was taken against - see prepareChildIpcLaunch()'s + // comment for why a post-spawn second derivation is not equivalent. + // Skipped entirely for processes that can never emit the signal, so + // unrelated permitted processes (autodetect etc.) pay no ::realpath() cost. const SPreparedChildIpcLaunch prepared{ sandboxEligible ? prepareChildIpcLaunch(args) : SPreparedChildIpcLaunch{}}; const std::string deploymentId{ sandboxEligible ? prepared.s_Validation.s_Spec.s_ChildId : std::string()}; + std::string childIpcRoot; + if (sandboxEligible) { + if (prepared.s_Validation.s_Ok) { + childIpcRoot = prepared.s_Validation.s_Spec.s_ChildIpcRoot; + } else { + childIpcRoot = sandbox::perChildIpcRootFromArgs( + trustedTmpDirFromEnvironment(), args); + } + } + const std::string* childIpcRootPtr{ + sandboxEligible && childIpcRoot.empty() == false ? &childIpcRoot : nullptr}; m_LastSpawnFailureReason.clear(); bool spawned{false}; @@ -278,7 +297,7 @@ bool CProcessSpawnerRouter::spawn(ERoute route, // sandbox2_launch signal below can report it. LOG_INFO(<< "Launching '" << processPath << "' without Sandbox2 (legacy route selected by the controller); " << "the in-process seccomp filter applies"); - spawned = m_LegacySpawner.spawn(processPath, args, childPid); + spawned = m_LegacySpawner.spawn(processPath, args, childPid, childIpcRootPtr); } else if (sandboxEligible) { // route == ERoute::E_Sandbox2, and processPath is configured as // sandboxed. @@ -305,6 +324,7 @@ bool CProcessSpawnerRouter::spawn(ERoute route, // contract as the legacy spawner - see the member's declaration. if (m_SandboxSpawner == nullptr) { m_SandboxSpawner = std::make_unique(); + m_SandboxSpawner->setChildIpcDirectoryReaper(m_ChildIpcReaper); } // No automatic fallback on a Sandbox2 *failure*: a host that can // run Sandbox2 but fails this launch has a problem worth @@ -335,7 +355,7 @@ bool CProcessSpawnerRouter::spawn(ERoute route, LOG_INFO(<< sandbox::landlockFallbackMessage(host, processPath)); TStrVec landlockArgs{args}; landlockArgs.emplace_back(RESTRICT_FILESYSTEM_TOKEN); - spawned = m_LegacySpawner.spawn(processPath, landlockArgs, childPid); + spawned = m_LegacySpawner.spawn(processPath, landlockArgs, childPid, childIpcRootPtr); break; } case sandbox::EConfinementLevel::E_Unavailable: @@ -369,6 +389,11 @@ bool CProcessSpawnerRouter::spawn(ERoute route, if (sandboxEligible) { this->emitLaunchSignal(route, legacyReason, deploymentId, args, spawned, landlockFallback); + if (spawned == false && childIpcRoot.empty() == false) { + // Backstop for routes/backends that do not self-notify (legacy, + // Landlock, unavailable); Sandbox2 may already have called this. + m_ChildIpcReaper->onSpawnFailed(childIpcRoot); + } } return spawned; diff --git a/bin/controller/CProcessSpawnerRouter.h b/bin/controller/CProcessSpawnerRouter.h index 8a90487dd4..211f3196b4 100644 --- a/bin/controller/CProcessSpawnerRouter.h +++ b/bin/controller/CProcessSpawnerRouter.h @@ -14,6 +14,7 @@ #include #include +#include #include #include @@ -212,6 +213,8 @@ class CProcessSpawnerRouter { //! See lastSpawnFailureReason(). std::string m_LastSpawnFailureReason; + + std::shared_ptr m_ChildIpcReaper; }; } // namespace controller diff --git a/bin/pytorch_inference/Main.cc b/bin/pytorch_inference/Main.cc index 4e1b85ed89..3d456064b3 100644 --- a/bin/pytorch_inference/Main.cc +++ b/bin/pytorch_inference/Main.cc @@ -356,37 +356,13 @@ int main(int argc, char** argv) { return EXIT_FAILURE; } - // Internal switch, deliberately still OFF (log-and-continue on a failed - // in-process seccomp installation, exactly as before typed routing). - // - // Turning it on is only safe once a degraded/legacy-route launch is - // guaranteed to be a deliberate decision rather than an unrequested - // default. CProcessSpawnerRouter supplies half of that guarantee - it - // never falls back to the legacy spawner after a failed Sandbox2 - // attempt - but the controller's no-token case still always takes the - // legacy route (see bin/controller/CCommandProcessor.cc), and a caller - // that omits both routing tokens is not necessarily choosing that - // deliberately. So an ordinary launch with no explicit token is a - // degraded-route launch, and terminating on seccomp-install failure - // would fail every launch on a host lacking usable seccomp BPF - // (restricted containers, some CI images) with no fallback to select - // instead. - // - // Activate this once every caller that matters (in practice, - // Elasticsearch) always sends an explicit --disableSandbox or - // --requireSandbox token per launch, so a degraded launch really is - // only ever reachable via an explicit, controller-validated - // --disableSandbox token, which is what makes hard termination safe - // (track: elastic/ml-cpp#3213). - constexpr bool TERMINATE_ON_DEGRADED_SECCOMP_FAILURE{false}; - // The in-process filter belongs to the legacy/non-sandboxed route only. // On the Sandbox2 route the executor's own policy is already the // security boundary and ML_SANDBOXED is exactly "1", so the whole step - // install, degraded-mode decision, attestation marker - is skipped. // Attempting it from inside an already-sandboxed environment would - // either fail (which would terminate every enforced-route launch once - // hard termination above is activated) or succeed and emit the + // either fail (which would terminate every enforced-route launch when + // TERMINATE_ON_DEGRADED_SECCOMP_FAILURE is true) or succeed and emit the // legacy-route attestation marker on a launch the controller's // sandbox2_launch signal reports as "route":"sandbox2". const bool sandbox2Launched{ml::seccomp::sandbox2LaunchedChild()}; @@ -394,7 +370,7 @@ int main(int argc, char** argv) { // are meant to stack - so its attestation names that route, matching the // controller's sandbox2_launch signal for this launch. const ml::seccomp::SInProcessFilterResult seccompResult{ml::seccomp::applyInProcessSeccompFilter( - sandbox2Launched, TERMINATE_ON_DEGRADED_SECCOMP_FAILURE, + sandbox2Launched, ml::seccomp::TERMINATE_ON_DEGRADED_SECCOMP_FAILURE, [] { return ml::seccomp::CSystemCallFilter::installSystemCallFilter(); }, restrictFilesystem ? "landlock" : "legacy")}; diff --git a/docs/sandbox2_production_failure_modes.md b/docs/sandbox2_production_failure_modes.md index 70061d2280..319208fba6 100644 --- a/docs/sandbox2_production_failure_modes.md +++ b/docs/sandbox2_production_failure_modes.md @@ -126,12 +126,13 @@ in-process filter. Only `CSandboxedProcessSpawner` sets it, and only on real sandboxees. Hard termination on a failed in-process seccomp installation -(`TERMINATE_ON_DEGRADED_SECCOMP_FAILURE` in -`bin/pytorch_inference/Main.cc`) is deliberately **off**: an ordinary launch -with no explicit routing token is a degraded-route launch, so terminating -would fail every launch on a host without usable seccomp BPF. It becomes -safe to activate once every caller that matters always sends an explicit -`--disableSandbox` or `--requireSandbox` token per launch. +(`TERMINATE_ON_DEGRADED_SECCOMP_FAILURE` in `include/seccomp/CSystemCallFilter.h`) +is **on** for legacy and Landlock launches: if the filter cannot be installed, +`pytorch_inference` exits before reading untrusted model bytes. Elasticsearch +sends an explicit `--disableSandbox` or `--requireSandbox` token on every Linux +production launch; direct controller invocations with no token still take the +legacy route and fail closed the same way. Sandbox2 children (`ML_SANDBOXED=1`) +never install this filter. Example: diff --git a/include/core/CDetachedProcessSpawner.h b/include/core/CDetachedProcessSpawner.h index 2b28f518f3..2874a1b94d 100644 --- a/include/core/CDetachedProcessSpawner.h +++ b/include/core/CDetachedProcessSpawner.h @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -154,6 +155,25 @@ class CORE_EXPORT CDetachedProcessSpawner { //! started. bool spawn(const std::string& processPath, const TStrVec& args, CProcess::TPid& childPid); + //! As above. When \p childIpcRoot is non-null and non-empty, invokes the + //! child-spawned IPC callback (see setChildIpcDirectoryCallbacks()) with + //! the new pid and root before the tracker mutex is released, so a fast + //! child exit cannot be observed before the binding exists. + bool spawn(const std::string& processPath, + const TStrVec& args, + CProcess::TPid& childPid, + const std::string* childIpcRoot); + + //! Optional hooks for CChildIpcDirectoryReaper wiring from the controller + //! router. The exit callback is invoked from the tracker thread after a + //! reaped pid is removed from the live set; it must not call back into + //! this spawner. + using TChildExitedCallback = std::function; + using TChildSpawnedIpcCallback = + std::function; + void setChildIpcDirectoryCallbacks(TChildExitedCallback onChildExited, + TChildSpawnedIpcCallback onChildSpawnedWithIpc); + //! Kill the child process with the specified PID. If there is a //! process running with the specified PID that was not spawned by this //! object then it will NOT be killed. @@ -171,6 +191,8 @@ class CORE_EXPORT CDetachedProcessSpawner { //! alive. TTrackerThreadP m_TrackerThread; + TChildSpawnedIpcCallback m_OnChildSpawnedWithIpc; + #ifndef Windows //! On *nix testing which files need to be closed when spawning a process //! can be expensive, so this variable is used to learn the highest file diff --git a/include/sandbox/CChildIpcDirectoryReaper.h b/include/sandbox/CChildIpcDirectoryReaper.h new file mode 100644 index 0000000000..71811d6fb8 --- /dev/null +++ b/include/sandbox/CChildIpcDirectoryReaper.h @@ -0,0 +1,60 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ +#ifndef INCLUDED_ml_sandbox_CChildIpcDirectoryReaper_h +#define INCLUDED_ml_sandbox_CChildIpcDirectoryReaper_h + +#include + +#include +#include +#include + +namespace ml { +namespace sandbox { + +//! Removes a single per-deployment directory under $TMPDIR/ml-child-ipc/ +//! once the controller no longer has a live child bound to it. +//! +//! Lifecycle (paired with ensureChildIpcDirectory()): +//! - ensureChildIpcDirectory() creates $TMPDIR/ml-child-ipc/ (0700) before +//! realpath()/policy construction; the intermediate ml-child-ipc directory is kept. +//! - noteSpawn() records which pid owns that canonical root for a successful launch. +//! - onChildExited() removes the directory only when the root is still bound to that pid +//! (a fast restart reusing the same deployment id updates the binding first). +//! - onSpawnFailed() removes a root that was created but never got a live pid. +//! Ownership validation and directory removal run under one lock: a concurrent +//! noteSpawn() for the same deployment root cannot interleave between unbinding +//! and unlink/rmdir. +//! +//! Controller crash can still leave empty directories; there is no startup sweep because +//! that would race another controller on the same $TMPDIR. +class CChildIpcDirectoryReaper { +public: + void noteSpawn(core::CProcess::TPid pid, const std::string& canonicalChildRoot); + + void onChildExited(core::CProcess::TPid pid); + + void onSpawnFailed(const std::string& canonicalChildRoot); + +private: + static bool canonicalPerChildIpcRootHasExpectedShape(const std::string& canonicalChildRoot); + + static bool tryRemovePerChildIpcDirectory(const std::string& canonicalChildRoot); + + std::mutex m_Mutex; + std::map m_PidToRoot; + std::map m_RootToPid; +}; + +} // namespace sandbox +} // namespace ml + +#endif // INCLUDED_ml_sandbox_CChildIpcDirectoryReaper_h diff --git a/include/sandbox/CPytorchInferenceSandboxPolicy.h b/include/sandbox/CPytorchInferenceSandboxPolicy.h index 6d647dfc7c..9634b3cce2 100644 --- a/include/sandbox/CPytorchInferenceSandboxPolicy.h +++ b/include/sandbox/CPytorchInferenceSandboxPolicy.h @@ -124,10 +124,11 @@ enum class EChildIpcDirectoryOutcome { //! the sandbox2_launch signal, which runs even on the legacy route) - must //! call this first. //! -//! The per-child directory is not removed here: it is keyed by deployment -//! id, reused across controller/process restarts for the same id, and is -//! empty once pytorch_inference has unlinked its FIFOs. Removing it is the -//! caller's responsibility once the deployment ends (elastic/ml-cpp#3214). +//! The per-child directory is not removed here. Once pytorch_inference has +//! unlinked its FIFOs the directory is usually empty; CChildIpcDirectoryReaper +//! removes it when the controller observes that child exit (or after a failed +//! spawn that never bound a live pid). The parent $TMPDIR/ml-child-ipc +//! directory is never removed here. //! //! Idempotent: an already-existing directory is E_Ready, not an error, so a //! retry/restart that reuses the same child-id never fails here. Uses only @@ -139,6 +140,13 @@ enum class EChildIpcDirectoryOutcome { EChildIpcDirectoryOutcome ensureChildIpcDirectory(const std::string& trustedTmpDir, const std::vector& args); +//! Returns the canonical per-child IPC root when validation succeeds, or the +//! literal $TMPDIR/ml-child-ipc/ path implied by args when a path +//! option matches the expected shape (used to reap directories left behind by +//! a failed spawn after ensureChildIpcDirectory()). +std::string perChildIpcRootFromArgs(const std::string& trustedTmpDir, + const std::vector& args); + #ifdef SANDBOX2_AVAILABLE //! Builds the filesystem and network-shape portion of the pytorch_inference diff --git a/include/sandbox/CSandboxedProcessSpawner.h b/include/sandbox/CSandboxedProcessSpawner.h index a84aa28f2d..86e6d80192 100644 --- a/include/sandbox/CSandboxedProcessSpawner.h +++ b/include/sandbox/CSandboxedProcessSpawner.h @@ -44,6 +44,8 @@ class Sandbox2; namespace ml { namespace sandbox { +class CChildIpcDirectoryReaper; + //! \brief //! Spawn and own the lifecycle of processes inside a Sandbox2 isolation //! boundary. @@ -269,6 +271,8 @@ class CSandboxedProcessSpawner { //! PID that is still live (not yet Reaped or Failed). bool hasChild(core::CProcess::TPid pid) const; + void setChildIpcDirectoryReaper(const std::shared_ptr& reaper); + private: const TPidRegistryPtr m_PidRegistry{std::make_shared()}; @@ -282,6 +286,8 @@ class CSandboxedProcessSpawner { #ifdef SANDBOX2_AVAILABLE TAwaitResultFn m_AwaitResultFn; #endif + + std::shared_ptr m_ChildIpcReaper; }; } // namespace sandbox diff --git a/include/seccomp/CSystemCallFilter.h b/include/seccomp/CSystemCallFilter.h index 7dbae3ec29..df74b7bddb 100644 --- a/include/seccomp/CSystemCallFilter.h +++ b/include/seccomp/CSystemCallFilter.h @@ -83,25 +83,21 @@ enum class EDegradedModeAction { //! Pure decision function: does this install outcome require terminating //! before untrusted IO/model processing? //! -//! terminateOnFailure is an internal switch, not an operator setting. Every -//! degraded-mode seccomp failure should eventually terminate before -//! processing, but flipping that on for every call site before the -//! ml-cpp/Elasticsearch controller protocol can guarantee a degraded-mode -//! launch was a deliberate operator choice would fail every launch on a -//! host lacking seccomp BPF, with no operator fallback setting to select -//! instead. It is only safe to pass true where a degraded-mode launch is -//! guaranteed to be a deliberate route decision rather than the production -//! default. bin/controller's CProcessSpawnerRouter provides half of that -//! guarantee (it never retries a failed Sandbox2 spawn through the legacy -//! spawner), but while CCommandProcessor's no-token case still always -//! routes to legacy, and no caller is yet guaranteed to always send an -//! explicit --disableSandbox/--requireSandbox token, an ordinary launch -//! *is* a degraded-route launch, so bin/pytorch_inference/Main.cc passes -//! false. See the comment at TERMINATE_ON_DEGRADED_SECCOMP_FAILURE there -//! for when it flips. -//! This decision only ever -//! applies to a launch that installs its own in-process filter at all - see -//! sandbox2LaunchedChild() and applyInProcessSeccompFilter() below. +//! Legacy and Landlock pytorch_inference launches pass this to +//! applyInProcessSeccompFilter(). When true, a failed in-process filter +//! installation terminates before untrusted model IO. Elasticsearch sends an +//! explicit `--disableSandbox` or `--requireSandbox` token on every Linux +//! production launch; direct controller invocations with no token still take +//! the legacy route and fail closed here too. Sandbox2 children +//! (`ML_SANDBOXED=1`) never install this filter regardless of this value. +inline constexpr bool TERMINATE_ON_DEGRADED_SECCOMP_FAILURE{true}; + +//! Maps a filter installation outcome to the action the pytorch_inference +//! startup path must take before model IO. \p terminateOnFailure is normally +//! TERMINATE_ON_DEGRADED_SECCOMP_FAILURE above. +//! This decision only ever applies to a launch that installs its own +//! in-process filter at all - see sandbox2LaunchedChild() and +//! applyInProcessSeccompFilter() below. inline EDegradedModeAction decideDegradedModeAction(ESystemCallFilterInstallOutcome outcome, bool terminateOnFailure) { if (outcome == ESystemCallFilterInstallOutcome::E_Installed || !terminateOnFailure) { @@ -117,8 +113,8 @@ inline EDegradedModeAction decideDegradedModeAction(ESystemCallFilterInstallOutc //! before initIo() ran" — with an explicit signal a test or observer can //! assert on directly. Returns empty when installation did not succeed so //! this marker can never falsely attest a filter that isn't there. -//! Terminate-before-initIo() applies only when decideDegradedModeAction() -//! is called with terminateOnFailure true (not today's production default). +//! Terminate-before-initIo() applies when decideDegradedModeAction() is +//! called with terminateOnFailure true (TERMINATE_ON_DEGRADED_SECCOMP_FAILURE). //! Logged over the existing per-process log pipe; this is not a new startup //! channel. //! @@ -188,7 +184,7 @@ struct SInProcessFilterResult { //! marker is produced, regardless of what an installation attempt //! would have returned. Installing an in-process filter from inside //! an already-sandboxed environment can fail (which would kill every -//! enforced-route launch once TERMINATE_ON_DEGRADED_SECCOMP_FAILURE is activated) or +//! enforced-route launch when TERMINATE_ON_DEGRADED_SECCOMP_FAILURE is true) or //! succeed and mislabel the launch as legacy. //! \param terminateOnFailure passed through to decideDegradedModeAction(). //! \param installer invoked at most once; normally diff --git a/lib/core/CDetachedProcessSpawner.cc b/lib/core/CDetachedProcessSpawner.cc index 1ec2f0b17f..032f229aca 100644 --- a/lib/core/CDetachedProcessSpawner.cc +++ b/lib/core/CDetachedProcessSpawner.cc @@ -17,7 +17,9 @@ #include #include +#include #include +#include #include #include @@ -127,6 +129,11 @@ class CTrackerThread : public CThread { //! conditions. CMutex& mutex() { return m_Mutex; } + void setOnChildExited(std::function onChildExited) { + CScopedLock lock(m_Mutex); + m_OnChildExited = std::move(onChildExited); + } + //! Add a PID to track. void addPid(CProcess::TPid pid) { CScopedLock lock(m_Mutex); @@ -160,27 +167,50 @@ class CTrackerThread : public CThread { return false; } - CScopedLock lock(m_Mutex); - // Do an extra cycle of waiting for zombies, so we give the most - // up-to-date answer possible - const_cast(this)->checkForDeadChildren(); - return m_Pids.find(pid) != m_Pids.end(); + std::vector exitedPids; + std::function onChildExited; + bool found{false}; + { + CScopedLock lock(m_Mutex); + // Do an extra cycle of waiting for zombies, so we give the most + // up-to-date answer possible + const_cast(this)->checkForDeadChildren(exitedPids); + onChildExited = m_OnChildExited; + found = m_Pids.find(pid) != m_Pids.end(); + } + // The reaper may unlink directories; keep that IO off the tracker mutex + // so spawn() is not blocked behind a slow $TMPDIR. + for (CProcess::TPid exitedPid : exitedPids) { + if (onChildExited) { + onChildExited(exitedPid); + } + } + return found; } protected: void run() override { - CScopedLock lock(m_Mutex); - while (!m_Shutdown) { - // Reap zombies every 50ms if child processes are running, - // otherwise wait for a child process to start. - if (m_Pids.empty()) { - m_Condition.wait(); - } else { - m_Condition.wait(50); - } + std::vector exitedPids; + std::function onChildExited; + { + CScopedLock lock(m_Mutex); + // Reap zombies every 50ms if child processes are running, + // otherwise wait for a child process to start. + if (m_Pids.empty()) { + m_Condition.wait(); + } else { + m_Condition.wait(50); + } - this->checkForDeadChildren(); + this->checkForDeadChildren(exitedPids); + onChildExited = m_OnChildExited; + } + for (CProcess::TPid exitedPid : exitedPids) { + if (onChildExited) { + onChildExited(exitedPid); + } + } } } @@ -193,8 +223,9 @@ class CTrackerThread : public CThread { private: //! Reap zombie child processes and adjust the set of live child PIDs - //! accordingly. MUST be called with m_Mutex locked. - void checkForDeadChildren() { + //! accordingly. MUST be called with m_Mutex locked. Appends each reaped + //! PID to \p exitedPids for the caller to notify outside the lock. + void checkForDeadChildren(std::vector& exitedPids) { int status = 0; for (;;) { CProcess::TPid pid = ::waitpid(-1, &status, WNOHANG); @@ -243,6 +274,7 @@ class CTrackerThread : public CThread { } } m_Pids.erase(pid); + exitedPids.push_back(pid); } } } @@ -250,6 +282,7 @@ class CTrackerThread : public CThread { private: bool m_Shutdown; TPidSet m_Pids; + std::function m_OnChildExited; mutable CMutex m_Mutex; CCondition m_Condition; }; @@ -269,6 +302,12 @@ CDetachedProcessSpawner::~CDetachedProcessSpawner() { } } +void CDetachedProcessSpawner::setChildIpcDirectoryCallbacks(TChildExitedCallback onChildExited, + TChildSpawnedIpcCallback onChildSpawnedWithIpc) { + m_OnChildSpawnedWithIpc = std::move(onChildSpawnedWithIpc); + m_TrackerThread->setOnChildExited(std::move(onChildExited)); +} + bool CDetachedProcessSpawner::spawn(const std::string& processPath, const TStrVec& args) { CProcess::TPid dummy(0); return this->spawn(processPath, args, dummy); @@ -277,6 +316,13 @@ bool CDetachedProcessSpawner::spawn(const std::string& processPath, const TStrVe bool CDetachedProcessSpawner::spawn(const std::string& processPath, const TStrVec& args, CProcess::TPid& childPid) { + return this->spawn(processPath, args, childPid, nullptr); +} + +bool CDetachedProcessSpawner::spawn(const std::string& processPath, + const TStrVec& args, + CProcess::TPid& childPid, + const std::string* childIpcRoot) { if (std::find(m_PermittedProcessPaths.begin(), m_PermittedProcessPaths.end(), processPath) == m_PermittedProcessPaths.end()) { LOG_ERROR(<< "Spawning process '" << processPath << "' is not permitted"); @@ -348,6 +394,9 @@ bool CDetachedProcessSpawner::spawn(const std::string& processPath, return false; } + if (childIpcRoot != nullptr && childIpcRoot->empty() == false && m_OnChildSpawnedWithIpc) { + m_OnChildSpawnedWithIpc(childPid, *childIpcRoot); + } m_TrackerThread->addPid(childPid); } diff --git a/lib/core/CDetachedProcessSpawner_Windows.cc b/lib/core/CDetachedProcessSpawner_Windows.cc index 2c2a81ef14..08d7e6f26a 100644 --- a/lib/core/CDetachedProcessSpawner_Windows.cc +++ b/lib/core/CDetachedProcessSpawner_Windows.cc @@ -206,6 +206,12 @@ CDetachedProcessSpawner::~CDetachedProcessSpawner() { } } +void CDetachedProcessSpawner::setChildIpcDirectoryCallbacks(TChildExitedCallback onChildExited, + TChildSpawnedIpcCallback onChildSpawnedWithIpc) { + m_OnChildSpawnedWithIpc = std::move(onChildSpawnedWithIpc); + // Per-child ml-child-ipc directories are a Linux controller contract only. +} + bool CDetachedProcessSpawner::spawn(const std::string& processPath, const TStrVec& args) { CProcess::TPid dummy(0); return this->spawn(processPath, args, dummy); @@ -214,6 +220,13 @@ bool CDetachedProcessSpawner::spawn(const std::string& processPath, const TStrVe bool CDetachedProcessSpawner::spawn(const std::string& processPath, const TStrVec& args, CProcess::TPid& childPid) { + return this->spawn(processPath, args, childPid, nullptr); +} + +bool CDetachedProcessSpawner::spawn(const std::string& processPath, + const TStrVec& args, + CProcess::TPid& childPid, + const std::string* /* childIpcRoot */) { if (std::find(m_PermittedProcessPaths.begin(), m_PermittedProcessPaths.end(), processPath) == m_PermittedProcessPaths.end()) { LOG_ERROR(<< "Spawning process '" << processPath << "' is not permitted"); diff --git a/lib/core/unittest/CDetachedProcessSpawnerTest.cc b/lib/core/unittest/CDetachedProcessSpawnerTest.cc index 4a27865345..3d05c88aaa 100644 --- a/lib/core/unittest/CDetachedProcessSpawnerTest.cc +++ b/lib/core/unittest/CDetachedProcessSpawnerTest.cc @@ -17,6 +17,7 @@ #include +#include #include #include #include @@ -171,6 +172,22 @@ BOOST_AUTO_TEST_CASE(testNonExistent) { } #ifndef Windows +BOOST_AUTO_TEST_CASE(testChildExitedCallbackFiresForSpawnedChild) { + std::atomic callbackFired{false}; + ml::core::CDetachedProcessSpawner::TStrVec permittedPaths{"/bin/sh"}; + ml::core::CDetachedProcessSpawner spawner(permittedPaths); + spawner.setChildIpcDirectoryCallbacks( + [&](ml::core::CProcess::TPid) { callbackFired.store(true); }, + [](ml::core::CProcess::TPid, const std::string&) {}); + + ml::core::CProcess::TPid childPid{0}; + BOOST_TEST_REQUIRE(spawner.spawn( + "/bin/sh", ml::core::CDetachedProcessSpawner::TStrVec{"-c", "true"}, childPid)); + + std::this_thread::sleep_for(std::chrono::milliseconds(500)); + BOOST_TEST_REQUIRE(callbackFired.load()); +} + BOOST_AUTO_TEST_CASE(testMlSandboxedStrippedFromChildEnvironment) { // ML_SANDBOXED=1 is the Sandbox2 sandboxee marker // (lib/sandbox/CSandboxedProcessSpawner_Linux.cc) and pytorch_inference diff --git a/lib/sandbox/CChildIpcDirectoryReaper.cc b/lib/sandbox/CChildIpcDirectoryReaper.cc new file mode 100644 index 0000000000..d2e08d33e8 --- /dev/null +++ b/lib/sandbox/CChildIpcDirectoryReaper.cc @@ -0,0 +1,181 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the following additional limitation. Functionality enabled by the + * files subject to the Elastic License 2.0 may only be used in production when + * invoked by an Elasticsearch process with a license key installed that permits + * use of machine learning features. You may not use this file except in + * compliance with the Elastic License 2.0 and the foregoing additional + * limitation. + */ +#include + +#include + +#ifndef _WIN32 +#include +#include +#include +#endif + +#include +#include +#include + +namespace { + +#ifndef _WIN32 +std::vector splitPathComponents(const std::string& path) { + std::vector components; + std::size_t start{0}; + while (start < path.size()) { + const std::size_t end{path.find('/', start)}; + if (end == start) { + start = end + 1; + continue; + } + if (end == std::string::npos) { + components.push_back(path.substr(start)); + break; + } + components.push_back(path.substr(start, end - start)); + start = end + 1; + } + return components; +} +#endif + +} // namespace + +namespace ml { +namespace sandbox { + +bool CChildIpcDirectoryReaper::canonicalPerChildIpcRootHasExpectedShape(const std::string& canonicalChildRoot) { +#ifndef _WIN32 + if (canonicalChildRoot.empty()) { + return false; + } + const std::vector components{splitPathComponents(canonicalChildRoot)}; + if (components.size() < 2) { + return false; + } + return components[components.size() - 2] == "ml-child-ipc"; +#else + return false; +#endif +} + +bool CChildIpcDirectoryReaper::tryRemovePerChildIpcDirectory(const std::string& canonicalChildRoot) { +#ifndef _WIN32 + if (canonicalPerChildIpcRootHasExpectedShape(canonicalChildRoot) == false) { + LOG_WARN(<< "Refusing to remove per-child IPC directory with unexpected shape: " + << canonicalChildRoot); + return false; + } + + static const char* const KNOWN_FIFO_LEAVES[]{"input", "output", "restore", "logPipe"}; + for (const char* leaf : KNOWN_FIFO_LEAVES) { + const std::string fifoPath{canonicalChildRoot + '/' + leaf}; + struct stat pathStat {}; + if (::lstat(fifoPath.c_str(), &pathStat) != 0) { + continue; + } + if (S_ISFIFO(pathStat.st_mode)) { + if (::unlink(fifoPath.c_str()) != 0 && errno != ENOENT) { + LOG_WARN(<< "Failed to unlink FIFO " << fifoPath << ": " + << ::strerror(errno)); + } + } + } + + DIR* dir{::opendir(canonicalChildRoot.c_str())}; + if (dir == nullptr) { + return errno == ENOENT; + } + bool unexpectedEntry{false}; + for (;;) { + errno = 0; + const dirent* entry{::readdir(dir)}; + if (entry == nullptr) { + break; + } + const char* name{entry->d_name}; + if (name[0] == '.' && (name[1] == '\0' || (name[1] == '.' && name[2] == '\0'))) { + continue; + } + unexpectedEntry = true; + LOG_DEBUG(<< "Leaving per-child IPC directory " << canonicalChildRoot + << " in place: unexpected entry '" << name << '\''); + break; + } + ::closedir(dir); + if (unexpectedEntry) { + return false; + } + + if (::rmdir(canonicalChildRoot.c_str()) != 0) { + if (errno == ENOENT) { + return true; + } + LOG_WARN(<< "Failed to remove per-child IPC directory " + << canonicalChildRoot << ": " << ::strerror(errno)); + return false; + } + LOG_DEBUG(<< "Removed per-child IPC directory " << canonicalChildRoot); + return true; +#else + return false; +#endif +} + +void CChildIpcDirectoryReaper::noteSpawn(core::CProcess::TPid pid, + const std::string& canonicalChildRoot) { + if (pid <= 0 || canonicalChildRoot.empty()) { + return; + } + std::lock_guard lock(m_Mutex); + const auto previousRootIt{m_PidToRoot.find(pid)}; + if (previousRootIt != m_PidToRoot.end() && previousRootIt->second != canonicalChildRoot) { + const auto rootOwnerIt{m_RootToPid.find(previousRootIt->second)}; + if (rootOwnerIt != m_RootToPid.end() && rootOwnerIt->second == pid) { + m_RootToPid.erase(rootOwnerIt); + } + } + m_PidToRoot[pid] = canonicalChildRoot; + m_RootToPid[canonicalChildRoot] = pid; +} + +void CChildIpcDirectoryReaper::onChildExited(core::CProcess::TPid pid) { + if (pid <= 0) { + return; + } + std::lock_guard lock(m_Mutex); + const auto pidIt{m_PidToRoot.find(pid)}; + if (pidIt == m_PidToRoot.end()) { + return; + } + const std::string root{pidIt->second}; + m_PidToRoot.erase(pidIt); + const auto rootIt{m_RootToPid.find(root)}; + if (rootIt == m_RootToPid.end() || rootIt->second != pid) { + // A newer spawn reused this deployment id; do not remove its directory. + return; + } + m_RootToPid.erase(rootIt); + tryRemovePerChildIpcDirectory(root); +} + +void CChildIpcDirectoryReaper::onSpawnFailed(const std::string& canonicalChildRoot) { + if (canonicalChildRoot.empty()) { + return; + } + std::lock_guard lock(m_Mutex); + const auto rootIt{m_RootToPid.find(canonicalChildRoot)}; + if (rootIt != m_RootToPid.end()) { + return; + } + tryRemovePerChildIpcDirectory(canonicalChildRoot); +} + +} // namespace sandbox +} // namespace ml diff --git a/lib/sandbox/CMakeLists.txt b/lib/sandbox/CMakeLists.txt index ed2faa34f3..4ab5c4eb09 100644 --- a/lib/sandbox/CMakeLists.txt +++ b/lib/sandbox/CMakeLists.txt @@ -25,6 +25,7 @@ set(ML_LINK_LIBRARIES ) set(SRCS + CChildIpcDirectoryReaper.cc CMlSandboxAvailability.cc CPytorchInferenceSandboxPolicy.cc CSandbox2Diagnostics_Linux.cc diff --git a/lib/sandbox/CPytorchInferenceSandboxPolicy.cc b/lib/sandbox/CPytorchInferenceSandboxPolicy.cc index e01d58bc19..06bf0a9deb 100644 --- a/lib/sandbox/CPytorchInferenceSandboxPolicy.cc +++ b/lib/sandbox/CPytorchInferenceSandboxPolicy.cc @@ -452,6 +452,59 @@ EChildIpcDirectoryOutcome ensureChildIpcDirectory(const std::string& trustedTmpD return EChildIpcDirectoryOutcome::E_Ready; } +std::string perChildIpcRootFromArgs(const std::string& trustedTmpDir, + const std::vector& args) { + const SChildIpcValidationResult validated{validateChildIpcLaunchSpec(trustedTmpDir, args)}; + if (validated.s_Ok) { + return validated.s_Spec.s_ChildIpcRoot; + } + + std::string base{trustedTmpDir}; + while (base.empty() == false && base.back() == '/') { + base.pop_back(); + } + const std::string mlChildIpcDir{base + "/ml-child-ipc"}; + const std::string expectedPrefix{mlChildIpcDir + "/"}; + + for (const std::string& arg : args) { + const std::size_t eqPos = arg.find('='); + if (eqPos == std::string::npos) { + continue; + } + + std::string optionName{arg.substr(0, eqPos)}; + while (optionName.empty() == false && optionName[0] == '-') { + optionName.erase(0, 1); + } + if (isPathOptionName(optionName) == false) { + continue; + } + + const std::string value{eqPos + 1 < arg.size() ? arg.substr(eqPos + 1) + : std::string{}}; + if (value.empty() || value[0] != '/') { + continue; + } + + const std::vector components{splitPathComponents(value)}; + if (containsDotDot(components) || components.size() < 2) { + continue; + } + + const std::size_t lastSlash = value.rfind('/'); + const std::string literalParent{value.substr(0, lastSlash)}; + if (literalParent.compare(0, expectedPrefix.size(), expectedPrefix) != 0) { + continue; + } + const std::string candidateChildId{literalParent.substr(expectedPrefix.size())}; + if (candidateChildId.empty() || candidateChildId.find('/') != std::string::npos) { + continue; + } + return mlChildIpcDir + "/" + candidateChildId; + } + return std::string(); +} + #ifdef SANDBOX2_AVAILABLE absl::StatusOr diff --git a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc index 7150dfb9d6..f91ddb1e2e 100644 --- a/lib/sandbox/CSandboxedProcessSpawner_Linux.cc +++ b/lib/sandbox/CSandboxedProcessSpawner_Linux.cc @@ -11,6 +11,7 @@ #include #include +#include #include #include @@ -398,6 +399,11 @@ CSandboxedProcessSpawner::CSandboxedProcessSpawner(TPidFdOpenFn pidFdOpenFn, CSandboxedProcessSpawner::~CSandboxedProcessSpawner() = default; +void CSandboxedProcessSpawner::setChildIpcDirectoryReaper( + const std::shared_ptr& reaper) { + m_ChildIpcReaper = reaper; +} + bool CSandboxedProcessSpawner::spawn(const std::string& processPath, const TStrVec& args, core::CProcess::TPid& childPid) { @@ -450,6 +456,18 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, // false must fail the spawn outright - never fall back to a // partially-built policy. const SChildIpcValidationResult validated{validateChildIpcLaunchSpec(trustedTmpDir, args)}; + const std::string childIpcRoot{validated.s_Spec.s_ChildIpcRoot}; + const auto notifySpawnFailed = [this, &trustedTmpDir, &args, &childIpcRoot]() { + if (m_ChildIpcReaper == nullptr) { + return; + } + const std::string root{childIpcRoot.empty() + ? perChildIpcRootFromArgs(trustedTmpDir, args) + : childIpcRoot}; + if (root.empty() == false) { + m_ChildIpcReaper->onSpawnFailed(root); + } + }; if (validated.s_Ok == false) { std::ostringstream rejected; for (const SRejectedChildIpcPath& r : validated.s_Rejected) { @@ -458,6 +476,7 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, } LOG_ERROR(<< "Rejected pytorch_inference child-IPC launch spec for " << processPath << ':' << rejected.str()); + notifySpawnFailed(); return false; } @@ -484,6 +503,7 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, if (!built.ok()) { LOG_ERROR(<< "Failed to build Sandbox2 policy for " << processPath << ": " << built.status()); + notifySpawnFailed(); return false; } sandbox2::PolicyBuilder policyBuilder{std::move(*built)}; @@ -491,6 +511,7 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, auto policyResult = policyBuilder.TryBuild(); if (!policyResult.ok()) { LOG_ERROR(<< "Failed to build Sandbox2 policy for " << processPath); + notifySpawnFailed(); return false; } @@ -515,6 +536,7 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, } catch (const std::exception& e) { LOG_ERROR(<< "Failed to take shared ownership of a sandboxee for " << processPath << ": " << e.what()); + notifySpawnFailed(); return false; } @@ -531,6 +553,7 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, LOG_ERROR(<< "Sandbox2 failed to start " << processPath << ": status=" << sandbox2::Result::StatusEnumToString(result.final_status()) << " reason=" << result.reason_code() << " (" << result.ToString() << ')'); + notifySpawnFailed(); return false; } @@ -539,6 +562,7 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, sandbox->AwaitResult(); childPid = 0; LOG_ERROR(<< "Sandbox2 returned an invalid PID for " << processPath); + notifySpawnFailed(); return false; } @@ -554,141 +578,171 @@ bool CSandboxedProcessSpawner::spawn(const std::string& processPath, // monitor-launch-span throw, monitor-launch-seam false - independent of // when `sandbox`/`child.s_Sandbox` themselves get destroyed during // stack unwinding. - CKillAndReapGuard killAndReapGuard{sandbox, m_AwaitResultFn}; - - const SPidFdAcquisitionResult pidFdResult{ - m_PidFdOpenFn ? m_PidFdOpenFn(sandboxPid) : defaultPidFdOpen(sandboxPid)}; - CScopedPidFd pidFdGuard{pidFdResult.s_Fd}; - const EPidFdOutcome pidFdOutcome{classifyPidFdOutcome(pidFdResult)}; - - // An errno other than ENOSYS (ESRCH, EMFILE, ENFILE, ...) is a - // resource/identity error, not "no kernel support" for pidfd - it must - // never be treated the same as E_KernelUnsupported. Fail registration - // outright rather than register a child whose termination would need an - // undefined fallback. pidFdGuard closes any fd this path somehow still - // holds; killAndReapGuard (still armed) Kill()s/awaits the sandboxee. - if (pidFdOutcome == EPidFdOutcome::E_Failed) { - LOG_ERROR(<< "pidfd_open failed for sandboxed process " << processPath - << " (PID " << sandboxPid << ") with errno " - << pidFdResult.s_Errno << " (" << ::strerror(pidFdResult.s_Errno) - << "); refusing to register a child with an undefined termination fallback"); - childPid = 0; - return false; // killAndReapGuard fires here; pidFdGuard closes any fd on unwind. - } - - SSandboxedChild child; - child.s_State = EChildLifecycleState::E_IdentityCaptured; - child.s_Sandbox = sandbox; - child.s_PidFd = pidFdGuard.get(); - child.s_PidFdOutcome = pidFdOutcome; - child.s_Outcome = std::make_shared(); - + bool removeIpcAfterKillAndReap{false}; + bool monitorHandoffSucceeded{false}; std::uint64_t generation{0}; - try { - generation = m_RegistryInsertFn - ? m_RegistryInsertFn(*m_PidRegistry, sandboxPid, child) - : defaultRegistryInsert(*m_PidRegistry, sandboxPid, child); - } catch (const std::exception& e) { - LOG_ERROR(<< "Failed to register sandboxed process " << processPath - << " (PID " << sandboxPid << "): " << e.what()); - childPid = 0; - return false; // killAndReapGuard fires here; pidFdGuard still owns the fd. - } - // E_Registered. The registry entry now owns the pidfd; do not double- - // close it via pidFdGuard's destructor on this path. - pidFdGuard.release(); - - // Erase the registry entry this call just inserted, matching by - // generation (in case a racing call already replaced it). Shared by - // every failure path between a successful registry insertion and a - // successful monitor handoff, since no monitor thread exists on any of - // those paths to ever perform that erase itself. - const auto eraseRegistryEntry = [this, sandboxPid, generation]() { - std::lock_guard lock(m_PidRegistry->s_Mutex); - const auto it = m_PidRegistry->s_Children.find(sandboxPid); - if (it != m_PidRegistry->s_Children.end() && it->second.s_Generation == generation) { - closePidFdIfOpen(it->second.s_PidFd); - m_PidRegistry->s_Children.erase(it); - } - }; + { + CKillAndReapGuard killAndReapGuard{sandbox, m_AwaitResultFn}; + + const SPidFdAcquisitionResult pidFdResult{ + m_PidFdOpenFn ? m_PidFdOpenFn(sandboxPid) : defaultPidFdOpen(sandboxPid)}; + CScopedPidFd pidFdGuard{pidFdResult.s_Fd}; + const EPidFdOutcome pidFdOutcome{classifyPidFdOutcome(pidFdResult)}; + + // An errno other than ENOSYS (ESRCH, EMFILE, ENFILE, ...) is a + // resource/identity error, not "no kernel support" for pidfd - it must + // never be treated the same as E_KernelUnsupported. Fail registration + // outright rather than register a child whose termination would need an + // undefined fallback. pidFdGuard closes any fd this path somehow still + // holds; killAndReapGuard (still armed) Kill()s/awaits the sandboxee. + if (pidFdOutcome == EPidFdOutcome::E_Failed) { + LOG_ERROR(<< "pidfd_open failed for sandboxed process " + << processPath << " (PID " << sandboxPid << ") with errno " + << pidFdResult.s_Errno << " (" << ::strerror(pidFdResult.s_Errno) + << "); refusing to register a child with an undefined termination fallback"); + removeIpcAfterKillAndReap = true; + } else { + SSandboxedChild child; + child.s_State = EChildLifecycleState::E_IdentityCaptured; + child.s_Sandbox = sandbox; + child.s_PidFd = pidFdGuard.get(); + child.s_PidFdOutcome = pidFdOutcome; + child.s_Outcome = std::make_shared(); - // The sandboxee is a child of the Sandbox2 forkserver rather than of the - // controller, so waitpid() never sees it. Own the sandbox instance on a - // dedicated monitor thread that keeps it alive for the lifetime of - // pytorch_inference, waits for its result (via the injectable - // AwaitResult seam), and removes the registry entry before logging - // termination. The thread co-owns the registry and the Sandbox2 - // shared_ptr rather than capturing this: it can still be waiting on a - // live sandboxee when the spawner is destroyed, and a raw pointer - // back to the spawner would be dangling by then. - // - // Everything from copying m_PidRegistry/m_AwaitResultFn through - // launching the monitor thread runs inside a try/catch: those copies - // and constructing monitorBody's capture list can themselves throw - // (e.g. std::bad_alloc copying a std::function), and left unguarded - // that exception would otherwise escape spawn() uncaught, leaking the - // just-inserted registry entry. Catching here ensures every throw in - // this span still erases the registry entry and returns false with - // childPid == 0; killAndReapGuard's destructor performs the - // Kill()/await half of cleanup on unwind either way. - bool monitorStarted{false}; - try { - const TPidRegistryPtr registry{m_PidRegistry}; - const TAwaitResultFn awaitResultFn{m_AwaitResultFn}; - auto monitorBody = [sandboxPid, registry, sandbox, generation, awaitResultFn]() { - // Detached threads must not let exceptions escape: std::terminate(). try { - const sandbox2::Result result{awaitResultFn ? awaitResultFn(*sandbox) - : sandbox->AwaitResult()}; - if (completeMonitorRegistryCleanup(registry, sandboxPid, generation)) { - logSandboxeeTermination(sandboxPid, result); - } + generation = m_RegistryInsertFn + ? m_RegistryInsertFn(*m_PidRegistry, sandboxPid, child) + : defaultRegistryInsert(*m_PidRegistry, sandboxPid, child); } catch (const std::exception& e) { - LOG_ERROR(<< "Monitor thread for sandboxed pytorch_inference PID " - << sandboxPid << " failed: " << e.what()); - try { - sandbox->Kill(); - sandbox->AwaitResult(); - } catch (...) {} - eraseRegistryEntryOnMonitorFailure(registry, sandboxPid, generation); - } catch (...) { - LOG_ERROR(<< "Monitor thread for sandboxed pytorch_inference PID " - << sandboxPid << " failed with a non-standard exception"); - try { - sandbox->Kill(); - sandbox->AwaitResult(); - } catch (...) {} - eraseRegistryEntryOnMonitorFailure(registry, sandboxPid, generation); + LOG_ERROR(<< "Failed to register sandboxed process " << processPath + << " (PID " << sandboxPid << "): " << e.what()); + removeIpcAfterKillAndReap = true; } - }; - monitorStarted = m_MonitorLaunchFn - ? m_MonitorLaunchFn(std::move(monitorBody)) - : defaultMonitorLaunch(std::move(monitorBody)); - } catch (const std::exception& e) { - eraseRegistryEntry(); - LOG_ERROR(<< "Failed to launch monitor thread for sandboxed process " - << processPath << " (PID " << sandboxPid << "): " << e.what()); + if (removeIpcAfterKillAndReap == false) { + // E_Registered. The registry entry now owns the pidfd; do not double- + // close it via pidFdGuard's destructor on this path. + pidFdGuard.release(); + + // Erase the registry entry this call just inserted, matching by + // generation (in case a racing call already replaced it). Shared by + // every failure path between a successful registry insertion and a + // successful monitor handoff, since no monitor thread exists on any of + // those paths to ever perform that erase itself. + const auto eraseRegistryEntry = [this, sandboxPid, generation]() { + std::lock_guard lock(m_PidRegistry->s_Mutex); + const auto it = m_PidRegistry->s_Children.find(sandboxPid); + if (it != m_PidRegistry->s_Children.end() && + it->second.s_Generation == generation) { + closePidFdIfOpen(it->second.s_PidFd); + m_PidRegistry->s_Children.erase(it); + } + }; + + // The sandboxee is a child of the Sandbox2 forkserver rather than of the + // controller, so waitpid() never sees it. Own the sandbox instance on a + // dedicated monitor thread that keeps it alive for the lifetime of + // pytorch_inference, waits for its result (via the injectable + // AwaitResult seam), and removes the registry entry before logging + // termination. The thread co-owns the registry and the Sandbox2 + // shared_ptr rather than capturing this: it can still be waiting on a + // live sandboxee when the spawner is destroyed, and a raw pointer + // back to the spawner would be dangling by then. + // + // Everything from copying m_PidRegistry/m_AwaitResultFn through + // launching the monitor thread runs inside a try/catch: those copies + // and constructing monitorBody's capture list can themselves throw + // (e.g. std::bad_alloc copying a std::function), and left unguarded + // that exception would otherwise escape spawn() uncaught, leaking the + // just-inserted registry entry. Catching here ensures every throw in + // this span still erases the registry entry and returns false with + // childPid == 0; killAndReapGuard's destructor performs the + // Kill()/await half of cleanup on unwind either way. + bool monitorStarted{false}; + try { + const TPidRegistryPtr registry{m_PidRegistry}; + const TAwaitResultFn awaitResultFn{m_AwaitResultFn}; + const std::shared_ptr childIpcReaper{m_ChildIpcReaper}; + auto monitorBody = [sandboxPid, registry, sandbox, generation, + awaitResultFn, childIpcReaper, childIpcRoot]() { + // Detached threads must not let exceptions escape: std::terminate(). + if (childIpcReaper && childIpcRoot.empty() == false) { + childIpcReaper->noteSpawn(sandboxPid, childIpcRoot); + } + const auto notifyChildExited = [&]() { + if (childIpcReaper) { + childIpcReaper->onChildExited(sandboxPid); + } + }; + try { + const sandbox2::Result result{ + awaitResultFn ? awaitResultFn(*sandbox) + : sandbox->AwaitResult()}; + if (completeMonitorRegistryCleanup(registry, sandboxPid, generation)) { + logSandboxeeTermination(sandboxPid, result); + notifyChildExited(); + } + } catch (const std::exception& e) { + LOG_ERROR(<< "Monitor thread for sandboxed pytorch_inference PID " + << sandboxPid << " failed: " << e.what()); + try { + sandbox->Kill(); + sandbox->AwaitResult(); + } catch (...) {} + eraseRegistryEntryOnMonitorFailure(registry, sandboxPid, generation); + notifyChildExited(); + } catch (...) { + LOG_ERROR(<< "Monitor thread for sandboxed pytorch_inference PID " + << sandboxPid << " failed with a non-standard exception"); + try { + sandbox->Kill(); + sandbox->AwaitResult(); + } catch (...) {} + eraseRegistryEntryOnMonitorFailure(registry, sandboxPid, generation); + notifyChildExited(); + } + }; + + monitorStarted = m_MonitorLaunchFn + ? m_MonitorLaunchFn(std::move(monitorBody)) + : defaultMonitorLaunch(std::move(monitorBody)); + } catch (const std::exception& e) { + eraseRegistryEntry(); + LOG_ERROR(<< "Failed to launch monitor thread for sandboxed process " << processPath + << " (PID " << sandboxPid << "): " << e.what()); + removeIpcAfterKillAndReap = true; + } + + if (removeIpcAfterKillAndReap == false && monitorStarted == false) { + // Monitor handoff failed: no thread is running to ever erase + // this registry entry or call AwaitResult(), so this frame owns + // both. killAndReapGuard's destructor Kill()s/awaits the sandboxee. + eraseRegistryEntry(); + LOG_ERROR(<< "Failed to start monitor thread for sandboxed process " + << processPath << " (PID " << sandboxPid << ")"); + removeIpcAfterKillAndReap = true; + } + + if (removeIpcAfterKillAndReap == false) { + // E_Monitoring: registry insertion and monitor handoff both + // succeeded, so the monitor thread now owns calling AwaitResult() and + // removing the registry entry. Disarm - the guard must not also reap. + killAndReapGuard.disarm(); + monitorHandoffSucceeded = true; + } + } + } + } + if (removeIpcAfterKillAndReap) { + notifySpawnFailed(); childPid = 0; - return false; // killAndReapGuard fires here. + return false; } - - if (monitorStarted == false) { - // Monitor handoff failed: no thread is running to ever erase - // this registry entry or call AwaitResult(), so this frame owns - // both. killAndReapGuard's destructor Kill()s/awaits the sandboxee. - eraseRegistryEntry(); - LOG_ERROR(<< "Failed to start monitor thread for sandboxed process " - << processPath << " (PID " << sandboxPid << ")"); + if (monitorHandoffSucceeded == false) { childPid = 0; - return false; // killAndReapGuard fires here. + return false; } - // E_Monitoring: registry insertion and monitor handoff both - // succeeded, so the monitor thread now owns calling AwaitResult() and - // removing the registry entry. Disarm - the guard must not also reap. - killAndReapGuard.disarm(); - // Record E_Monitoring under the lock, generation-matched. Only advance // from E_Registered so a racing terminator is not overwritten. { diff --git a/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc b/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc index 7c90b3a96e..e73f160776 100644 --- a/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc +++ b/lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc @@ -15,6 +15,7 @@ // not just Linux - but not Windows, which has none of those APIs; see // lib/sandbox/unittest/CMakeLists.txt's NOT WIN32 guard. +#include #include #include @@ -389,6 +390,84 @@ BOOST_AUTO_TEST_CASE(testEnsureChildIpcDirectoryRejectsRegularFileInTheWay) { ::unlink(childRoot.c_str()); } +#ifndef _WIN32 +BOOST_AUTO_TEST_CASE(testChildIpcDirectoryReaperRemovesEmptyPerChildDirectory) { + CTrustedBaseOnlyFixture fixture; + const std::string childRoot{fixture.canonicalTrustedBase() + "/ml-child-ipc/reap-empty"}; + const std::vector args{"--input=" + childRoot + "/input.fifo"}; + BOOST_REQUIRE(ml::sandbox::ensureChildIpcDirectory(fixture.canonicalTrustedBase(), args) == + ml::sandbox::EChildIpcDirectoryOutcome::E_Ready); + + ml::sandbox::CChildIpcDirectoryReaper reaper; + reaper.noteSpawn(1001, childRoot); + reaper.onChildExited(1001); + + struct stat childRootStat; + BOOST_REQUIRE_EQUAL(-1, ::stat(childRoot.c_str(), &childRootStat)); + BOOST_REQUIRE_EQUAL(ENOENT, errno); + + struct stat parentStat; + BOOST_REQUIRE_EQUAL( + 0, ::stat((fixture.canonicalTrustedBase() + "/ml-child-ipc").c_str(), &parentStat)); +} + +BOOST_AUTO_TEST_CASE(testChildIpcDirectoryReaperKeepsDirectoryWithUnexpectedFile) { + CTrustedBaseOnlyFixture fixture; + const std::string childRoot{fixture.canonicalTrustedBase() + "/ml-child-ipc/reap-nonempty"}; + const std::vector args{"--input=" + childRoot + "/input.fifo"}; + BOOST_REQUIRE(ml::sandbox::ensureChildIpcDirectory(fixture.canonicalTrustedBase(), args) == + ml::sandbox::EChildIpcDirectoryOutcome::E_Ready); + FILE* file{::fopen((childRoot + "/leftover.txt").c_str(), "w")}; + BOOST_TEST_REQUIRE(file != nullptr); + ::fclose(file); + + ml::sandbox::CChildIpcDirectoryReaper reaper; + reaper.noteSpawn(1002, childRoot); + reaper.onChildExited(1002); + + struct stat childRootStat; + BOOST_REQUIRE_EQUAL(0, ::stat(childRoot.c_str(), &childRootStat)); + ::unlink((childRoot + "/leftover.txt").c_str()); +} + +BOOST_AUTO_TEST_CASE(testChildIpcDirectoryReaperPidGenerationGuard) { + CTrustedBaseOnlyFixture fixture; + const std::string childRoot{fixture.canonicalTrustedBase() + "/ml-child-ipc/reap-gen"}; + const std::vector args{"--input=" + childRoot + "/input.fifo"}; + BOOST_REQUIRE(ml::sandbox::ensureChildIpcDirectory(fixture.canonicalTrustedBase(), args) == + ml::sandbox::EChildIpcDirectoryOutcome::E_Ready); + + ml::sandbox::CChildIpcDirectoryReaper reaper; + reaper.noteSpawn(2001, childRoot); + reaper.noteSpawn(2002, childRoot); + reaper.onChildExited(2001); + + struct stat childRootStat; + BOOST_REQUIRE_EQUAL(0, ::stat(childRoot.c_str(), &childRootStat)); + + reaper.onChildExited(2002); + BOOST_REQUIRE_EQUAL(-1, ::stat(childRoot.c_str(), &childRootStat)); + BOOST_REQUIRE_EQUAL(ENOENT, errno); +} + +BOOST_AUTO_TEST_CASE(testChildIpcDirectoryReaperOnSpawnFailedSkipsLivePid) { + CTrustedBaseOnlyFixture fixture; + const std::string childRoot{fixture.canonicalTrustedBase() + "/ml-child-ipc/reap-live"}; + const std::vector args{"--input=" + childRoot + "/input.fifo"}; + BOOST_REQUIRE(ml::sandbox::ensureChildIpcDirectory(fixture.canonicalTrustedBase(), args) == + ml::sandbox::EChildIpcDirectoryOutcome::E_Ready); + + ml::sandbox::CChildIpcDirectoryReaper reaper; + reaper.noteSpawn(3001, childRoot); + reaper.onSpawnFailed(childRoot); + + struct stat childRootStat; + BOOST_REQUIRE_EQUAL(0, ::stat(childRoot.c_str(), &childRootStat)); + reaper.onChildExited(3001); + BOOST_REQUIRE_EQUAL(-1, ::stat(childRoot.c_str(), &childRootStat)); +} +#endif // !_WIN32 + BOOST_AUTO_TEST_CASE(testEnsureChildIpcDirectoryRejectsLoosePermissionsOnExistingDirectory) { CTrustedBaseOnlyFixture fixture; const std::string mlChildIpc{fixture.canonicalTrustedBase() + "/ml-child-ipc"}; diff --git a/lib/seccomp/unittest/CSeccompFilterBuilderTest.cc b/lib/seccomp/unittest/CSeccompFilterBuilderTest.cc index a53e63a628..4d21c05e06 100644 --- a/lib/seccomp/unittest/CSeccompFilterBuilderTest.cc +++ b/lib/seccomp/unittest/CSeccompFilterBuilderTest.cc @@ -35,6 +35,8 @@ #endif // __linux__ +static_assert(ml::seccomp::TERMINATE_ON_DEGRADED_SECCOMP_FAILURE); + BOOST_AUTO_TEST_SUITE(CSeccompFilterBuilderTest) #ifdef __linux__ @@ -284,10 +286,9 @@ BOOST_AUTO_TEST_CASE(testDecideDegradedModeActionFaultInjection) { ESystemCallFilterInstallOutcome::E_Installed, true))); // Every fault-injected failure class - capability probe failure, - // PR_SET_NO_NEW_PRIVS, and filter installation - with the internal - // switch off (today's production default), every call site continues; - // with it on (the behaviour a later change activates), every one - // terminates. + // PR_SET_NO_NEW_PRIVS, and filter installation - with terminateOnFailure + // false every call site continues; with it true (production default via + // TERMINATE_ON_DEGRADED_SECCOMP_FAILURE), every one terminates. const ESystemCallFilterInstallOutcome failureModes[]{ ESystemCallFilterInstallOutcome::E_MechanismUnavailable, ESystemCallFilterInstallOutcome::E_PrivilegeRestrictionFailed, @@ -325,7 +326,7 @@ BOOST_AUTO_TEST_CASE(testInProcessFilterSkippedEntirelyForSandbox2LaunchedChild) // termination may be derived and no attestation marker may be produced - // and that must hold for every outcome an installation attempt could // have returned, including the failure classes that would otherwise - // terminate the launch once TERMINATE_ON_DEGRADED_SECCOMP_FAILURE is activated. + // terminate the launch when TERMINATE_ON_DEGRADED_SECCOMP_FAILURE is true. const ESystemCallFilterInstallOutcome allOutcomes[]{ ESystemCallFilterInstallOutcome::E_Installed, ESystemCallFilterInstallOutcome::E_MechanismUnavailable,