Never crash: scripts that nest without brackets, or through eval, return an error

crates/expr/src/rewrite.rs

@@ -557,7 +557,7 @@ pub const MAX_NESTING: usize = 32;
 
 /// Stack cost of one level of brackets (also of a template substitution, a regex group and a
 /// prefix operator, which cost the parser about as much or are never chained in real code).
-const LEVEL: usize = 256;
+pub const LEVEL: usize = 256;
 /// Stack cost of a construct the parser recurses on without a bracket: `?:`, `=>`, assignments,
 /// `**`, `||`, labels, `if`/`else`/`while`/`for`/`with`/`do` without a block (about 1/8 of a
 /// bracket: 256 of them in a row are allowed; boa overflows from about 750).
@@ -567,11 +567,38 @@ const HEAVY: usize = 32;
 /// about 3 800 links).
 const LIGHT: usize = 6;
 
+/// What [`too_nested`]'s estimate charges, in units where one level of brackets costs
+/// [`LEVEL`]. Constructs the parser recurses on without a bracket (`?:`, `=>`, assignments,
+/// `**`, `||`, labels, `if`/`else`/`while`/`for`/`with`/`do` without a block) always cost
+/// [`HEAVY`], and a prefix operator a whole level.
+#[derive(Clone, Copy, Debug)]
+pub struct Weights {
+    /// The budget, in levels of brackets.
+    pub levels: usize,
+    /// A binary operator in a chain (`a + b + …`).
+    pub operator: usize,
+    /// A member access, call, index or tagged template in a chain (`a.b`, `f()`, `a[0]`).
+    pub access: usize,
+    /// A comma operator (`(a, b, …)`).
+    pub comma: usize,
+    /// Tell object literals, argument lists and parameter lists from blocks and comma
+    /// operators, and array literals from indexing: the parser reads a list's items in a loop,
+    /// so its commas cost nothing. Without it only `[` holds a list.
+    pub lists: bool,
+}
+
+/// The expressions' weights.
+const EXPRESSION: Weights = Weights { levels: MAX_NESTING, operator: LIGHT, access: LIGHT, comma: LIGHT, lists: false };
+
 /// One bracket level of [`too_nested`]'s walk.
 #[derive(Default)]
 struct Level {
-    /// `[`: commas separate elements (a loop in the parser), not operands.
-    array: bool,
+    /// Commas separate items (a loop in the parser), not operands.
+    list: bool,
+    /// An object literal (`:` precedes its values).
+    object: bool,
+    /// The header of `if`, `while`, `for`, `switch`, `catch` or `with` (not a call).
+    header: bool,
     /// Cost of the chains of the current statement or element.
     chain: usize,
     /// Comma operators so far in the current statement.
@@ -666,14 +693,53 @@ fn substitutions(template: &str) -> Vec<String> {
 /// use, summing brackets, runs of prefix operators and, per bracket level, the chains of the
 /// current statement (or array element) weighted by what each link costs the parser.
 fn too_nested(toks: &[Tok]) -> bool {
-    too_nested_from(toks, 0)
+    too_nested_from(toks, 0, EXPRESSION)
+}
+
+/// Whether `code` nests deeper than [`too_nested`]'s estimate allows with weights `w`, without
+/// the expression rewrite: for other JavaScript, run on another stack (scripts).
+pub fn nests_deeper_than(code: &str, w: Weights) -> bool {
+    too_nested_from(&tokenize(code).0, 0, w)
+}
+
+/// Whether the bracket `open`, after `prev`, holds a list ([`Weights::lists`]). `parent` is the
+/// level it opens in; `after_header` is set when `prev` closes an `if (…)`-style header.
+fn opens_list(open: &Tok, prev: Option<&Tok>, parent: Option<&Level>, after_header: bool, lists: bool) -> bool {
+    if !lists {
+        return open.text == "[";
+    }
+    let Some(prev) = prev else { return open.text == "[" };
+    // A value: what follows it is a call or an index, not a literal.
+    let value = match prev.kind {
+        Kind::Ident => !prev.is_kw(DELIM_KW) && !prev.is_kw(PREFIX_KW),
+        Kind::Num | Kind::Str | Kind::Template | Kind::Regex => true,
+        Kind::Punct => matches!(prev.text.as_str(), ")" | "]" | "}"),
+    };
+    match open.text.as_str() {
+        "[" => !value,
+        // Arguments (`f(a, b)`, `f(a)(b, c)`) and parameters (`function f(a, b)`); after a block's
+        // `}` or an `if (…)` header, `(` groups a comma operator.
+        "(" => value && !prev.is("}") && !(after_header && prev.is(")")),
+        // An object literal stands where an operand is expected; anywhere else `{` opens a block
+        // (also after `=>`, a label, `case …:` and a `return` that ends its line).
+        _ => match prev.kind {
+            Kind::Punct if prev.is(":") => parent.is_some_and(|l| l.object),
+            Kind::Punct => !matches!(prev.text.as_str(), ")" | "]" | "}" | "=>" | ";" | "++" | "--"),
+            Kind::Ident => {
+                prev.is_kw(&["throw", "typeof", "void", "delete", "in", "instanceof", "of", "yield", "await", "case"]) || (prev.is_kw(&["return"]) && !open.nl)
+            }
+            _ => false,
+        },
+    }
 }
 
-/// [`too_nested`] for code that already sits `base` deep (a template substitution).
-fn too_nested_from(toks: &[Tok], base: usize) -> bool {
+/// [`too_nested`] for code that already sits `base` deep (a template substitution), with
+/// weights `w`.
+fn too_nested_from(toks: &[Tok], base: usize, w: Weights) -> bool {
     let mut levels = vec![Level::default()];
     let mut prefix = 0usize;
     let mut prev: Option<&Tok> = None;
+    let mut after_header = false;
     for t in toks {
         let p = t.kind == Kind::Punct;
         if let Some(l) = levels.last_mut() {
@@ -693,30 +759,36 @@ fn too_nested_from(toks: &[Tok], base: usize) -> bool {
             if t.text != "{"
                 && let Some(l) = levels.last_mut()
             {
-                l.chain = l.chain.saturating_add(LIGHT);
+                l.chain = l.chain.saturating_add(w.access);
             }
-            levels.push(Level { array: t.text == "[", ..Level::default() });
+            let list = opens_list(t, prev, levels.last(), after_header, w.lists);
+            let header = t.text == "(" && prev.is_some_and(|p| p.is_kw(HEADER_KW));
+            levels.push(Level { list, object: list && t.text == "{", header, ..Level::default() });
         } else if p && matches!(t.text.as_str(), ")" | "]" | "}") {
-            if levels.len() > 1 {
-                levels.pop();
+            if levels.len() > 1
+                && let Some(closed) = levels.pop()
+            {
+                after_header = closed.header;
             }
         } else if let Some(l) = levels.last_mut() {
             if t.is(";") {
                 l.boundary();
             } else if t.is(",") {
-                l.commas += usize::from(!l.array);
-                l.chain = l.commas.saturating_mul(LIGHT);
+                l.commas += usize::from(!l.list);
+                l.chain = l.commas.saturating_mul(w.comma);
             } else if t.is_kw(&["else"]) {
                 l.elses += 1;
             } else if (p && (matches!(t.text.as_str(), "?" | ":" | "=>" | "**" | "||") || ASSIGN.contains(&t.text.as_str())))
                 || t.is_kw(&["if", "while", "for", "with", "do"])
             {
                 l.chain = l.chain.saturating_add(HEAVY);
+            } else if t.is(".") || t.is("?.") {
+                l.chain = l.chain.saturating_add(w.access);
             } else if p || t.is_kw(&["in", "instanceof"]) {
-                l.chain = l.chain.saturating_add(LIGHT);
+                l.chain = l.chain.saturating_add(w.operator);
             } else if matches!(t.kind, Kind::Template | Kind::Regex) {
                 // A tagged template is a link like a call.
-                l.chain = l.chain.saturating_add(LIGHT);
+                l.chain = l.chain.saturating_add(w.access);
                 inner = inner_depth(&t.text);
             }
         }
@@ -726,7 +798,7 @@ fn too_nested_from(toks: &[Tok], base: usize) -> bool {
         for l in &levels {
             cost = cost.saturating_add(l.chain).saturating_add(l.elses.saturating_mul(HEAVY));
         }
-        if cost > MAX_NESTING * LEVEL {
+        if cost > w.levels.saturating_mul(LEVEL) {
             return true;
         }
         // The code inside `${…}` is parsed like any other, one level deeper (the depth check
@@ -735,7 +807,7 @@ fn too_nested_from(toks: &[Tok], base: usize) -> bool {
             && t.text.contains("${")
             && substitutions(&t.text)
                 .iter()
-                .any(|sub| too_nested_from(&tokenize(sub).0, cost.saturating_sub(inner.saturating_mul(LEVEL)).saturating_add(LEVEL)))
+                .any(|sub| too_nested_from(&tokenize(sub).0, cost.saturating_sub(inner.saturating_mul(LEVEL)).saturating_add(LEVEL), w))
         {
             return true;
         }

crates/script/Cargo.toml

@@ -16,6 +16,7 @@ effectcraft-keyframe = { workspace = true }
 effectcraft-time = { workspace = true }
 effectcraft-render = { workspace = true }
 effectcraft-path = { workspace = true }
+effectcraft-expr = { workspace = true }
 serde_json = { workspace = true }
 boa_engine = { workspace = true }
 web-time = { workspace = true }
@@ -25,5 +26,4 @@ web-time = { workspace = true }
 boa_engine = { workspace = true, features = ["js"] }
 
 [dev-dependencies]
-effectcraft-expr = { workspace = true }
 effectcraft-psd = { workspace = true }

crates/script/src/runtime.rs

@@ -5,10 +5,13 @@ use std::cell::RefCell;
 use std::mem::ManuallyDrop;
 use std::sync::Arc;
 
+use boa_engine::context::HostHooks;
+use boa_engine::realm::Realm;
 use boa_engine::{Context, JsNativeError, JsResult, JsString, JsValue, NativeFunction, Source, js_string};
 use effectcraft_engine::history::History;
 use effectcraft_engine::project::Project;
 use effectcraft_engine::{ScriptRequest, Session};
+use effectcraft_expr::rewrite::{Weights, nests_deeper_than};
 use serde_json::{Value as J, json};
 
 const PRELUDE: &str = include_str!("prelude.js");
@@ -512,8 +515,23 @@ fn native_eval_file(_: &JsValue, args: &[JsValue], ctx: &mut Context) -> JsResul
     r
 }
 
+/// Host hooks of a script's context: code it compiles at run time (`eval`, `new Function`, …)
+/// passes [`check_nesting`] like the script itself, instead of reaching the parser unchecked.
+struct Hooks;
+
+impl HostHooks for Hooks {
+    fn ensure_can_compile_strings(&self, _realm: Realm, parameters: &[JsString], body: &JsString, _direct: bool, _context: &mut Context) -> JsResult<()> {
+        // `Function`'s parameters are parsed joined with commas (`Function("[[a", "b]]", …)`).
+        let params = parameters.iter().map(JsString::to_std_string_lossy).collect::<Vec<_>>().join(",");
+        for code in [params, body.to_std_string_lossy()] {
+            check_nesting(&code).map_err(|e| JsNativeError::range().with_message(e.trim_start_matches("Error: ").to_string()))?;
+        }
+        Ok(())
+    }
+}
+
 pub(crate) fn new_context() -> Result<Context, String> {
-    let mut ctx = Context::default();
+    let mut ctx = Context::builder().host_hooks(std::rc::Rc::new(Hooks)).build().map_err(|e| e.to_string())?;
     let limits = ctx.runtime_limits_mut();
     limits.set_loop_iteration_limit(200_000_000);
     limits.set_recursion_limit(2000);
@@ -745,10 +763,20 @@ fn run_expanded(session: &mut Session, req: &ScriptRequest) -> Outcome {
 /// The deepest bracket nesting a script may have. The JavaScript parser recurses per level and
 /// overflows the stack (aborting the app, which can't be caught) at about 200 levels on an
 /// 8 MiB thread such as the Script Console's, so deeper scripts are rejected up front.
+/// Constructs that nest without brackets (`x => x => …`, `a ? b : c ? …`, `if (a) if (b) …`,
+/// `!!!…`) are limited too, by the expressions' estimate ([`SCRIPT_WEIGHTS`]).
 pub const MAX_NESTING: usize = 96;
 
+/// What the nesting estimate ([`nests_deeper_than`]) charges a script. Measured on a release
+/// build and scaled to an 8 MiB thread, boa overflows at about 2 400 `=>` links, 4 300 `||`
+/// links, 4 900 comma operators, 15 000 member accesses and 15 000 to 30 000 binary
+/// operators, and never on an object literal's keys or a call's arguments (lists). Each is
+/// allowed at least twice less. The budget is a quarter more than [`MAX_NESTING`]: brackets are
+/// counted exactly against it first, and the estimate charges a call's `(` as a link too.
+const SCRIPT_WEIGHTS: Weights = Weights { levels: MAX_NESTING + MAX_NESTING / 4, operator: 4, access: 6, comma: 16, lists: true };
+
 /// An error when `code` nests brackets deeper than [`MAX_NESTING`] (strings, template literals
-/// and comments are skipped).
+/// and comments are skipped), or nests as deep through constructs without brackets.
 pub fn check_nesting(code: &str) -> Result<(), String> {
     let b = code.as_bytes();
     let (mut i, mut depth, mut max) = (0usize, 0usize, 0usize);
@@ -790,6 +818,9 @@ pub fn check_nesting(code: &str) -> Result<(), String> {
     if max > MAX_NESTING {
         return Err(format!("Error: the script is nested too deeply ({max} levels of brackets; at most {MAX_NESTING})"));
     }
+    if nests_deeper_than(code, SCRIPT_WEIGHTS) {
+        return Err("Error: the script is nested too deeply (too long a chain of operators, or of statements without braces)".to_string());
+    }
     Ok(())
 }
 
@@ -858,6 +889,81 @@ mod nesting_tests {
         assert!(check_nesting(&("(".repeat(MAX_NESTING) + &")".repeat(MAX_NESTING))).is_ok());
     }
 
+    #[test]
+    fn nesting_without_brackets_is_an_error_not_a_stack_overflow() {
+        // Each of these overflowed the parser's stack and aborted the process: they nest as deep
+        // as brackets do but have none to count.
+        let mut s = Session::default();
+        let deep = [
+            "x => ".repeat(20_000) + "1",
+            "if (1) ".repeat(100_000) + "1",
+            "1 ? ".repeat(100_000) + "1" + &" : 1".repeat(100_000),
+            "2 ** ".repeat(100_000) + "1",
+            "1 || ".repeat(100_000) + "1",
+            "var x; ".to_string() + &"x = ".repeat(100_000) + "1",
+            "!".repeat(200_000) + "1",
+            "typeof ".repeat(100_000) + "1",
+            "while (0) ".repeat(100_000) + "1",
+            (0..100_000).map(|i| format!("L{i}: ")).collect::<String>() + "1",
+            // Long left-deep chains, and comma operators where a list could be mistaken for one.
+            "var s = ".to_string() + &"\"a\" + ".repeat(300_000) + "\"a\"",
+            "var o = {}; o.x = o; o".to_string() + &".x".repeat(200_000),
+            "var x = (".to_string() + &"1, ".repeat(100_000) + "1)",
+            "if (1) {} (".to_string() + &"1, ".repeat(100_000) + "1)",
+            "if (1) (".to_string() + &"1, ".repeat(100_000) + "1)",
+            "var a = [0]; a[".to_string() + &"1, ".repeat(100_000) + "0]",
+            "{ ".to_string() + &"1, ".repeat(100_000) + "1 }",
+        ];
+        for code in &deep {
+            let out = run(&mut s, &ScriptRequest { code, name: "deep", console: true });
+            assert!(out.error.is_some_and(|e| e.message.contains("nested too deeply")), "{}", &code[..20]);
+        }
+        // Ordinary code still runs, however long its lists and chains (on a Script Console-sized
+        // stack): object literals and arguments cost nothing, a long concatenation little.
+        let keys = |n: usize| (0..n).map(|i| format!("\"k{i}\": {i}")).collect::<Vec<_>>().join(", ");
+        let elses = (0..200).map(|i| format!("else if (n == {i}) r = {i};")).collect::<String>();
+        let fine = [
+            (format!("var f = {}1; typeof f", "x => ".repeat(40)), json!("function")),
+            (format!("var o = {{{}}}; o.k5999", keys(6000)), json!(5999)),
+            (format!("var json = '{{{}}}'; var o = eval('(' + json + ')'); o.k5999", keys(6000)), json!(5999)),
+            (format!("var s = {}; s.length", vec!["\"a\""; 6000].join(" + ")), json!(6000)),
+            (format!("Math.max({})", (0..20_000).map(|i| i.to_string()).collect::<Vec<_>>().join(", ")), json!(19_999)),
+            (format!("var a = [{}]; a.length", vec!["[1, 2]"; 20_000].join(", ")), json!(20_000)),
+            (format!("var n = 150, r = -1; if (n < 0) r = 0; {elses} r"), json!(150)),
+            (format!("{}1", "!".repeat(40)), json!(true)),
+        ];
+        let ran = std::thread::Builder::new().stack_size(8 << 20).spawn(move || {
+            let mut s = Session::default();
+            for (code, want) in &fine {
+                let out = run(&mut s, &ScriptRequest { code, name: "ok", console: true });
+                assert!(out.error.is_none(), "{:?}: {}", out.error, &code[..40.min(code.len())]);
+                assert_eq!(&out.result, want, "{}", &code[..40.min(code.len())]);
+            }
+        });
+        ran.unwrap().join().unwrap();
+    }
+
+    #[test]
+    fn code_compiled_at_run_time_is_checked_too() {
+        // `eval` and `new Function` handed the parser their code unchecked: deep nesting in a
+        // string aborted the process like deep nesting in the script.
+        let mut s = Session::default();
+        for code in [
+            "eval(Array(100001).join('(') + '1' + Array(100001).join(')'))",
+            "new Function(Array(100001).join('['))",
+            "eval(Array(20001).join('x => ') + '1')",
+            "new Function('a', 'return ' + Array(100001).join('1 ? ') + '1' + Array(100001).join(' : 1'))",
+            // Each parameter nests 80 deep, but they are parsed joined: 240 deep.
+            "Function(Array(81).join('[') + 'a', Array(81).join('[') + 'b', Array(81).join('[') + 'c' + Array(241).join(']'), 'return 1')",
+        ] {
+            let out = run(&mut s, &ScriptRequest { code, name: "eval", console: true });
+            assert!(out.error.is_some_and(|e| e.message.contains("nested too deeply")), "{code}");
+        }
+        let out = run(&mut s, &ScriptRequest { code: "eval('1 + 1') + new Function('a', 'return a * 2')(3)", name: "ok", console: true });
+        assert!(out.error.is_none(), "{:?}", out.error);
+        assert_eq!(out.result, serde_json::json!(8));
+    }
+
     /// A canonical drive path and a lexical one compare once the `\\?\` prefix is gone; a UNC
     /// share and a Unix path are left as they are.
     #[test]