diff --git a/xls/dslx/frontend/parser.cc b/xls/dslx/frontend/parser.cc index 98814efc05..5c6a403cad 100644 --- a/xls/dslx/frontend/parser.cc +++ b/xls/dslx/frontend/parser.cc @@ -645,12 +645,7 @@ absl::StatusOr Parser::ParseExpression(Bindings& bindings, ExprRestrictions restrictions) { XLS_ASSIGN_OR_RETURN(const Token* peek, PeekToken()); - if (++approximate_expression_depth_ >= kApproximateExpressionDepthLimit) { - return ParseErrorStatus(peek->span(), - "Expression is too deeply nested, please break " - "into multiple statements"); - } - auto bump_down = absl::Cleanup([this] { approximate_expression_depth_--; }); + XLS_ASSIGN_OR_RETURN(ExpressionDepthGuard expr_depth, BumpExpressionDepth()); VLOG(5) << "ParseExpression @ " << GetPos() << " peek: `" << peek->ToString() << "`"; @@ -819,6 +814,8 @@ absl::StatusOr Parser::ParseTypeRef(Bindings& bindings, absl::StatusOr Parser::ParseTypeAnnotation( Bindings& bindings, std::optional first) { + XLS_ASSIGN_OR_RETURN(ExpressionDepthGuard expr_depth, BumpExpressionDepth()); + VLOG(5) << "ParseTypeAnnotation @ " << GetPos(); if (!first.has_value()) { XLS_ASSIGN_OR_RETURN(first, PopToken()); @@ -1135,14 +1132,8 @@ absl::StatusOr Parser::ParseNameDefTree(Bindings& bindings) { this]() -> absl::StatusOr { XLS_ASSIGN_OR_RETURN(const Token* peek, PeekToken()); if (peek->kind() == TokenKind::kOParen) { - if (++approximate_expression_depth_ >= kApproximateExpressionDepthLimit) { - return ParseErrorStatus( - peek->span(), - "Name definition tree is too deeply nested, please break " - "into multiple statements"); - } - auto bump_down = - absl::Cleanup([this] { approximate_expression_depth_--; }); + XLS_ASSIGN_OR_RETURN(ExpressionDepthGuard expr_depth, + BumpExpressionDepth()); return ParseNameDefTree(bindings); } XLS_ASSIGN_OR_RETURN(auto name_def, ParseNameDefOrWildcard(bindings)); @@ -1427,6 +1418,8 @@ absl::StatusOr Parser::ParseComparisonExpression( } absl::StatusOr Parser::ParsePattern(Bindings& bindings) { + XLS_ASSIGN_OR_RETURN(ExpressionDepthGuard depth_guard, BumpExpressionDepth()); + XLS_ASSIGN_OR_RETURN(std::optional oparen, TryPopToken(TokenKind::kOParen)); if (oparen.has_value()) { @@ -1812,12 +1805,7 @@ absl::StatusOr Parser::ParseTermLhs(Bindings& outer_bindings, ExprRestrictions restrictions) { XLS_ASSIGN_OR_RETURN(const Token* peek, PeekToken()); - if (++approximate_expression_depth_ >= kApproximateExpressionDepthLimit) { - return ParseErrorStatus(peek->span(), - "Expression is too deeply nested, please break " - "into multiple statements"); - } - auto bump_down = absl::Cleanup([this] { approximate_expression_depth_--; }); + XLS_ASSIGN_OR_RETURN(ExpressionDepthGuard expr_depth, BumpExpressionDepth()); const Pos start_pos = peek->span().start(); VLOG(5) << "ParseTerm @ " << start_pos << " peek: `" << peek->ToString() @@ -2161,8 +2149,27 @@ absl::StatusOr Parser::ParseTermRhs(Expr* lhs, Bindings& outer_bindings, return lhs; } +ExpressionDepthGuard::~ExpressionDepthGuard() { + if (parser_ != nullptr) { + parser_->approximate_expression_depth_--; + CHECK_GE(parser_->approximate_expression_depth_, 0); + parser_ = nullptr; + } +} + +absl::StatusOr Parser::BumpExpressionDepth() { + if (++approximate_expression_depth_ >= kApproximateExpressionDepthLimit) { + return ParseErrorStatus(Span(GetPos(), GetPos()), + "Extremely deep nesting detected -- please break " + "into multiple statements"); + } + return ExpressionDepthGuard(this); +} + absl::StatusOr Parser::ParseTerm(Bindings& outer_bindings, ExprRestrictions restrictions) { + XLS_ASSIGN_OR_RETURN(ExpressionDepthGuard expr_depth, BumpExpressionDepth()); + XLS_ASSIGN_OR_RETURN(Expr * lhs, ParseTermLhs(outer_bindings, restrictions)); while (true) { diff --git a/xls/dslx/frontend/parser.h b/xls/dslx/frontend/parser.h index f9813239da..0a9cc86724 100644 --- a/xls/dslx/frontend/parser.h +++ b/xls/dslx/frontend/parser.h @@ -76,6 +76,35 @@ XLS_DEFINE_STRONG_INT_TYPE(ExprRestrictions, uint32_t); constexpr ExprRestrictions kNoRestrictions = ExprRestrictions(0); +// forward declaration +class Parser; + +// RAII guard used to ensure the expression nesting depth does not get +// unreasonably deep (which can cause stack overflows and "erroneously" flag +// fuzzing issues in that dimension). +class ABSL_MUST_USE_RESULT ExpressionDepthGuard final { + public: + explicit ExpressionDepthGuard(Parser* parser) : parser_(parser) {} + ~ExpressionDepthGuard(); + + // move-only type, and we use the parser pointer to track which instance is + // performing the side effect in the destructor if the original is moved + ExpressionDepthGuard(ExpressionDepthGuard&& other) : parser_(other.parser_) { + other.parser_ = nullptr; + } + ExpressionDepthGuard& operator=(ExpressionDepthGuard&& other) { + parser_ = other.parser_; + other.parser_ = nullptr; + return *this; + } + + ExpressionDepthGuard(const ExpressionDepthGuard&) = delete; + ExpressionDepthGuard& operator=(const ExpressionDepthGuard&) = delete; + + private: + Parser* parser_; +}; + class Parser : public TokenParser { public: Parser(std::string module_name, Scanner* scanner) @@ -128,6 +157,7 @@ class Parser : public TokenParser { private: friend class ParserTest; + friend class ExpressionDepthGuard; // Simple helper class to wrap the operations necessary to evaluate [parser] // productions as transactions - with "Commit" or "Rollback" operations. @@ -657,6 +687,12 @@ class Parser : public TokenParser { Bindings& outer_bindings, Keyword keyword); + // Bumps the internally-tracked expression depth so we can provide a useful + // error if we over-recurse on expression depth past the point a user would + // reasonably be expected to do. This (e.g.) helps avoid stack overflows + // during fuzzing. + absl::StatusOr BumpExpressionDepth(); + std::unique_ptr module_; // `Let` nodes are created _after_ those that use their namedefs (due to the diff --git a/xls/dslx/frontend/parser_test.cc b/xls/dslx/frontend/parser_test.cc index d56478ba8e..31ea0f9ead 100644 --- a/xls/dslx/frontend/parser_test.cc +++ b/xls/dslx/frontend/parser_test.cc @@ -3181,7 +3181,41 @@ TEST_F(ParserTest, UnreasonablyDeepExpr) { Parser parser{"test", &s}; absl::StatusOr> module = parser.ParseModule(); EXPECT_THAT(module, StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("Expression is too deeply nested"))); + HasSubstr("Extremely deep nesting detected"))); +} + +// Previously we could avoid the nesting detector by entering an interior +// production in the grammar like a Term. +TEST_F(ParserTest, UnreasonablyDeepTermExprNesting) { + constexpr std::string_view kProgram = R"(const E= +0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0(0()"; + Scanner s{file_table_, Fileno(0), std::string(kProgram)}; + Parser parser{"test", &s}; + absl::StatusOr> module = parser.ParseModule(); + EXPECT_THAT(module, StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("Extremely deep nesting detected"))); +} + +// As above but guards the TypeAnnotation production in the grammar. +TEST_F(ParserTest, UnreasonablyDeepTypeDefinition) { + constexpr std::string_view kProgram = R"(type T= +((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((()"; + Scanner s{file_table_, Fileno(0), std::string(kProgram)}; + Parser parser{"test", &s}; + absl::StatusOr> module = parser.ParseModule(); + EXPECT_THAT(module, StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("Extremely deep nesting detected"))); +} + +// As above but guards the pattern-match production in the grammar. +TEST_F(ParserTest, UnreasonablyDeepPatternMatch) { + constexpr std::string_view kProgram = R"(fn f(x: u32) { match x { +((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((((()"; + Scanner s{file_table_, Fileno(0), std::string(kProgram)}; + Parser parser{"test", &s}; + absl::StatusOr> module = parser.ParseModule(); + EXPECT_THAT(module, StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("Extremely deep nesting detected"))); } TEST_F(ParserTest, NonTypeDefinitionBeforeArrayLiteralColon) { diff --git a/xls/fuzzer/crashers/crasher_2020-01-08_2173.x b/xls/fuzzer/crashers/crasher_2020-01-08_2173.x index 25e7ebd4b0..8261762c1c 100644 --- a/xls/fuzzer/crashers/crasher_2020-01-08_2173.x +++ b/xls/fuzzer/crashers/crasher_2020-01-08_2173.x @@ -78,7 +78,7 @@ fn main(x0: s57, x1: s36, x2: s24, x3: u12) -> (u62, u45, u62) { ; let x20: s29 = (s29:0x40000); let x21: uN[57] = (x0 as u57)[:]; - let x22: uN[1320] = (((((((((((((((((((((((x10) ++ (x18)) ++ (x10)) ++ (x10)) ++ (x18)) ++ (x10)) ++ (x19)) ++ (x6)) ++ (x10)) ++ (x6)) ++ (x18)) ++ (x10)) ++ (x19)) ++ (x19)) ++ (x6)) ++ (x6)) ++ (x10)) ++ (x19)) ++ (x18)) ++ (x6)) ++ (x3)) ++ (x3)) ++ (x10)) ++ (x6); + let x22: uN[1320] = x10 ++ x18 ++ x10 ++ x10 ++ x18 ++ x10 ++ x19 ++ x6 ++ x10 ++ x6 ++ x18 ++ x10 ++ x19 ++ x19 ++ x6 ++ x6 ++ x10 ++ x19 ++ x18 ++ x6 ++ x3 ++ x3 ++ x10 ++ x6; let x23: s24 = for (i, x): (u4, s24) in range((u4:0x0), (u4:0x7)) { x }(x15)