bound the AST height of flat script expressions

src/script_parser.cpp

@@ -14,6 +14,7 @@
 
 #include "behaviortree_cpp/scripting/operators.hpp"
 
+#include <algorithm>
 #include <charconv>
 
 namespace BT
@@ -55,6 +56,15 @@ class ScriptParser
   // parser already applies in VerifyXML and recursivelyCreateSubtree.
   static constexpr int kMaxNestingDepth = 256;
 
+  // Height of the AST returned by the last parsePrefix() / parseExpr() call.
+  // A flat chain like "1+1+1+..." is parsed by the loop in parseExpr without
+  // recursing, but it builds a left-deep AST that evaluate() and the node
+  // destructors walk recursively, one frame per operator. Bounding the height
+  // bounds that recursion; it is larger than kMaxNestingDepth so that long
+  // flat expressions keep working.
+  int height_ = 0;
+  static constexpr int kMaxExpressionHeight = 1024;
+
   // Binding power constants.  Higher value = tighter binding.
   static constexpr int kAssignmentBP = 2;
   static constexpr int kTernaryBP = 4;
@@ -165,18 +175,36 @@ class ScriptParser
            type == TokenType::StarEqual || type == TokenType::SlashEqual;
   }
 
+  // Height of a node whose children are a subtree of height lhs_height and the
+  // expression parsed last (height_). Throws if the AST gets too tall.
+  int combinedHeight(int lhs_height) const
+  {
+    const int height = std::max(lhs_height, height_) + 1;
+    if(height > kMaxExpressionHeight)
+    {
+      throw RuntimeError(StrCat("Parse error at position ", std::to_string(peek().pos),
+                                ": expression is too long or too deeply nested (limit ",
+                                std::to_string(kMaxExpressionHeight), " levels)"));
+    }
+    return height;
+  }
+
   //--- Parsing ---
 
   /// Prefix: atoms and unary prefix operators
   Ast::expr_ptr parsePrefix()
   {
     const auto& tok = peek();
+    // literals and identifiers are leaves; the cases below that parse a
+    // subexpression overwrite it
+    height_ = 1;
 
     // Unary minus
     if(tok.type == TokenType::Minus)
     {
       advance();
       auto operand = parseExpr(kPrefixBP);
+      height_ = combinedHeight(0);
       return std::make_shared<Ast::ExprUnaryArithmetic>(Ast::ExprUnaryArithmetic::negate,
                                                         std::move(operand));
     }
@@ -185,6 +213,7 @@ class ScriptParser
     {
       advance();
       auto operand = parseExpr(kPrefixBP);
+      height_ = combinedHeight(0);
       return std::make_shared<Ast::ExprUnaryArithmetic>(
           Ast::ExprUnaryArithmetic::complement, std::move(operand));
     }
@@ -193,6 +222,7 @@ class ScriptParser
     {
       advance();
       auto operand = parseExpr(kPrefixBP);
+      height_ = combinedHeight(0);
       return std::make_shared<Ast::ExprUnaryArithmetic>(
           Ast::ExprUnaryArithmetic::logical_not, std::move(operand));
     }
@@ -280,6 +310,7 @@ class ScriptParser
     }
 
     auto left = parsePrefix();
+    int left_height = height_;
 
     while(true)
     {
@@ -294,20 +325,23 @@ class ScriptParser
       if(isAssignment(tokType))
       {
         left = parseAssignment(std::move(left));
+        left_height = combinedHeight(left_height);
         break;
       }
 
       // Ternary (non-associative: parse once, then break)
       if(tokType == TokenType::Question)
       {
         left = parseTernary(std::move(left));
+        left_height = combinedHeight(left_height);
         break;
       }
 
       // Chained comparison
       if(isComparison(tokType))
       {
         left = parseChainedComparison(std::move(left));
+        left_height = combinedHeight(left_height);
         continue;
       }
 
@@ -316,9 +350,11 @@ class ScriptParser
       // Right BP = LBP + 1 for left-associativity
       auto right = parseExpr(lbp + 1);
       left = makeBinary(std::move(left), opTok.type, std::move(right));
+      left_height = combinedHeight(left_height);
     }
 
     --depth_;
+    height_ = left_height;
     return left;
   }
 
@@ -358,8 +394,10 @@ class ScriptParser
   {
     advance();                     // consume '?'
     auto thenExpr = parseExpr(0);  // full expression inside
+    const int then_height = height_;
     expect(TokenType::Colon, "expected ':' in ternary expression");
     auto elseExpr = parseExpr(kTernaryBP);
+    height_ = std::max(then_height, height_);
     return std::make_shared<Ast::ExprIf>(std::move(condition), std::move(thenExpr),
                                          std::move(elseExpr));
   }
@@ -369,13 +407,16 @@ class ScriptParser
     auto node = std::make_shared<Ast::ExprComparison>();
     node->operands.push_back(std::move(first));
 
+    int max_height = 0;
     while(isComparison(peek().type))
     {
       node->ops.push_back(mapComparisonOp(advance().type));
       // Parse the next operand above comparison level
       // so that arithmetic binds tighter
       node->operands.push_back(parseExpr(kComparisonBP + 1));
+      max_height = std::max(max_height, height_);
     }
+    height_ = max_height;
     return node;
   }
 

tests/script_parser_test.cpp

@@ -143,6 +143,45 @@ TEST(ParserTest, DeeplyNestedScriptIsRejected)
   EXPECT_TRUE(BT::ValidateScript("((((1))))"));
 }
 
+TEST(ParserTest, LongFlatScriptIsRejected)
+{
+  // A flat chain like "1+1+1+..." is parsed without recursion, but it builds a
+  // left-deep AST that is evaluated (and destroyed) recursively, one stack frame
+  // per operator. 100k terms used to overflow the stack (SIGSEGV) when executed.
+  BT::Ast::Environment env = { BT::Blackboard::create(), {} };
+
+  std::string flat = "x := 1";
+  for(int i = 0; i < 100000; ++i)
+  {
+    flat += "+1";
+  }
+  EXPECT_FALSE(BT::ValidateScript(flat));
+  EXPECT_FALSE(BT::ParseScriptAndExecute(env, flat).has_value());
+
+  // Nested chains add up as well: 100 levels of 20 operators each.
+  std::string nested_chain = "1";
+  for(int i = 0; i < 100; ++i)
+  {
+    nested_chain = "(" + nested_chain;
+    for(int j = 0; j < 20; ++j)
+    {
+      nested_chain += "+1";
+    }
+    nested_chain += ")";
+  }
+  EXPECT_FALSE(BT::ValidateScript(nested_chain));
+
+  // Ordinary long expressions are still accepted and evaluated.
+  std::string wide = "1";
+  for(int i = 0; i < 1000; ++i)
+  {
+    wide += "+1";
+  }
+  auto result = BT::ParseScriptAndExecute(env, wide);
+  ASSERT_TRUE(result.has_value());
+  EXPECT_EQ(result->cast<int>(), 1001);
+}
+
 TEST(ParserTest, Equations)
 {
   BT::Ast::Environment environment = { BT::Blackboard::create(), {} };