Skip to content

Commit 180324d

Browse files
bound the AST height of flat script expressions (#1259)
Bound the AST height of script expressions to kMaxNestingDepth (256), so that evaluating or destroying a long flat chain like 1+1+1+... cannot overflow the stack (1 MB default on Windows). Behavior change: a single script expression with more than ~255 chained operators is now rejected when the tree is created.
1 parent b6709b6 commit 180324d

2 files changed

Lines changed: 90 additions & 1 deletion

File tree

‎src/script_parser.cpp‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
#include "behaviortree_cpp/scripting/operators.hpp"
1616

17+
#include <algorithm>
1718
#include <charconv>
1819

1920
namespace BT
@@ -55,6 +56,16 @@ class ScriptParser
5556
// parser already applies in VerifyXML and recursivelyCreateSubtree.
5657
static constexpr int kMaxNestingDepth = 256;
5758

59+
// Height of the AST returned by the last parsePrefix() / parseExpr() call.
60+
// A flat chain like "1+1+1+..." is parsed by the loop in parseExpr without
61+
// recursing, but it builds a left-deep AST that evaluate() and the node
62+
// destructors walk recursively, one frame per operator. Bounding the height
63+
// bounds that recursion. It uses the same limit as kMaxNestingDepth: each
64+
// level of evaluate() can take about 1 KB of stack in an unoptimized MSVC
65+
// build, so 1024 levels overflowed the 1 MB default stack on Windows.
66+
int height_ = 0;
67+
static constexpr int kMaxExpressionHeight = kMaxNestingDepth;
68+
5869
// Binding power constants. Higher value = tighter binding.
5970
static constexpr int kAssignmentBP = 2;
6071
static constexpr int kTernaryBP = 4;
@@ -165,18 +176,36 @@ class ScriptParser
165176
type == TokenType::StarEqual || type == TokenType::SlashEqual;
166177
}
167178

179+
// Height of a node whose children are a subtree of height lhs_height and the
180+
// expression parsed last (height_). Throws if the AST gets too tall.
181+
int combinedHeight(int lhs_height) const
182+
{
183+
const int height = std::max(lhs_height, height_) + 1;
184+
if(height > kMaxExpressionHeight)
185+
{
186+
throw RuntimeError(StrCat("Parse error at position ", std::to_string(peek().pos),
187+
": expression is too long or too deeply nested (limit ",
188+
std::to_string(kMaxExpressionHeight), " levels)"));
189+
}
190+
return height;
191+
}
192+
168193
//--- Parsing ---
169194

170195
/// Prefix: atoms and unary prefix operators
171196
Ast::expr_ptr parsePrefix()
172197
{
173198
const auto& tok = peek();
199+
// literals and identifiers are leaves; the cases below that parse a
200+
// subexpression overwrite it
201+
height_ = 1;
174202

175203
// Unary minus
176204
if(tok.type == TokenType::Minus)
177205
{
178206
advance();
179207
auto operand = parseExpr(kPrefixBP);
208+
height_ = combinedHeight(0);
180209
return std::make_shared<Ast::ExprUnaryArithmetic>(Ast::ExprUnaryArithmetic::negate,
181210
std::move(operand));
182211
}
@@ -185,6 +214,7 @@ class ScriptParser
185214
{
186215
advance();
187216
auto operand = parseExpr(kPrefixBP);
217+
height_ = combinedHeight(0);
188218
return std::make_shared<Ast::ExprUnaryArithmetic>(
189219
Ast::ExprUnaryArithmetic::complement, std::move(operand));
190220
}
@@ -193,6 +223,7 @@ class ScriptParser
193223
{
194224
advance();
195225
auto operand = parseExpr(kPrefixBP);
226+
height_ = combinedHeight(0);
196227
return std::make_shared<Ast::ExprUnaryArithmetic>(
197228
Ast::ExprUnaryArithmetic::logical_not, std::move(operand));
198229
}
@@ -280,6 +311,7 @@ class ScriptParser
280311
}
281312

282313
auto left = parsePrefix();
314+
int left_height = height_;
283315

