From 0ef58efbc1ef275992a529366ff1133aee0dfe9d Mon Sep 17 00:00:00 2001 From: Aysha Afrah Ziya Date: Sun, 25 Oct 2026 22:55:23 +0530 Subject: [PATCH 1/2] bound the AST height of flat script expressions The nesting cap in ScriptParser::parseExpr counts parser recursion only. A flat chain such as 1+1+1+... is parsed by the loop without recursing, but it builds a left-deep AST that evaluate() and the node destructors walk recursively, one frame per operator, so a long Script attribute passes tree creation and overflows the stack on the first tick. Track the height of the expression being built and reject it past 1024 levels, so the evaluation depth no longer grows with the script length. --- src/script_parser.cpp | 41 ++++++++++++++++++++++++++++++++++++ tests/script_parser_test.cpp | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/src/script_parser.cpp b/src/script_parser.cpp index 34f1d5e8a..7ffa67aa7 100644 --- a/src/script_parser.cpp +++ b/src/script_parser.cpp @@ -14,6 +14,7 @@ #include "behaviortree_cpp/scripting/operators.hpp" +#include #include 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::negate, std::move(operand)); } @@ -185,6 +213,7 @@ class ScriptParser { advance(); auto operand = parseExpr(kPrefixBP); + height_ = combinedHeight(0); return std::make_shared( 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::logical_not, std::move(operand)); } @@ -280,6 +310,7 @@ class ScriptParser } auto left = parsePrefix(); + int left_height = height_; while(true) { @@ -294,6 +325,7 @@ class ScriptParser if(isAssignment(tokType)) { left = parseAssignment(std::move(left)); + left_height = combinedHeight(left_height); break; } @@ -301,6 +333,7 @@ class ScriptParser if(tokType == TokenType::Question) { left = parseTernary(std::move(left)); + left_height = combinedHeight(left_height); break; } @@ -308,6 +341,7 @@ class ScriptParser 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(std::move(condition), std::move(thenExpr), std::move(elseExpr)); } @@ -369,13 +407,16 @@ class ScriptParser auto node = std::make_shared(); 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; } diff --git a/tests/script_parser_test.cpp b/tests/script_parser_test.cpp index 9a4360eff..a9bc67b74 100644 --- a/tests/script_parser_test.cpp +++ b/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(), 1001); +} + TEST(ParserTest, Equations) { BT::Ast::Environment environment = { BT::Blackboard::create(), {} }; From f7cc673f84c9e843caa16fefc592cf6f36f77016 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sat, 7 Nov 2026 18:31:01 +0100 Subject: [PATCH 2/2] use the nesting-depth limit (256) for the script expression height The height limit of 1024 still let accepted scripts overflow the 1 MB default stack of an unoptimized Windows build when evaluated: each level of ExprBinaryArithmetic::evaluate() can take about 1 KB there, which is why ParserTest.LongFlatScriptIsRejected crashed in the windows-2022 Debug job. Use kMaxNestingDepth (256) for both limits, shorten the accepted 'wide' scripts to 200 terms and check that 300 terms are rejected. Co-Authored-By: Claude Opus 5.5 --- src/script_parser.cpp | 7 ++++--- tests/script_parser_test.cpp | 14 +++++++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/script_parser.cpp b/src/script_parser.cpp index 7ffa67aa7..5b696cf51 100644 --- a/src/script_parser.cpp +++ b/src/script_parser.cpp @@ -60,10 +60,11 @@ class ScriptParser // 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. + // bounds that recursion. It uses the same limit as kMaxNestingDepth: each + // level of evaluate() can take about 1 KB of stack in an unoptimized MSVC + // build, so 1024 levels overflowed the 1 MB default stack on Windows. int height_ = 0; - static constexpr int kMaxExpressionHeight = 1024; + static constexpr int kMaxExpressionHeight = kMaxNestingDepth; // Binding power constants. Higher value = tighter binding. static constexpr int kAssignmentBP = 2; diff --git a/tests/script_parser_test.cpp b/tests/script_parser_test.cpp index d34ca16aa..6715431b2 100644 --- a/tests/script_parser_test.cpp +++ b/tests/script_parser_test.cpp @@ -135,7 +135,7 @@ TEST(ParserTest, DeeplyNestedScriptIsRejected) // The depth cap must not reject ordinary scripts: a long flat expression is // parsed iteratively (shallow recursion) and light nesting is well within it. std::string wide = "1"; - for(int i = 0; i < 1000; ++i) + for(int i = 0; i < 200; ++i) { wide += "+1"; } @@ -171,15 +171,23 @@ TEST(ParserTest, LongFlatScriptIsRejected) } EXPECT_FALSE(BT::ValidateScript(nested_chain)); + // The limit is the same as the nesting depth (256): 300 terms are rejected. + std::string over = "1"; + for(int i = 0; i < 300; ++i) + { + over += "+1"; + } + EXPECT_FALSE(BT::ValidateScript(over)); + // Ordinary long expressions are still accepted and evaluated. std::string wide = "1"; - for(int i = 0; i < 1000; ++i) + for(int i = 0; i < 200; ++i) { wide += "+1"; } auto result = BT::ParseScriptAndExecute(env, wide); ASSERT_TRUE(result.has_value()); - EXPECT_EQ(result->cast(), 1001); + EXPECT_EQ(result->cast(), 201); } TEST(ParserTest, Equations)