From cf4deab60e30532c4a463ee1827ddd4833fec879 Mon Sep 17 00:00:00 2001 From: Ethan Nicholas Date: Fri, 13 Sep 2019 16:28:14 -0400 Subject: [PATCH] Improved skslc depth tracking This fixes a class of bugs discovered by fuzzing, in which a very complicated expression leads to a stack overflow. Bug: oss-fuzz:15510 Change-Id: Idee6df8bb1dd4ca101bcc0ef21a974c58017f8a8 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/240510 Commit-Queue: Ethan Nicholas Reviewed-by: Brian Osman --- src/sksl/SkSLParser.cpp | 82 +++++++++++++++++++++++++++++++++++------ 1 file changed, 70 insertions(+), 12 deletions(-) diff --git a/src/sksl/SkSLParser.cpp b/src/sksl/SkSLParser.cpp index d2e687907c..8cfb8d4ca7 100644 --- a/src/sksl/SkSLParser.cpp +++ b/src/sksl/SkSLParser.cpp @@ -23,15 +23,16 @@ namespace SkSL { class AutoDepth { public: AutoDepth(Parser* p) - : fParser(p) { - fParser->fDepth++; - } + : fParser(p) + , fDepth(0) {} ~AutoDepth() { - fParser->fDepth--; + fParser->fDepth -= fDepth; } - bool checkValid() { + bool increase() { + ++fDepth; + ++fParser->fDepth; if (fParser->fDepth > MAX_PARSE_DEPTH) { fParser->error(fParser->peek(), String("exceeded max parse depth")); return false; @@ -41,6 +42,7 @@ public: private: Parser* fParser; + int fDepth; }; std::unordered_map* Parser::layoutTokens; @@ -1016,7 +1018,7 @@ Modifiers Parser::modifiersWithDefaults(int defaultFlags) { ASTNode::ID Parser::statement() { Token start = this->nextToken(); AutoDepth depth(this); - if (!depth.checkValid()) { + if (!depth.increase()) { return ASTNode::ID::Invalid(); } this->pushback(start); @@ -1450,7 +1452,7 @@ ASTNode::ID Parser::block() { return ASTNode::ID::Invalid(); } AutoDepth depth(this); - if (!depth.checkValid()) { + if (!depth.increase()) { return ASTNode::ID::Invalid(); } CREATE_NODE(result, start.fOffset, ASTNode::Kind::kBlock); @@ -1510,6 +1512,7 @@ ASTNode::ID Parser::expression() { assignmentExpression)* */ ASTNode::ID Parser::assignmentExpression() { + AutoDepth depth(this); ASTNode::ID result = this->ternaryExpression(); if (!result) { return ASTNode::ID::Invalid(); @@ -1530,6 +1533,9 @@ ASTNode::ID Parser::assignmentExpression() { case Token::LOGICALANDEQ: // fall through case Token::LOGICALXOREQ: // fall through case Token::LOGICALOREQ: { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } Token t = this->nextToken(); ASTNode::ID right = this->assignmentExpression(); if (!right) { @@ -1550,11 +1556,15 @@ ASTNode::ID Parser::assignmentExpression() { /* logicalOrExpression ('?' expression ':' assignmentExpression)? */ ASTNode::ID Parser::ternaryExpression() { + AutoDepth depth(this); ASTNode::ID base = this->logicalOrExpression(); if (!base) { return ASTNode::ID::Invalid(); } if (this->checkNext(Token::QUESTION)) { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } ASTNode::ID trueExpr = this->expression(); if (!trueExpr) { return ASTNode::ID::Invalid(); @@ -1577,12 +1587,16 @@ ASTNode::ID Parser::ternaryExpression() { /* logicalXorExpression (LOGICALOR logicalXorExpression)* */ ASTNode::ID Parser::logicalOrExpression() { + AutoDepth depth(this); ASTNode::ID result = this->logicalXorExpression(); if (!result) { return ASTNode::ID::Invalid(); } Token t; while (this->checkNext(Token::LOGICALOR, &t)) { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } ASTNode::ID right = this->logicalXorExpression(); if (!right) { return ASTNode::ID::Invalid(); @@ -1597,12 +1611,16 @@ ASTNode::ID Parser::logicalOrExpression() { /* logicalAndExpression (LOGICALXOR logicalAndExpression)* */ ASTNode::ID Parser::logicalXorExpression() { + AutoDepth depth(this); ASTNode::ID result = this->logicalAndExpression(); if (!result) { return ASTNode::ID::Invalid(); } Token t; while (this->checkNext(Token::LOGICALXOR, &t)) { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } ASTNode::ID right = this->logicalAndExpression(); if (!right) { return ASTNode::ID::Invalid(); @@ -1617,12 +1635,16 @@ ASTNode::ID Parser::logicalXorExpression() { /* bitwiseOrExpression (LOGICALAND bitwiseOrExpression)* */ ASTNode::ID Parser::logicalAndExpression() { + AutoDepth depth(this); ASTNode::ID result = this->bitwiseOrExpression(); if (!result) { return ASTNode::ID::Invalid(); } Token t; while (this->checkNext(Token::LOGICALAND, &t)) { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } ASTNode::ID right = this->bitwiseOrExpression(); if (!right) { return ASTNode::ID::Invalid(); @@ -1637,12 +1659,16 @@ ASTNode::ID Parser::logicalAndExpression() { /* bitwiseXorExpression (BITWISEOR bitwiseXorExpression)* */ ASTNode::ID Parser::bitwiseOrExpression() { + AutoDepth depth(this); ASTNode::ID result = this->bitwiseXorExpression(); if (!result) { return ASTNode::ID::Invalid(); } Token t; while (this->checkNext(Token::BITWISEOR, &t)) { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } ASTNode::ID right = this->bitwiseXorExpression(); if (!right) { return ASTNode::ID::Invalid(); @@ -1657,12 +1683,16 @@ ASTNode::ID Parser::bitwiseOrExpression() { /* bitwiseAndExpression (BITWISEXOR bitwiseAndExpression)* */ ASTNode::ID Parser::bitwiseXorExpression() { + AutoDepth depth(this); ASTNode::ID result = this->bitwiseAndExpression(); if (!result) { return ASTNode::ID::Invalid(); } Token t; while (this->checkNext(Token::BITWISEXOR, &t)) { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } ASTNode::ID right = this->bitwiseAndExpression(); if (!right) { return ASTNode::ID::Invalid(); @@ -1677,12 +1707,16 @@ ASTNode::ID Parser::bitwiseXorExpression() { /* equalityExpression (BITWISEAND equalityExpression)* */ ASTNode::ID Parser::bitwiseAndExpression() { + AutoDepth depth(this); ASTNode::ID result = this->equalityExpression(); if (!result) { return ASTNode::ID::Invalid(); } Token t; while (this->checkNext(Token::BITWISEAND, &t)) { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } ASTNode::ID right = this->equalityExpression(); if (!right) { return ASTNode::ID::Invalid(); @@ -1697,6 +1731,7 @@ ASTNode::ID Parser::bitwiseAndExpression() { /* relationalExpression ((EQEQ | NEQ) relationalExpression)* */ ASTNode::ID Parser::equalityExpression() { + AutoDepth depth(this); ASTNode::ID result = this->relationalExpression(); if (!result) { return ASTNode::ID::Invalid(); @@ -1705,6 +1740,9 @@ ASTNode::ID Parser::equalityExpression() { switch (this->peek().fKind) { case Token::EQEQ: // fall through case Token::NEQ: { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } Token t = this->nextToken(); ASTNode::ID right = this->relationalExpression(); if (!right) { @@ -1725,6 +1763,7 @@ ASTNode::ID Parser::equalityExpression() { /* shiftExpression ((LT | GT | LTEQ | GTEQ) shiftExpression)* */ ASTNode::ID Parser::relationalExpression() { + AutoDepth depth(this); ASTNode::ID result = this->shiftExpression(); if (!result) { return ASTNode::ID::Invalid(); @@ -1735,6 +1774,9 @@ ASTNode::ID Parser::relationalExpression() { case Token::GT: // fall through case Token::LTEQ: // fall through case Token::GTEQ: { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } Token t = this->nextToken(); ASTNode::ID right = this->shiftExpression(); if (!right) { @@ -1755,6 +1797,7 @@ ASTNode::ID Parser::relationalExpression() { /* additiveExpression ((SHL | SHR) additiveExpression)* */ ASTNode::ID Parser::shiftExpression() { + AutoDepth depth(this); ASTNode::ID result = this->additiveExpression(); if (!result) { return ASTNode::ID::Invalid(); @@ -1763,6 +1806,9 @@ ASTNode::ID Parser::shiftExpression() { switch (this->peek().fKind) { case Token::SHL: // fall through case Token::SHR: { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } Token t = this->nextToken(); ASTNode::ID right = this->additiveExpression(); if (!right) { @@ -1783,6 +1829,7 @@ ASTNode::ID Parser::shiftExpression() { /* multiplicativeExpression ((PLUS | MINUS) multiplicativeExpression)* */ ASTNode::ID Parser::additiveExpression() { + AutoDepth depth(this); ASTNode::ID result = this->multiplicativeExpression(); if (!result) { return ASTNode::ID::Invalid(); @@ -1791,6 +1838,9 @@ ASTNode::ID Parser::additiveExpression() { switch (this->peek().fKind) { case Token::PLUS: // fall through case Token::MINUS: { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } Token t = this->nextToken(); ASTNode::ID right = this->multiplicativeExpression(); if (!right) { @@ -1811,6 +1861,7 @@ ASTNode::ID Parser::additiveExpression() { /* unaryExpression ((STAR | SLASH | PERCENT) unaryExpression)* */ ASTNode::ID Parser::multiplicativeExpression() { + AutoDepth depth(this); ASTNode::ID result = this->unaryExpression(); if (!result) { return ASTNode::ID::Invalid(); @@ -1820,6 +1871,9 @@ ASTNode::ID Parser::multiplicativeExpression() { case Token::STAR: // fall through case Token::SLASH: // fall through case Token::PERCENT: { + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } Token t = this->nextToken(); ASTNode::ID right = this->unaryExpression(); if (!right) { @@ -1840,6 +1894,7 @@ ASTNode::ID Parser::multiplicativeExpression() { /* postfixExpression | (PLUS | MINUS | NOT | PLUSPLUS | MINUSMINUS) unaryExpression */ ASTNode::ID Parser::unaryExpression() { + AutoDepth depth(this); switch (this->peek().fKind) { case Token::PLUS: // fall through case Token::MINUS: // fall through @@ -1847,11 +1902,10 @@ ASTNode::ID Parser::unaryExpression() { case Token::BITWISENOT: // fall through case Token::PLUSPLUS: // fall through case Token::MINUSMINUS: { - Token t = this->nextToken(); - AutoDepth depth(this); - if (!depth.checkValid()) { + if (!depth.increase()) { return ASTNode::ID::Invalid(); } + Token t = this->nextToken(); ASTNode::ID expr = this->unaryExpression(); if (!expr) { return ASTNode::ID::Invalid(); @@ -1867,6 +1921,7 @@ ASTNode::ID Parser::unaryExpression() { /* term suffix* */ ASTNode::ID Parser::postfixExpression() { + AutoDepth depth(this); ASTNode::ID result = this->term(); if (!result) { return ASTNode::ID::Invalid(); @@ -1880,6 +1935,9 @@ ASTNode::ID Parser::postfixExpression() { case Token::MINUSMINUS: case Token::COLONCOLON: case Token::FLOAT_LITERAL: + if (!depth.increase()) { + return ASTNode::ID::Invalid(); + } result = this->suffix(result); if (!result) { return ASTNode::ID::Invalid(); @@ -1897,7 +1955,7 @@ ASTNode::ID Parser::suffix(ASTNode::ID base) { SkASSERT(base); Token next = this->nextToken(); AutoDepth depth(this); - if (!depth.checkValid()) { + if (!depth.increase()) { return ASTNode::ID::Invalid(); } switch (next.fKind) { @@ -2021,7 +2079,7 @@ ASTNode::ID Parser::term() { case Token::LPAREN: { this->nextToken(); AutoDepth depth(this); - if (!depth.checkValid()) { + if (!depth.increase()) { return ASTNode::ID::Invalid(); } ASTNode::ID result = this->expression();