fix: Don't stackoverflow on deeply nested expressions (#4876)

* fix: Don't stackoverflow on deeply nested expressions

* refactor: Avoid checking the string message of a fatal error

* refactor: Move depth checking to the parser

* fix: Add stacker to manage stack growth in the parser

Just in case another environment has a smaller stack size than my local environment we also grow
the stack automatically. We still have the depth limit to prevent the stack from growing unbounded

* test: Add a second stack overflow test

* fix: Ensure the second stack overflow issue errors

Since this test has nested binary expressions it does not overflow the stack or hit the depth checking path in the parser so we also needed the second check.

* chore: make generate

* fix: Don't try to use stacker on wasm

* chore: Don't use stacker on wasm

* chore: make generate

* chore: Don't use stacker on windows either

Not sure why

libflux/flux-core/Cargo.toml

@@ -68,6 +68,9 @@ walkdir = "2.2.9"
 rusqlite = { version = "0.26", optional = true }
 pretty_assertions = { version = "1", optional = true }
 
+[target.'cfg(not(any(target_arch = "wasm32", windows)))'.dependencies]
+stacker = "0.1"
+
 [dev-dependencies]
 colored = "2.0"
 criterion = "0.3.3"

libflux/flux-core/src/ast/check/mod.rs

@@ -11,29 +11,48 @@ use crate::{
 /// Inspects an AST node and returns a list of found AST errors plus
 /// any errors existed before `ast.check()` is performed.
 pub fn check(node: walk::Node) -> Result<(), Errors<Error>> {
-    let mut errors = Errors::new();
-    walk::walk(
-        &mut |n: walk::Node| {
+    const MAX_DEPTH: u32 = 180;
+
+    #[derive(Default)]
+    struct Check {
+        depth: u32,
+        errors: Errors<Error>,
+    }
+
+    impl<'a> walk::Visitor<'a> for Check {
+        fn visit(&mut self, n: walk::Node<'a>) -> bool {
+            self.depth += 1;
+
+            let errors = &mut self.errors;
+
+            if self.depth > MAX_DEPTH {
+                errors.push(located(n.base().location.clone(), ErrorKind::NestedToDeep));
+
+                return false;
+            }
+
             // collect any errors we found prior to ast.check().
             for err in n.base().errors.iter() {
-                errors.push(located(
-                    n.base().location.clone(),
-                    ErrorKind {
+                let err = if err == "Program is nested too deep" {
+                    ErrorKind::NestedToDeep
+                } else {
+                    ErrorKind::Message {
                         message: err.clone(),
-                    },
-                ));
+                    }
+                };
+                errors.push(located(n.base().location.clone(), err));
             }
 
             match n {
                 walk::Node::BadStmt(n) => errors.push(located(
                     n.base.location.clone(),
-                    ErrorKind {
+                    ErrorKind::Message {
                         message: format!("invalid statement: {}", n.text),
                     },
                 )),
                 walk::Node::BadExpr(n) if !n.text.is_empty() => errors.push(located(
                     n.base.location.clone(),
-                    ErrorKind {
+                    ErrorKind::Message {
                         message: format!("invalid expression: {}", n.text),
                     },
                 )),
@@ -48,7 +67,7 @@ pub fn check(node: walk::Node) -> Result<(), Errors<Error>> {
                                     if let PropertyKey::StringLit(s) = &p.key {
                                         errors.push(located(
                                             n.base.location.clone(),
-                                            ErrorKind {
+                                            ErrorKind::Message {
                                                 message: format!(
                                                     "string literal key {} must have a value",
                                                     s.value
@@ -66,7 +85,7 @@ pub fn check(node: walk::Node) -> Result<(), Errors<Error>> {
                     if has_implicit && has_explicit {
                         errors.push(located(
                             n.base.location.clone(),
-                            ErrorKind {
+                            ErrorKind::Message {
                                 message: String::from(
                                     "cannot mix implicit and explicit properties",
                                 ),
@@ -76,9 +95,19 @@ pub fn check(node: walk::Node) -> Result<(), Errors<Error>> {
                 }
                 _ => {}
             }
-        },
-        node,
-    );
+
+            true
+        }
+
+        fn done(&mut self, _: walk::Node<'a>) {
+            self.depth -= 1;
+        }
+    }
+
+    let mut check = Check::default();
+    walk::walk(&mut check, node);
+    let errors = check.errors;
+
     if errors.is_empty() {
         Ok(())
     } else {
@@ -91,10 +120,18 @@ pub type Error = Located<ErrorKind>;
 
 /// An error that can be returned while checking the AST.
 #[derive(Error, Debug, PartialEq)]
-#[error("{}", message)]
-pub struct ErrorKind {
-    /// Error message.
-    pub message: String,
+#[allow(missing_docs)]
+pub enum ErrorKind {
+    #[error("Program is nested too deep")]
+    NestedToDeep,
+    #[error("{message}")]
+    Message { message: String },
+}
+
+impl ErrorKind {
+    pub(crate) fn is_fatal(&self) -> bool {
+        matches!(self, Self::NestedToDeep)
+    }
 }
 
 impl AsDiagnostic for ErrorKind {

libflux/flux-core/src/ast/check/tests.rs

@@ -21,7 +21,7 @@ fn test_object_check() {
             },
             source: Some(String::from("{c: 2, a}")),
         },
-        ErrorKind {
+        ErrorKind::Message {
             message: String::from("cannot mix implicit and explicit properties"),
         },
     )]));
@@ -39,7 +39,7 @@ fn test_bad_stmt() {
             end: Position { line: 3, column: 3 },
             source: Some(String::from("=")),
         },
-        ErrorKind {
+        ErrorKind::Message {
             message: String::from("invalid statement: ="),
         },
     )]));
@@ -60,7 +60,7 @@ fn test_bad_expr() {
             },
             source: Some(String::from("/")),
         },
-        ErrorKind {
+        ErrorKind::Message {
             message: String::from("invalid expression: invalid token for primary expression: DIV"),
         },
     )]));
@@ -131,6 +131,6 @@ fn test_check_collect_existing_error() {
     let got = check(walk::Node::File(&file)).unwrap_err();
     assert_eq!(3, got.len());
     for (i, err) in got.iter().enumerate() {
-        assert_eq!(err.error.message, format!("error {}", i + 1));
+        assert_eq!(err.error.to_string(), format!("error {}", i + 1));
     }
 }

libflux/flux-core/src/parser/mod.rs

@@ -20,6 +20,22 @@ struct TokenError {
     pub token: Token,
 }
 
+const MAX_DEPTH: u32 = 80;
+
+#[cfg(not(any(target_arch = "wasm32", windows)))]
+#[inline]
+fn maybe_grow_stack<T>(f: impl FnOnce() -> T) -> T {
+    // Numbers are arbitrary and were just picked from the stacker docs
+    stacker::maybe_grow(32 * 1024, 1024 * 1024, f)
+}
+
+// Stacker doesn't work on wasm so we just have to rely on checking against `MAX_DEPTH`
+#[cfg(any(target_arch = "wasm32", windows))]
+#[inline]
+fn maybe_grow_stack<T>(f: impl FnOnce() -> T) -> T {
+    f()
+}
+
 /// Represents a Flux parser and its state.
 pub struct Parser<'input> {
     s: Scanner<'input>,
@@ -31,6 +47,8 @@ pub struct Parser<'input> {
 
     fname: String,
     source: &'input str,
+
+    depth: u32,
 }
 
 impl<'input> Parser<'input> {
@@ -44,6 +62,7 @@ impl<'input> Parser<'input> {
             blocks: HashMap::default(),
             fname: "".to_string(),
             source: src,
+            depth: 0,
         }
     }
 
@@ -433,6 +452,17 @@ impl<'input> Parser<'input> {
 
     /// Parses a flux statement
     pub fn parse_statement(&mut self) -> Statement {
+        self.depth_guard(|this| this.parse_statement_inner())
+            .unwrap_or_else(|| {
+                let t = self.consume();
+                Statement::Bad(Box::new(BadStmt {
+                    base: self.base_node_from_token(&t),
+                    text: t.lit,
+                }))
+            })
+    }
+
+    fn parse_statement_inner(&mut self) -> Statement {
         let t = self.peek();
         match t.tok {
             TokenType::Int
@@ -899,10 +929,30 @@ impl<'input> Parser<'input> {
         }
     }
 
+    fn depth_guard<T>(&mut self, f: impl FnOnce(&mut Self) -> T) -> Option<T> {
+        self.depth += 1;
+
+        let x = if self.depth > MAX_DEPTH {
+            self.errs.push("Program is nested too deep".into());
+            None
+        } else {
+            Some(maybe_grow_stack(|| f(self)))
+        };
+
+        self.depth -= 1;
+
+        x
+    }
+
     /// Parses a flux expression
     pub fn parse_expression(&mut self) -> Expression {
-        self.parse_conditional_expression()
+        self.depth_guard(|this| this.parse_conditional_expression())
+            .unwrap_or_else(|| {
+                let t = self.consume();
+                self.create_bad_expression(t)
+            })
     }
+
     // From GoDoc:
     // parseExpressionWhile will continue to parse expressions until
     // the function while returns true.

libflux/flux-core/src/parser/tests/errors.rs

@@ -946,3 +946,23 @@ builtin y : int
                 .to_string(),
         );
 }
+
+#[test]
+fn dont_stack_overflow() {
+    let mut p = Parser::new(include_str!("stack_overflow.flux"));
+    let parsed = p.parse_file("".to_string());
+    assert!(&ast::check::check(ast::walk::Node::File(&parsed))
+        .unwrap_err()
+        .iter()
+        .any(|err| err.error.is_fatal()));
+}
+
+#[test]
+fn dont_stack_overflow_2() {
+    let mut p = Parser::new(include_str!("stack_overflow_2.flux"));
+    let parsed = p.parse_file("".to_string());
+    assert!(&ast::check::check(ast::walk::Node::File(&parsed))
+        .unwrap_err()
+        .iter()
+        .any(|err| err.error.is_fatal()));
+}