Detach VarDeclarations and Variables from each other during deletion.
Variable and VarDeclarations cross-reference one another. They generally
get deleted around the same time, but this is not always the case. In
this CL we explicitly detach them from each other at destruction time
to avoid holding a stale pointer, removing the risk of accessing it
later. (Accessing null is still fatal, of course, but it's less
dangerous than using a recently-freed pointer, and easier to debug as
well.)
Modifying things in the symbol table requires a const_cast, but it's not
too risky to null out a pointer field on a conceptually-dead object.
A Variable without a VarDeclaration is inert.
Change-Id: Ie01244495a82a8007269522a561b2512c5f12384
Bug: oss-fuzz:32587
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/390056
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/SkSLConstantFolder.cpp b/src/sksl/SkSLConstantFolder.cpp
index 8bd721d..bd760c5 100644
--- a/src/sksl/SkSLConstantFolder.cpp
+++ b/src/sksl/SkSLConstantFolder.cpp
@@ -218,7 +218,11 @@
break;
}
expr = var.initialValue();
- SkASSERT(expr);
+ if (!expr) {
+ SkDEBUGFAILF("found a const variable without an initial value (%s)",
+ var.description().c_str());
+ break;
+ }
if (expr->isCompileTimeConstant()) {
return expr;
}
diff --git a/src/sksl/SkSLDehydrator.cpp b/src/sksl/SkSLDehydrator.cpp
index a1da56f..88b9c0e 100644
--- a/src/sksl/SkSLDehydrator.cpp
+++ b/src/sksl/SkSLDehydrator.cpp
@@ -495,8 +495,7 @@
this->write(en.typeName());
AutoDehydratorSymbolTable symbols(this, en.symbols());
for (const std::unique_ptr<const Symbol>& s : en.symbols()->fOwnedSymbols) {
- SkASSERT(s->kind() == Symbol::Kind::kVariable);
- Variable& v = (Variable&) *s;
+ const Variable& v = s->as<Variable>();
SkASSERT(v.initialValue());
const IntLiteral& i = v.initialValue()->as<IntLiteral>();
this->writeS32(i.value());
diff --git a/src/sksl/ir/SkSLVarDeclarations.h b/src/sksl/ir/SkSLVarDeclarations.h
index 6e263b9..aba1869 100644
--- a/src/sksl/ir/SkSLVarDeclarations.h
+++ b/src/sksl/ir/SkSLVarDeclarations.h
@@ -38,6 +38,13 @@
, fArraySize(arraySize)
, fValue(std::move(value)) {}
+ ~VarDeclaration() override {
+ // Unhook this VarDeclaration from its associated Variable, since we're being deleted.
+ if (fVar) {
+ fVar->detachDeadVarDeclaration();
+ }
+ }
+
// Does proper error checking and type coercion; reports errors via ErrorReporter.
static std::unique_ptr<Statement> Convert(const Context& context,
Variable* var,
@@ -54,6 +61,8 @@
}
const Variable& var() const {
+ // This should never be called after the Variable has been deleted.
+ SkASSERT(fVar);
return *fVar;
}
diff --git a/src/sksl/ir/SkSLVariable.cpp b/src/sksl/ir/SkSLVariable.cpp
index 50c2316..bf354b4 100644
--- a/src/sksl/ir/SkSLVariable.cpp
+++ b/src/sksl/ir/SkSLVariable.cpp
@@ -11,6 +11,13 @@
namespace SkSL {
+Variable::~Variable() {
+ // Unhook this Variable from its associated VarDeclaration, since we're being deleted.
+ if (fDeclaration) {
+ fDeclaration->setVar(nullptr);
+ }
+}
+
const Expression* Variable::initialValue() const {
return fDeclaration ? fDeclaration->value().get() : nullptr;
}
diff --git a/src/sksl/ir/SkSLVariable.h b/src/sksl/ir/SkSLVariable.h
index dc9f10a..a4caeed 100644
--- a/src/sksl/ir/SkSLVariable.h
+++ b/src/sksl/ir/SkSLVariable.h
@@ -48,6 +48,8 @@
, fStorage(storage)
, fBuiltin(builtin) {}
+ ~Variable() override;
+
const Modifiers& modifiers() const {
return *fModifiers;
}
@@ -67,6 +69,12 @@
fDeclaration = declaration;
}
+ void detachDeadVarDeclaration() const {
+ // The VarDeclaration is being deleted, so our reference to it has become stale.
+ // This variable is now dead, so it shouldn't matter that we are modifying its symbol.
+ const_cast<Variable*>(this)->fDeclaration = nullptr;
+ }
+
String description() const override {
return this->modifiers().description() + this->type().name() + " " + this->name();
}