From 02b617c63bc9d09314ac1cf37569c75f3c36cdb0 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Mon, 27 Jul 2026 15:27:23 -0700 Subject: [PATCH] Add limit for expression nodes in parsed ASTs. This is mainly to prevent misbehaving macros from generating an AST that is much larger than expected (not directly proportional to source length). PiperOrigin-RevId: 954860770 --- parser/internal/options.h | 1 + parser/internal/pratt_parser_worker.cc | 10 ++ parser/internal/pratt_parser_worker.h | 8 +- parser/options.h | 8 ++ parser/parser.cc | 122 +++++++++++++++---------- parser/parser_test.cc | 49 ++++++++++ 6 files changed, 151 insertions(+), 47 deletions(-) diff --git a/parser/internal/options.h b/parser/internal/options.h index 4066d2619..cca9670a5 100644 --- a/parser/internal/options.h +++ b/parser/internal/options.h @@ -21,6 +21,7 @@ inline constexpr int kDefaultErrorRecoveryLimit = 12; inline constexpr int kDefaultMaxRecursionDepth = 32; inline constexpr int kExpressionSizeCodepointLimit = 100'000; inline constexpr int kDefaultErrorRecoveryTokenLookaheadLimit = 512; +inline constexpr int kDefaultExpressionNodeLimit = 100'000; inline constexpr bool kDefaultAddMacroCalls = false; } // namespace cel::parser_internal diff --git a/parser/internal/pratt_parser_worker.cc b/parser/internal/pratt_parser_worker.cc index c502796f5..f9efca03c 100644 --- a/parser/internal/pratt_parser_worker.cc +++ b/parser/internal/pratt_parser_worker.cc @@ -192,14 +192,24 @@ void ParserWorker::SynchronizeOnDelimiter() { NextToken(); } } + int64_t ParserWorker::NextId(int32_t position) { int64_t id = next_id_++; + if (id > options_.expression_node_limit) { + ReportError(position, "expression node limit exceeded"); + } if (position >= 0) { positions_.insert({id, position}); } return id; } +int64_t ParserWorker::NextId() { return NextId(-1); } + +bool ParserWorker::NodeLimitExceeded() { + return next_id_ > options_.expression_node_limit; +} + int64_t ParserWorker::CopyId(int64_t id) { if (id == 0) { return 0; diff --git a/parser/internal/pratt_parser_worker.h b/parser/internal/pratt_parser_worker.h index b022ae9ee..2e8534e78 100644 --- a/parser/internal/pratt_parser_worker.h +++ b/parser/internal/pratt_parser_worker.h @@ -76,7 +76,8 @@ class ParserWorker { // ID and Position tracking int64_t NextId(int32_t position); int64_t NextId(const Token& token) { return NextId(token.start); } - int64_t NextId() { return next_id_++; } + int64_t NextId(); + bool NodeLimitExceeded(); int64_t CopyId(int64_t id); void EraseId(int64_t id); @@ -1050,6 +1051,11 @@ std::optional PrattParserWorker::TryExpandMacro( if (!expander) { return std::nullopt; } + if (NodeLimitExceeded()) { + ReportError(expr_id, + "could not expand macro: expression node limit exceeded"); + return std::nullopt; + } std::vector macro_args; ExprNode macro_target; diff --git a/parser/options.h b/parser/options.h index eca41810a..eb9d2eeea 100644 --- a/parser/options.h +++ b/parser/options.h @@ -42,6 +42,14 @@ struct ParserOptions final { int error_recovery_token_lookahead_limit = ::cel::parser_internal::kDefaultErrorRecoveryTokenLookaheadLimit; + // Limit on the number of expression nodes in the abstract syntax tree for the + // expression. This prevents cases where macro expansion results in an AST + // that is larger than expected from the source expression. Once exceeded, + // the parser will record an error and stop expanding macros but continue + // parsing to report other errors. + int expression_node_limit = + ::cel::parser_internal::kDefaultExpressionNodeLimit; + // Add macro calls to macro_calls list in source_info. bool add_macro_calls = ::cel::parser_internal::kDefaultAddMacroCalls; diff --git a/parser/parser.cc b/parser/parser.cc index 6a66ffc9a..9bc471fdf 100644 --- a/parser/parser.cc +++ b/parser/parser.cc @@ -165,8 +165,9 @@ SourceRange SourceRangeFromParserRuleContext( class ParserMacroExprFactory final : public MacroExprFactory { public: - explicit ParserMacroExprFactory(const cel::Source& source) - : source_(source) {} + explicit ParserMacroExprFactory(const cel::Source& source, + int expression_node_limit) + : source_(source), expression_node_limit_(expression_node_limit) {} void BeginMacro(SourceRange macro_position) { macro_position_ = macro_position; @@ -203,12 +204,18 @@ class ParserMacroExprFactory final : public MacroExprFactory { int64_t NextId(const SourceRange& range) { auto id = expr_id_++; + if (id > expression_node_limit_ && !node_limit_exceeded_) { + node_limit_exceeded_ = true; + ReportError(range, "expression node limit exceeded"); + } if (range.begin != -1 || range.end != -1) { positions_.insert(std::pair{id, range}); } return id; } + bool is_node_limit_exceeded() const { return node_limit_exceeded_; } + bool HasErrors() const { return error_count_ != 0; } std::vector CollectIssues() { @@ -409,6 +416,8 @@ class ParserMacroExprFactory final : public MacroExprFactory { std::vector errors_; size_t error_count_ = 0; const Source& source_; + int expression_node_limit_; + bool node_limit_exceeded_ = false; SourceRange macro_position_; }; @@ -623,13 +632,14 @@ class ParserVisitor final : public CelBaseVisitor, public antlr4::BaseErrorListener { public: ParserVisitor(const cel::Source& source, int max_recursion_depth, + int max_expression_node_count, const cel::MacroRegistry& macro_registry, bool add_macro_calls = false, bool enable_optional_syntax = false, bool enable_quoted_identifiers = false, bool enable_variadic_logical_operators = false) : source_(source), - factory_(source_), + factory_(source_, max_expression_node_count), macro_registry_(macro_registry), recursion_depth_(0), max_recursion_depth_(max_recursion_depth), @@ -1227,6 +1237,8 @@ std::vector ParserVisitor::visitList( if (!enable_optional_syntax_ && expr_ctx->opt != nullptr) { factory_.ReportError(SourceRangeFromParserRuleContext(ctx), "unsupported syntax '?'"); + // Still generate an ID to detect node limit exceeded. + factory_.NextId(SourceRangeFromParserRuleContext(ctx)); rv.push_back(factory_.NewListElement(factory_.NewUnspecified(0), false)); continue; } @@ -1298,6 +1310,9 @@ std::vector ParserVisitor::visitEntries( if (!enable_optional_syntax_ && ctx->keys[i]->opt) { factory_.ReportError(SourceRangeFromParserRuleContext(ctx), "unsupported syntax '?'"); + // Still generate an ID to detect node limit exceeded. + factory_.NextId(SourceRangeFromParserRuleContext(ctx)); + factory_.NextId(SourceRangeFromParserRuleContext(ctx)); res.push_back(factory_.NewMapEntry(0, factory_.NewUnspecified(0), factory_.NewUnspecified(0), false)); continue; @@ -1461,29 +1476,34 @@ std::vector ParserVisitor::CollectIssues() { Expr ParserVisitor::GlobalCallOrMacroImpl(int64_t expr_id, absl::string_view function, std::vector args) { - if (auto macro = macro_registry_.FindMacro(function, args.size(), false); - macro) { - std::vector macro_args; - if (add_macro_calls_) { - macro_args.reserve(args.size()); - for (const auto& arg : args) { - macro_args.push_back(factory_.BuildMacroCallArg(arg)); - } + auto macro = macro_registry_.FindMacro(function, args.size(), false); + if (!macro) { + return factory_.NewCall(expr_id, function, std::move(args)); + } + if (factory_.is_node_limit_exceeded()) { + return factory_.ReportError( + factory_.GetSourceRange(expr_id), + "could not expand macro: expression node limit exceeded"); + } + std::vector macro_args; + if (add_macro_calls_) { + macro_args.reserve(args.size()); + for (const auto& arg : args) { + macro_args.push_back(factory_.BuildMacroCallArg(arg)); } - factory_.BeginMacro(factory_.GetSourceRange(expr_id)); - auto expr = macro->Expand(factory_, std::nullopt, absl::MakeSpan(args)); - factory_.EndMacro(); - if (expr) { - if (add_macro_calls_) { - factory_.AddMacroCall(expr->id(), function, std::nullopt, - std::move(macro_args)); - } - // We did not end up using `expr_id`. Delete metadata. - factory_.EraseId(expr_id); - return std::move(*expr); + } + factory_.BeginMacro(factory_.GetSourceRange(expr_id)); + auto expr = macro->Expand(factory_, std::nullopt, absl::MakeSpan(args)); + factory_.EndMacro(); + if (expr) { + if (add_macro_calls_) { + factory_.AddMacroCall(expr->id(), function, std::nullopt, + std::move(macro_args)); } + // We did not end up using `expr_id`. Delete metadata. + factory_.EraseId(expr_id); + return std::move(*expr); } - return factory_.NewCall(expr_id, function, std::move(args)); } @@ -1491,30 +1511,39 @@ Expr ParserVisitor::ReceiverCallOrMacroImpl(int64_t expr_id, absl::string_view function, Expr target, std::vector args) { - if (auto macro = macro_registry_.FindMacro(function, args.size(), true); - macro) { - Expr macro_target; - std::vector macro_args; - if (add_macro_calls_) { - macro_args.reserve(args.size()); - macro_target = factory_.BuildMacroCallArg(target); - for (const auto& arg : args) { - macro_args.push_back(factory_.BuildMacroCallArg(arg)); - } + auto macro = macro_registry_.FindMacro(function, args.size(), true); + if (!macro) { + return factory_.NewMemberCall(expr_id, function, std::move(target), + std::move(args)); + } + if (factory_.is_node_limit_exceeded()) { + return factory_.ReportError( + factory_.GetSourceRange(expr_id), + "could not expand macro: expression node limit exceeded"); + } + + Expr macro_target; + std::vector macro_args; + if (add_macro_calls_) { + macro_args.reserve(args.size()); + macro_target = factory_.BuildMacroCallArg(target); + for (const auto& arg : args) { + macro_args.push_back(factory_.BuildMacroCallArg(arg)); } - factory_.BeginMacro(factory_.GetSourceRange(expr_id)); - auto expr = macro->Expand(factory_, std::ref(target), absl::MakeSpan(args)); - factory_.EndMacro(); - if (expr) { - if (add_macro_calls_) { - factory_.AddMacroCall(expr->id(), function, std::move(macro_target), - std::move(macro_args)); - } - // We did not end up using `expr_id`. Delete metadata. - factory_.EraseId(expr_id); - return std::move(*expr); + } + factory_.BeginMacro(factory_.GetSourceRange(expr_id)); + auto expr = macro->Expand(factory_, std::ref(target), absl::MakeSpan(args)); + factory_.EndMacro(); + if (expr) { + if (add_macro_calls_) { + factory_.AddMacroCall(expr->id(), function, std::move(macro_target), + std::move(macro_args)); } + // We did not end up using `expr_id`. Delete metadata. + factory_.EraseId(expr_id); + return std::move(*expr); } + return factory_.NewMemberCall(expr_id, function, std::move(target), std::move(args)); } @@ -1677,8 +1706,9 @@ absl::StatusOr ParseImpl( CelParser parser(&tokens); ExprRecursionListener listener(options.max_recursion_depth); ParserVisitor visitor( - source, options.max_recursion_depth, registry, options.add_macro_calls, - options.enable_optional_syntax, options.enable_quoted_identifiers, + source, options.max_recursion_depth, options.expression_node_limit, + registry, options.add_macro_calls, options.enable_optional_syntax, + options.enable_quoted_identifiers, options.enable_variadic_logical_operators); lexer.removeErrorListeners(); diff --git a/parser/parser_test.cc b/parser/parser_test.cc index 9ea6e7832..2f0aae3e3 100644 --- a/parser/parser_test.cc +++ b/parser/parser_test.cc @@ -2140,6 +2140,55 @@ TEST_P(ParserTest, ParseFailurePopulatesIssues) { EXPECT_EQ(issues[0].location().column, 3); } +TEST_P(ParserTest, ExpressionNodeLimitExceeded) { + auto builder = cel::NewParserBuilder(options_); + builder->GetOptions().expression_node_limit = 2; + ASSERT_OK_AND_ASSIGN(auto parser, std::move(*builder).Build()); + + ASSERT_OK_AND_ASSIGN(auto source, cel::NewSource("a + b + c", "test.cel")); + std::vector issues; + auto ast_result = parser->Parse(*source, &issues); + EXPECT_THAT(ast_result, Not(IsOk())); + ASSERT_THAT(issues, testing::Not(testing::IsEmpty())); + EXPECT_THAT(ast_result.status().message(), + HasSubstr("expression node limit exceeded")); + EXPECT_THAT(issues[0].message(), HasSubstr("expression node limit exceeded")); +} + +TEST_P(ParserTest, MacroExpansionNodeLimitExceeded) { + auto builder = cel::NewParserBuilder(options_); + builder->GetOptions().expression_node_limit = 5; + ASSERT_OK_AND_ASSIGN(auto parser, std::move(*builder).Build()); + + ASSERT_OK_AND_ASSIGN( + auto source, cel::NewSource("[1, 2, 3, 4, 5].map(x, x * 2)", "test.cel")); + std::vector issues; + auto ast_result = parser->Parse(*source, &issues); + EXPECT_THAT(ast_result, Not(IsOk())); + ASSERT_THAT(issues, testing::Not(testing::IsEmpty())); + EXPECT_THAT(ast_result.status().message(), + HasSubstr("expression node limit exceeded")); + EXPECT_THAT( + issues, + testing::Contains(testing::Property( + &cel::ParseIssue::message, + HasSubstr( + "could not expand macro: expression node limit exceeded")))); +} + +TEST_P(ParserTest, MacroExpansionNodeLimitNotExceeded) { + auto builder = cel::NewParserBuilder(options_); + builder->GetOptions().expression_node_limit = 100; + ASSERT_OK_AND_ASSIGN(auto parser, std::move(*builder).Build()); + + ASSERT_OK_AND_ASSIGN( + auto source, cel::NewSource("[1, 2, 3, 4, 5].map(x, x * 2)", "test.cel")); + std::vector issues; + auto ast_result = parser->Parse(*source, &issues); + ASSERT_THAT(ast_result, IsOk()); + EXPECT_THAT(issues, testing::IsEmpty()); +} + std::string ExpressionTestName( const testing::TestParamInfo>& test_info) { const TestInfo& info = std::get<0>(test_info.param);