Fix invalid variable ref-kind discovered by fuzzer.
No-op arithmetic simplification will convert expressions like `x += 0`
to `x`. When making this simplification, we will also downgrade the ref-
kind of `x` from "write" to "read" since the new expression is no longer
an assignment.
The fuzzer discovered that the ref-kind downgrade was too aggressive,
and would also traverse into nested subexpressions and downgrade them
as well. That is, for `x[y=z] += 0` would convert both `x` and `y`
into "read" references, which is incorrect; `y` is still being written
to.
The fuzzer managed to turn this mistake into an assertion by leveraging
a separate optimization. It added a leading, side-effect-less comma
expression for us to detect as worthless and eliminate. In doing so, we
clone the expression with the busted ref-kind, triggering an assertion.
Change-Id: I42fc31f6932f679ae875e2b49db2ad2f4e89e2cb
Bug: oss-fuzz:37677
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/442536
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
index 6b31f86..f145008 100644
--- a/src/sksl/SkSLAnalysis.cpp
+++ b/src/sksl/SkSLAnalysis.cpp
@@ -288,6 +288,7 @@
// If a caller doesn't care about errors, we can use this trivial reporter that just counts up.
class TrivialErrorReporter : public ErrorReporter {
public:
+ ~TrivialErrorReporter() override { this->reportPendingErrors({}); }
void handleError(const char*, PositionInfo) override {}
};
@@ -808,36 +809,18 @@
return IsAssignableVisitor{errors ? errors : &trivialErrors}.visit(expr, info);
}
-void Analysis::UpdateRefKind(Expression* expr, VariableRefKind refKind) {
- class RefKindWriter : public ProgramWriter {
- public:
- RefKindWriter(VariableReference::RefKind refKind) : fRefKind(refKind) {}
-
- bool visitExpression(Expression& expr) override {
- if (expr.is<VariableReference>()) {
- expr.as<VariableReference>().setRefKind(fRefKind);
- }
- return INHERITED::visitExpression(expr);
- }
-
- private:
- VariableReference::RefKind fRefKind;
-
- using INHERITED = ProgramWriter;
- };
-
- RefKindWriter{refKind}.visitExpression(*expr);
-}
-
-bool Analysis::MakeAssignmentExpr(Expression* expr,
- VariableReference::RefKind kind,
- ErrorReporter* errors) {
+bool Analysis::UpdateVariableRefKind(Expression* expr,
+ VariableReference::RefKind kind,
+ ErrorReporter* errors) {
Analysis::AssignmentInfo info;
if (!Analysis::IsAssignable(*expr, &info, errors)) {
return false;
}
if (!info.fAssignedVar) {
- errors->error(expr->fOffset, "can't assign to expression '" + expr->description() + "'");
+ if (errors) {
+ errors->error(expr->fOffset, "can't assign to expression '" +
+ expr->description() + "'");
+ }
return false;
}
info.fAssignedVar->setRefKind(kind);
diff --git a/src/sksl/SkSLAnalysis.h b/src/sksl/SkSLAnalysis.h
index 2e31e1f..e599abf 100644
--- a/src/sksl/SkSLAnalysis.h
+++ b/src/sksl/SkSLAnalysis.h
@@ -91,13 +91,11 @@
static bool IsAssignable(Expression& expr, AssignmentInfo* info = nullptr,
ErrorReporter* errors = nullptr);
- // Updates the `refKind` field of exactly one VariableReference inside `expr`.
- // `expr` must be `IsAssignable`; returns an error otherwise.
- static bool MakeAssignmentExpr(Expression* expr, VariableRefKind kind, ErrorReporter* errors);
-
- // Updates the `refKind` field of every VariableReference found within `expr`.
- // `expr` is allowed to have zero, one, or multiple VariableReferences.
- static void UpdateRefKind(Expression* expr, VariableRefKind refKind);
+ // Updates the `refKind` field of the VariableReference at the top level of `expr`.
+ // If `expr` can be assigned to (`IsAssignable`), true is returned and no errors are reported.
+ // If not, false is returned. and an error is reported if `errors` is non-null.
+ static bool UpdateVariableRefKind(Expression* expr, VariableRefKind kind,
+ ErrorReporter* errors = nullptr);
// A "trivial" expression is one where we'd feel comfortable cloning it multiple times in
// the code, without worrying about incurring a performance penalty. Examples:
diff --git a/src/sksl/SkSLConstantFolder.cpp b/src/sksl/SkSLConstantFolder.cpp
index 3de16af..3f7c8c3 100644
--- a/src/sksl/SkSLConstantFolder.cpp
+++ b/src/sksl/SkSLConstantFolder.cpp
@@ -293,7 +293,7 @@
case Token::Kind::TK_MINUSEQ:
if (is_constant_value(right, 0.0)) { // x += 0, x -= 0
std::unique_ptr<Expression> result = cast_expression(context, left, resultType);
- Analysis::UpdateRefKind(result.get(), VariableRefKind::kRead);
+ Analysis::UpdateVariableRefKind(result.get(), VariableRefKind::kRead);
return result;
}
break;
@@ -302,7 +302,7 @@
case Token::Kind::TK_SLASHEQ:
if (is_constant_value(right, 1.0)) { // x *= 1, x /= 1
std::unique_ptr<Expression> result = cast_expression(context, left, resultType);
- Analysis::UpdateRefKind(result.get(), VariableRefKind::kRead);
+ Analysis::UpdateVariableRefKind(result.get(), VariableRefKind::kRead);
return result;
}
break;
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index ba7f2e5..e85b8ea 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -1420,8 +1420,7 @@
if (!converted) {
return nullptr;
}
- return IndexExpression::Convert(fContext, *fSymbolTable, std::move(base),
- std::move(converted));
+ return IndexExpression::Convert(fContext, *fSymbolTable, std::move(base), std::move(converted));
}
std::unique_ptr<Expression> IRGenerator::convertCallExpression(const ASTNode& callNode) {
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index 3d64ccc..671f69d 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -162,7 +162,7 @@
std::unique_ptr<Expression> clone_with_ref_kind(const Expression& expr,
VariableReference::RefKind refKind) {
std::unique_ptr<Expression> clone = expr.clone();
- Analysis::UpdateRefKind(clone.get(), refKind);
+ Analysis::UpdateVariableRefKind(clone.get(), refKind);
return clone;
}
diff --git a/src/sksl/ir/SkSLBinaryExpression.cpp b/src/sksl/ir/SkSLBinaryExpression.cpp
index 598e1b8..bb88c5c 100644
--- a/src/sksl/ir/SkSLBinaryExpression.cpp
+++ b/src/sksl/ir/SkSLBinaryExpression.cpp
@@ -88,11 +88,11 @@
bool isAssignment = op.isAssignment();
if (isAssignment &&
- !Analysis::MakeAssignmentExpr(left.get(),
- op.kind() != Token::Kind::TK_EQ
- ? VariableReference::RefKind::kReadWrite
- : VariableReference::RefKind::kWrite,
- context.fErrors)) {
+ !Analysis::UpdateVariableRefKind(left.get(),
+ op.kind() != Token::Kind::TK_EQ
+ ? VariableReference::RefKind::kReadWrite
+ : VariableReference::RefKind::kWrite,
+ context.fErrors)) {
return nullptr;
}
diff --git a/src/sksl/ir/SkSLFunctionCall.cpp b/src/sksl/ir/SkSLFunctionCall.cpp
index 2d92043..d402a51 100644
--- a/src/sksl/ir/SkSLFunctionCall.cpp
+++ b/src/sksl/ir/SkSLFunctionCall.cpp
@@ -864,7 +864,7 @@
const VariableRefKind refKind = paramModifiers.fFlags & Modifiers::kIn_Flag
? VariableReference::RefKind::kReadWrite
: VariableReference::RefKind::kPointer;
- if (!Analysis::MakeAssignmentExpr(arguments[i].get(), refKind, context.fErrors)) {
+ if (!Analysis::UpdateVariableRefKind(arguments[i].get(), refKind, context.fErrors)) {
return nullptr;
}
}
diff --git a/src/sksl/ir/SkSLPostfixExpression.cpp b/src/sksl/ir/SkSLPostfixExpression.cpp
index 4ec9749..f4a1523 100644
--- a/src/sksl/ir/SkSLPostfixExpression.cpp
+++ b/src/sksl/ir/SkSLPostfixExpression.cpp
@@ -22,7 +22,8 @@
baseType.displayName() + "'");
return nullptr;
}
- if (!Analysis::MakeAssignmentExpr(base.get(), VariableRefKind::kReadWrite, context.fErrors)) {
+ if (!Analysis::UpdateVariableRefKind(base.get(), VariableRefKind::kReadWrite,
+ context.fErrors)) {
return nullptr;
}
return PostfixExpression::Make(context, std::move(base), op);
diff --git a/src/sksl/ir/SkSLPrefixExpression.cpp b/src/sksl/ir/SkSLPrefixExpression.cpp
index 9e88fa8..c24a567 100644
--- a/src/sksl/ir/SkSLPrefixExpression.cpp
+++ b/src/sksl/ir/SkSLPrefixExpression.cpp
@@ -157,8 +157,8 @@
baseType.displayName() + "'");
return nullptr;
}
- if (!Analysis::MakeAssignmentExpr(base.get(), VariableReference::RefKind::kReadWrite,
- context.fErrors)) {
+ if (!Analysis::UpdateVariableRefKind(base.get(), VariableReference::RefKind::kReadWrite,
+ context.fErrors)) {
return nullptr;
}
break;