284316
while(true)
285317
{
@@ -294,20 +326,23 @@ class ScriptParser
294326
if(isAssignment(tokType))
295327
{
296328
left = parseAssignment(std::move(left));
329+
left_height = combinedHeight(left_height);
297330
break;
298331
}
299332

300333
// Ternary (non-associative: parse once, then break)
301334
if(tokType == TokenType::Question)
302335
{
303336
left = parseTernary(std::move(left));
337+
left_height = combinedHeight(left_height);
304338
break;
305339
}
306340

307341
// Chained comparison
308342
if(isComparison(tokType))
309343
{
310344
left = parseChainedComparison(std::move(left));
345+
left_height = combinedHeight(left_height);
311346
continue;
312347
}
313348

@@ -316,9 +351,11 @@ class ScriptParser
316351
// Right BP = LBP + 1 for left-associativity
317352
auto right = parseExpr(lbp + 1);
318353
left = makeBinary(std::move(left), opTok.type, std::move(right));
354+
left_height = combinedHeight(left_height);
319355
}
320356

321357
--depth_;
358+
height_ = left_height;
322359
return left;
323360
}
324361

@@ -358,8 +395,10 @@ class ScriptParser
358395
{
359396
advance(); // consume '?'
360397
auto thenExpr = parseExpr(0); // full expression inside
398+
const int then_height = height_;
361399
expect(TokenType::Colon, "expected ':' in ternary expression");
362400
auto elseExpr = parseExpr(kTernaryBP);
401+
height_ = std::max(then_height, height_);
363402
return std::make_shared<Ast::ExprIf>(std::move(condition), std::move(thenExpr),
364403
std::move(elseExpr));
365404
}
@@ -369,13 +408,16 @@ class ScriptParser
369408
auto node = std::make_shared<Ast::ExprComparison>();
370409
node->operands.push_back(std::move(first));
371410

411+
int max_height = 0;
372412
while(isComparison(peek().type))
373413
{
374414
node->ops.push_back(mapComparisonOp(advance().type));
375415
// Parse the next operand above comparison level
376416
// so that arithmetic binds tighter
377417
node->operands.push_back(parseExpr(kComparisonBP + 1));
418+
max_height = std::max(max_height, height_);
378419
}
420+
height_ = max_height;
379421
return node;
380422
}
381423

‎tests/script_parser_test.cpp‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,61 @@ TEST(ParserTest, DeeplyNestedScriptIsRejected)
135135
// The depth cap must not reject ordinary scripts: a long flat expression is
136136
// parsed iteratively (shallow recursion) and light nesting is well within it.
137137
std::string wide = "1";
138-
for(int i = 0; i < 1000; ++i)
138+
for(int i = 0; i < 200; ++i)
139139
{
140140
wide += "+1";
141141
}
142142
EXPECT_TRUE(BT::ValidateScript(wide));
143143
EXPECT_TRUE(BT::ValidateScript("((((1))))"));
144144
}
145145

146+
TEST(ParserTest, LongFlatScriptIsRejected)
147+
{
148+
// A flat chain like "1+1+1+..." is parsed without recursion, but it builds a
149+
// left-deep AST that is evaluated (and destroyed) recursively, one stack frame
150+
// per operator. 100k terms used to overflow the stack (SIGSEGV) when executed.
151+
BT::Ast::Environment env = { BT::Blackboard::create(), {} };
152+
153+
std::string flat = "x := 1";
154+
for(int i = 0; i < 100000; ++i)
155+
{
156+
flat += "+1";
157+
}
158+
EXPECT_FALSE(BT::ValidateScript(flat));
159+
EXPECT_FALSE(BT::ParseScriptAndExecute(env, flat).has_value());
160+
161+
// Nested chains add up as well: 100 levels of 20 operators each.
162+
std::string nested_chain = "1";
163+
for(int i = 0; i < 100; ++i)
164+
{
165+
nested_chain = "(" + nested_chain;
166+
for(int j = 0; j < 20; ++j)
167+
{
168+
nested_chain += "+1";
169+
}
170+
nested_chain += ")";
171+
}
172+
EXPECT_FALSE(BT::ValidateScript(nested_chain));
173+
174+
// The limit is the same as the nesting depth (256): 300 terms are rejected.
175+
std::string over = "1";
176+
for(int i = 0; i < 300; ++i)
177+
{
178+
over += "+1";
179+
}
180+
EXPECT_FALSE(BT::ValidateScript(over));
181+
182+
// Ordinary long expressions are still accepted and evaluated.
183+
std::string wide = "1";
184+
for(int i = 0; i < 200; ++i)
185+
{
186+
wide += "+1";
187+
}
188+
auto result = BT::ParseScriptAndExecute(env, wide);
189+
ASSERT_TRUE(result.has_value());
190+
EXPECT_EQ(result->cast<int>(), 201);
191+
}
192+
146193
TEST(ParserTest, Equations)
147194
{
148195
BT::Ast::Environment environment = { BT::Blackboard::create(), {} };

0 commit comments

Comments
 (0)