Skip to content

Commit aea0090

Browse files
committed
fix(procedural): harden executor against overflow, runaway loops, and cache collisions
Replace wrapping integer arithmetic with checked_add/sub/mul/div/rem in the expression evaluator so overflows return None rather than panicking or silently wrapping. Replace the unlimited trigger budget (1-hour wall clock, MAX iterations) with a trigger_default (100k iterations, 10s) to prevent runaway procedural bodies from pinning Control Plane workers indefinitely. Guard the plan cache against 64-bit hash collisions by verifying the cached body SQL matches before returning a cached block, and evicting on mismatch.
1 parent 13a7cde commit aea0090

4 files changed

Lines changed: 31 additions & 19 deletions

File tree

nodedb/src/control/planner/procedural/executor/core/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ impl<'a> StatementExecutor<'a> {
100100
block: &ProceduralBlock,
101101
bindings: &RowBindings,
102102
) -> crate::Result<()> {
103-
let mut budget = ExecutionBudget::unlimited();
103+
let mut budget = ExecutionBudget::trigger_default();
104104
self.execute_block_with_exceptions(
105105
&block.statements,
106106
&block.exception_handlers,

nodedb/src/control/planner/procedural/executor/eval.rs

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -174,11 +174,11 @@ fn eval_binary_op(
174174

175175
match (l, r) {
176176
(Value::Integer(a), Value::Integer(b)) => match op {
177-
Plus => Some(Value::Integer(a + b)),
178-
Minus => Some(Value::Integer(a - b)),
179-
Multiply => Some(Value::Integer(a * b)),
180-
Divide if *b != 0 => Some(Value::Integer(a / b)),
181-
Modulo if *b != 0 => Some(Value::Integer(a % b)),
177+
Plus => Some(Value::Integer(a.checked_add(*b)?)),
178+
Minus => Some(Value::Integer(a.checked_sub(*b)?)),
179+
Multiply => Some(Value::Integer(a.checked_mul(*b)?)),
180+
Divide if *b != 0 => Some(Value::Integer(a.checked_div(*b)?)),
181+
Modulo if *b != 0 => Some(Value::Integer(a.checked_rem(*b)?)),
182182
Gt => Some(Value::Bool(a > b)),
183183
GtEq => Some(Value::Bool(a >= b)),
184184
Lt => Some(Value::Bool(a < b)),
@@ -191,7 +191,14 @@ fn eval_binary_op(
191191
Plus => Some(Value::Float(a + b)),
192192
Minus => Some(Value::Float(a - b)),
193193
Multiply => Some(Value::Float(a * b)),
194-
Divide if *b != 0.0 => Some(Value::Float(a / b)),
194+
Divide if *b != 0.0 && b.is_finite() && *b != -0.0 => {
195+
let result = a / b;
196+
if result.is_finite() {
197+
Some(Value::Float(result))
198+
} else {
199+
None
200+
}
201+
}
195202
Gt => Some(Value::Bool(a > b)),
196203
GtEq => Some(Value::Bool(a >= b)),
197204
Lt => Some(Value::Bool(a < b)),

nodedb/src/control/planner/procedural/executor/fuel.rs

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,10 @@ impl ExecutionBudget {
3232
}
3333
}
3434

35-
/// Create an unlimited budget (for triggers with no explicit limits).
36-
pub fn unlimited() -> Self {
37-
Self {
38-
fuel_remaining: u64::MAX,
39-
deadline: Instant::now() + std::time::Duration::from_secs(3600),
40-
max_iterations: u64::MAX,
41-
timeout_secs: 3600,
42-
}
35+
/// Default budget for trigger bodies. Caps iterations and wall-clock
36+
/// time to prevent runaway loops from pinning Control Plane workers.
37+
pub fn trigger_default() -> Self {
38+
Self::new(100_000, 10)
4339
}
4440

4541
/// Consume one iteration of fuel. Returns error if exhausted.
@@ -103,8 +99,8 @@ mod tests {
10399
}
104100

105101
#[test]
106-
fn unlimited() {
107-
let mut budget = ExecutionBudget::unlimited();
102+
fn trigger_default_allows_bounded_loops() {
103+
let mut budget = ExecutionBudget::trigger_default();
108104
for _ in 0..10_000 {
109105
budget.consume_iteration().unwrap();
110106
}

nodedb/src/control/planner/procedural/executor/plan_cache.rs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ struct CacheInner {
2727
}
2828

2929
struct CacheEntry {
30+
body_sql: String,
3031
block: Arc<ProceduralBlock>,
3132
}
3233

@@ -58,9 +59,16 @@ impl ProcedureBlockCache {
5859
// is uncontended in the cache-hit path (early return).
5960
let mut inner = self.inner.lock().unwrap_or_else(|p| p.into_inner());
6061

61-
// Cache hit — return immediately.
62+
// Cache hit — verify the body matches (not just the hash) to guard
63+
// against 64-bit hash collisions returning the wrong block.
6264
if let Some(entry) = inner.entries.get(&key) {
63-
return Ok(Arc::clone(&entry.block));
65+
if entry.body_sql == body_sql {
66+
return Ok(Arc::clone(&entry.block));
67+
}
68+
// Hash collision with different body — evict the stale entry
69+
// and fall through to re-parse.
70+
inner.entries.remove(&key);
71+
inner.order.retain(|k| *k != key);
6472
}
6573

6674
// Cache miss — parse, cache, return.
@@ -76,6 +84,7 @@ impl ProcedureBlockCache {
7684
inner.entries.insert(
7785
key,
7886
CacheEntry {
87+
body_sql: body_sql.to_string(),
7988
block: Arc::clone(&arc),
8089
},
8190
);

0 commit comments

Comments
 (0)