Skip to content

Commit

Permalink
Merge pull request #1817 from xlsynth:cdleary/2024-12-30-expression-d…
Browse files Browse the repository at this point in the history 
…epth-guard

PiperOrigin-RevId: 712597441
  • Loading branch information
@copybara-github
copybara-github committed Jan 6, 2025
2 parents cb4cf1f + 1cf4bf4 commit c646a73
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 22 deletions.
47 changes: 27 additions & 20 deletions xls/dslx/frontend/parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,12 +634,7 @@ absl::StatusOr<Expr*> 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()
<< "`";
Expand Down Expand Up @@ -808,6 +803,8 @@ absl::StatusOr<TypeRef*> Parser::ParseTypeRef(Bindings& bindings,

absl::StatusOr<TypeAnnotation*> Parser::ParseTypeAnnotation(
Bindings& bindings, std::optional<Token> first) {
XLS_ASSIGN_OR_RETURN(ExpressionDepthGuard expr_depth, BumpExpressionDepth());

VLOG(5) << "ParseTypeAnnotation @ " << GetPos();
if (!first.has_value()) {
XLS_ASSIGN_OR_RETURN(first, PopToken());
Expand Down Expand Up @@ -1124,14 +1121,8 @@ absl::StatusOr<NameDefTree*> Parser::ParseNameDefTree(Bindings& bindings) {
this]() -> absl::StatusOr<NameDefTree*> {
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));
Expand Down Expand Up @@ -1416,6 +1407,8 @@ absl::StatusOr<Expr*> Parser::ParseComparisonExpression(
}

absl::StatusOr<NameDefTree*> Parser::ParsePattern(Bindings& bindings) {
XLS_ASSIGN_OR_RETURN(ExpressionDepthGuard depth_guard, BumpExpressionDepth());

XLS_ASSIGN_OR_RETURN(std::optional<Token> oparen,
TryPopToken(TokenKind::kOParen));
if (oparen.has_value()) {
Expand Down Expand Up @@ -1801,12 +1794,7 @@ absl::StatusOr<Expr*> 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()
Expand Down Expand Up @@ -2150,8 +2138,27 @@ absl::StatusOr<Expr*> 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<ExpressionDepthGuard> 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<Expr*> 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) {
Expand Down
36 changes: 36 additions & 0 deletions xls/dslx/frontend/parser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -651,6 +681,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<ExpressionDepthGuard> BumpExpressionDepth();

std::unique_ptr<Module> module_;

// `Let` nodes are created _after_ those that use their namedefs (due to the
Expand Down
36 changes: 35 additions & 1 deletion xls/dslx/frontend/parser_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3163,7 +3163,41 @@ TEST_F(ParserTest, UnreasonablyDeepExpr) {
Parser parser{"test", &s};
absl::StatusOr<std::unique_ptr<Module>> 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<std::unique_ptr<Module>> 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<std::unique_ptr<Module>> 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<std::unique_ptr<Module>> module = parser.ParseModule();
EXPECT_THAT(module, StatusIs(absl::StatusCode::kInvalidArgument,
HasSubstr("Extremely deep nesting detected")));
}

TEST_F(ParserTest, NonTypeDefinitionBeforeArrayLiteralColon) {
Expand Down
2 changes: 1 addition & 1 deletion xls/fuzzer/crashers/crasher_2020-01-08_2173.x
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit c646a73

Please sign in to comment.