fix(audit-2026-06-11/H-C1): engine.on_progress deadline check terminates runaway scripts
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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<Option<Instant>> = 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<Instant>,
|
||||
}
|
||||
|
||||
impl DeadlineGuard {
|
||||
/// Set the current-thread deadline. Returns a guard whose `Drop`
|
||||
/// restores the prior value.
|
||||
#[must_use]
|
||||
pub fn set(deadline: Option<Instant>) -> 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<Instant> {
|
||||
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<Instant>,
|
||||
) -> Result<ExecResponse, ExecError> {
|
||||
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<AST>,
|
||||
req: ExecRequest,
|
||||
deadline: Option<Instant>,
|
||||
) -> Result<ExecResponse, ExecError> {
|
||||
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<ExecResponse, ExecError> {
|
||||
let effective_limits = self.limits.with_overrides(&req.sandbox_overrides);
|
||||
@@ -311,6 +381,22 @@ fn build_engine(limits: Limits, logs: Option<Arc<Mutex<Vec<LogEntry>>>>) -> 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);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user