From 99eb0025fa03946a0e7c03b91e5b3833830bb017 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Thu, 11 Jun 2026 20:33:01 +0200 Subject: [PATCH] fix(audit-2026-06-11/H-C1): engine.on_progress deadline check terminates runaway scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tokio::time::timeout over JoinHandle only drops the future — the spawn_blocking OS thread keeps running until the Rhai script self- completes or hits the per-op budget. A `loop {}` body with a generous max_operations could pin a blocking worker for tens of seconds; on Pi- class hardware one anonymous-callable script call could permanently subtract a worker from the pool. Installs an `engine.on_progress` hook that consults a thread-local deadline; when `Instant::now() >= deadline` the hook returns `Some(_)`, triggering `ErrorTerminated` inside the Rhai loop. The deadline is set by `Engine::execute_with_deadline` / `Engine::execute_ast_with_deadline` via an RAII `DeadlineGuard`, called from the orchestrator client; the old `execute` / `execute_ast` paths are unchanged so tests, validation, and the parse-only path keep working. Invoke re-entry (`sdk/invoke.rs`) inherits the parent's deadline transparently — the thread-local is set for the entire spawn_blocking lifetime, and Rhai is single-threaded so the same thread services the parent + every sub-script. New tests: * `deadline_terminates_a_runaway_loop` — `loop {}` body with `max_operations = u64::MAX` and a 100 ms deadline aborts within 2 s (typically <200 ms). Maps to `ExecError::Runtime`. * `no_deadline_set_does_not_abort` — `None` deadline keeps the pre- audit behavior; the op-budget still bites. Audit ref: security_audit/05_sandbox_exec.md#f-se-h-01. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/executor-core/src/engine.rs | 92 +++++++++++++++++++++++++- crates/executor-core/tests/engine.rs | 49 ++++++++++++++ crates/orchestrator-core/src/client.rs | 25 ++++--- 3 files changed, 153 insertions(+), 13 deletions(-) diff --git a/crates/executor-core/src/engine.rs b/crates/executor-core/src/engine.rs index 011dd92..f8fec14 100644 --- a/crates/executor-core/src/engine.rs +++ b/crates/executor-core/src/engine.rs @@ -1,7 +1,49 @@ +use std::cell::Cell; use std::collections::{BTreeMap, HashMap}; use std::sync::{Arc, Mutex, OnceLock, Weak}; use std::time::Instant; +// Audit 2026-06-11 H-C1 — thread-local deadline consulted by the Rhai +// `on_progress` hook so a runaway script (e.g., `loop {}`) actually +// terminates instead of holding its `spawn_blocking` OS thread until +// the per-op budget self-exhausts (which can be tens of seconds). The +// orchestrator client wraps each `execute*` call in a +// `DeadlineGuard::set(Some(now + timeout))` so invoke re-entries on the +// same thread inherit the same deadline. +thread_local! { + static CURRENT_DEADLINE: Cell> = const { Cell::new(None) }; +} + +/// RAII guard that sets the current-thread deadline for the lifetime of +/// the guard, restoring whatever was there before on drop. +/// +/// Use via [`Engine::execute_with_deadline`] / [`Engine::execute_ast_with_deadline`] +/// at the orchestrator boundary; SDK invoke re-entries piggyback on the +/// existing thread-local without touching it. +pub struct DeadlineGuard { + prev: Option, +} + +impl DeadlineGuard { + /// Set the current-thread deadline. Returns a guard whose `Drop` + /// restores the prior value. + #[must_use] + pub fn set(deadline: Option) -> Self { + let prev = CURRENT_DEADLINE.with(|c| c.replace(deadline)); + Self { prev } + } +} + +impl Drop for DeadlineGuard { + fn drop(&mut self) { + CURRENT_DEADLINE.with(|c| c.set(self.prev)); + } +} + +fn current_deadline() -> Option { + CURRENT_DEADLINE.with(Cell::get) +} + use chrono::{DateTime, Utc}; use picloud_shared::{ ScriptId, ScriptValidator, SdkCallCx, Services, TriggerEvent, ValidatedScript, ValidationError, @@ -187,10 +229,38 @@ impl Engine { .map_err(|e| ExecError::Parse(e.to_string())) } + /// Like [`Self::execute`] but also installs `deadline` on the + /// current thread so the Rhai `on_progress` hook can interrupt a + /// runaway script before `tokio::time::timeout` fires (which only + /// drops the future — the OS thread keeps running). Audit + /// 2026-06-11 H-C1. + pub fn execute_with_deadline( + &self, + source: &str, + req: ExecRequest, + deadline: Option, + ) -> Result { + let _guard = DeadlineGuard::set(deadline); + self.execute(source, req) + } + + /// Like [`Self::execute_ast`] but installs `deadline` on the current + /// thread. See [`Self::execute_with_deadline`] for the rationale. + pub fn execute_ast_with_deadline( + &self, + ast: &Arc, + req: ExecRequest, + deadline: Option, + ) -> Result { + let _guard = DeadlineGuard::set(deadline); + self.execute_ast(ast, req) + } + /// Execute `source` against `req`. Op-budget protection comes from - /// Rhai's `set_max_operations`; wall-clock enforcement is the - /// caller's responsibility. Per-script sandbox overrides on the - /// request replace the engine's defaults field-by-field; the + /// Rhai's `set_max_operations`; the wall-clock deadline (when set + /// via [`DeadlineGuard`] / [`Self::execute_with_deadline`]) aborts + /// any per-op step that crosses it. Per-script sandbox overrides on + /// the request replace the engine's defaults field-by-field; the /// manager already clamped them against the admin ceiling. pub fn execute(&self, source: &str, req: ExecRequest) -> Result { let effective_limits = self.limits.with_overrides(&req.sandbox_overrides); @@ -311,6 +381,22 @@ fn build_engine(limits: Limits, logs: Option>>>) -> Rhai engine.set_max_call_levels(limits.max_call_levels); engine.set_max_expr_depths(limits.max_expr_depth, limits.max_expr_depth); + // Audit 2026-06-11 H-C1 — wall-clock interrupter. Rhai invokes the + // progress callback once per operation; returning `Some(_)` triggers + // `ErrorTerminated`, which propagates out of `eval_ast_with_scope`. + // The deadline is read from a thread-local set by the orchestrator's + // `execute_with_deadline` / `execute_ast_with_deadline` entry points; + // `None` (the default) is a no-op so tests, validation, and bare + // `execute*` callers see the previous behavior. + engine.on_progress(|_ops| { + if let Some(deadline) = current_deadline() { + if Instant::now() >= deadline { + return Some(Dynamic::UNIT); + } + } + None + }); + // Reject `import` — scripts cannot pull external modules. engine.set_module_resolver(rhai::module_resolvers::DummyModuleResolver); diff --git a/crates/executor-core/tests/engine.rs b/crates/executor-core/tests/engine.rs index daff713..db05faf 100644 --- a/crates/executor-core/tests/engine.rs +++ b/crates/executor-core/tests/engine.rs @@ -173,6 +173,55 @@ fn override_only_replaces_specified_field() { assert_eq!(resp.body, json!("hello")); } +#[test] +fn deadline_terminates_a_runaway_loop() { + // Audit 2026-06-11 H-C1 closure. With a generous op budget the + // previous engine ran a `loop {}` body until `max_operations` + // self-exhausted (potentially seconds on Pi-class hardware) and the + // outer `tokio::time::timeout` only dropped the awaiting future — + // not the OS thread. With `on_progress` consulting the deadline, + // a 100 ms deadline aborts within a few hundred ms even when the + // op budget would allow far more work. + let limits = Limits { + max_operations: u64::MAX, + ..Limits::default() + }; + let engine = Engine::new(limits, Services::default()); + let src = r"let n = 0; loop { n += 1; }"; + let deadline = + Some(std::time::Instant::now() + std::time::Duration::from_millis(100)); + let started = std::time::Instant::now(); + let err = engine + .execute_with_deadline(src, req(json!(null)), deadline) + .expect_err("runaway loop must terminate"); + let elapsed = started.elapsed(); + assert!( + elapsed < std::time::Duration::from_secs(2), + "deadline should fire within ~hundreds of ms, took {elapsed:?}" + ); + // ErrorTerminated maps to ExecError::Runtime via map_eval_error. + assert!( + matches!(err, ExecError::Runtime(_)), + "expected Runtime, got {err:?}" + ); +} + +#[test] +fn no_deadline_set_does_not_abort() { + // Smoke: a deadline-less execute is unchanged from the pre-audit + // behavior — the per-op budget is what stops things. + let limits = Limits { + max_operations: 100, + ..Limits::default() + }; + let engine = Engine::new(limits, Services::default()); + let src = r"let n = 0; for i in 0..10000 { n += 1; } n"; + let err = engine + .execute_with_deadline(src, req(json!(null)), None) + .expect_err("budget should still bite"); + assert!(matches!(err, ExecError::OperationBudgetExceeded)); +} + #[test] fn runtime_error_is_mapped_to_runtime_variant() { let err = engine() diff --git a/crates/orchestrator-core/src/client.rs b/crates/orchestrator-core/src/client.rs index b6f530a..97744ba 100644 --- a/crates/orchestrator-core/src/client.rs +++ b/crates/orchestrator-core/src/client.rs @@ -1,6 +1,6 @@ use std::num::NonZeroUsize; use std::sync::{Arc, Mutex}; -use std::time::Duration; +use std::time::{Duration, Instant}; use async_trait::async_trait; use chrono::{DateTime, Utc}; @@ -183,13 +183,12 @@ impl ExecutorClient for LocalExecutorClient { ) -> Result { // Acquire before spending any wall-clock budget. The permit is // held by this future; on `tokio::time::timeout` firing, the - // future drops and the permit returns to the pool — but the - // detached `spawn_blocking` thread keeps running until the - // Rhai script finishes (or panics). So in-use blocking threads - // can briefly exceed the gate's permit count after a timeout. - // That is intentional: a new admission can be served while the - // already-doomed script winds down, which is preferable to - // wedging the slot for the worst-case timeout duration. + // future drops and the permit returns to the pool. Audit + // 2026-06-11 H-C1: the detached `spawn_blocking` thread is now + // also bounded by the Rhai `on_progress` deadline hook (installed + // via `execute_with_deadline`), so a runaway script terminates + // soon after the deadline instead of holding its OS thread until + // the per-op budget self-exhausts. let _permit = self.gate .try_acquire() @@ -201,10 +200,13 @@ impl ExecutorClient for LocalExecutorClient { let timeout = timeout.min(HARD_TIMEOUT_CAP); let timeout_secs = u32::try_from(timeout.as_secs()).unwrap_or(u32::MAX); + let deadline = Instant::now() + timeout; let engine = self.engine.clone(); let source = source.to_string(); - let join = tokio::task::spawn_blocking(move || engine.execute(&source, req)); + let join = tokio::task::spawn_blocking(move || { + engine.execute_with_deadline(&source, req, Some(deadline)) + }); match tokio::time::timeout(timeout, join).await { Err(_) => Err(ExecError::Timeout(timeout_secs)), @@ -235,9 +237,12 @@ impl ExecutorClient for LocalExecutorClient { let timeout = timeout.min(HARD_TIMEOUT_CAP); let timeout_secs = u32::try_from(timeout.as_secs()).unwrap_or(u32::MAX); + let deadline = Instant::now() + timeout; let engine = self.engine.clone(); - let join = tokio::task::spawn_blocking(move || engine.execute_ast(&ast, req)); + let join = tokio::task::spawn_blocking(move || { + engine.execute_ast_with_deadline(&ast, req, Some(deadline)) + }); match tokio::time::timeout(timeout, join).await { Err(_) => Err(ExecError::Timeout(timeout_secs)),