commit 68dd2c1fa051019354d0c441c78b3885d8e72a7a
author Ethan Nicholas <ethannicholas@google.com> Thu Mar 01 15:05:17 2018 -0500
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Thu Mar 01 20:42:04 2018 +0000
tree 9b518d216d71742c1948c574db089bdc627b060e
parent a7f320507dcf765313e27001774042cf1882dfea

Fixed SkSL use-after-free fuzzer bug and added defensive code to catch such problems in the future.

Bug: skia:7558
Change-Id: I5098c0ed08f2328828969e819db7785270b26656
Reviewed-on: https://skia-review.googlesource.com/111460
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>

src/sksl/SkSLIRGenerator.cpp

diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 719dca9..38c45f7 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -286,6 +286,9 @@
                 return nullptr;
             }
             value = this->coerce(std::move(value), *type);
+            if (!value) {
+                return nullptr;
+            }
             var->fWriteCount = 1;
             var->fInitialValue = value.get();
         }
@@ -761,7 +764,8 @@
 
 std::unique_ptr<InterfaceBlock> IRGenerator::convertInterfaceBlock(const ASTInterfaceBlock& intf) {
     std::shared_ptr<SymbolTable> old = fSymbolTable;
-    AutoSymbolTable table(this);
+    this->pushSymbolTable();
+    std::shared_ptr<SymbolTable> symbols = fSymbolTable;
     std::vector<Type::Field> fields;
     bool haveRuntimeArray = false;
     bool foundRTAdjust = false;
@@ -804,6 +808,7 @@
             }
         }
     }
+    this->popSymbolTable();
     Type* type = new Type(intf.fOffset, intf.fTypeName, fields);
     old->takeOwnership(type);
     std::vector<std::unique_ptr<Expression>> sizes;
@@ -826,11 +831,11 @@
                 name += "[]";
             }
             type = new Type(name, Type::kArray_Kind, *type, (int) count);
-            fSymbolTable->takeOwnership((Type*) type);
+            symbols->takeOwnership((Type*) type);
             sizes.push_back(std::move(converted));
         } else {
             type = new Type(type->name() + "[]", Type::kArray_Kind, *type, -1);
-            fSymbolTable->takeOwnership((Type*) type);
+            symbols->takeOwnership((Type*) type);
             sizes.push_back(nullptr);
         }
     }
@@ -858,7 +863,7 @@
                                                               intf.fTypeName,
                                                               intf.fInstanceName,
                                                               std::move(sizes),
-                                                              fSymbolTable));
+                                                              symbols));
 }
 
 void IRGenerator::getConstantInt(const Expression& value, int64_t* out) {

src/sksl/ir/SkSLSymbol.h

diff --git a/src/sksl/ir/SkSLSymbol.h b/src/sksl/ir/SkSLSymbol.h
index 4ec8f15..43fb742 100644
--- a/src/sksl/ir/SkSLSymbol.h
+++ b/src/sksl/ir/SkSLSymbol.h
@@ -29,6 +29,8 @@
     , fKind(kind)
     , fName(name) {}
 
+    virtual ~Symbol() {}
+
     Kind fKind;
     StringFragment fName;
 

src/sksl/ir/SkSLVariable.h

diff --git a/src/sksl/ir/SkSLVariable.h b/src/sksl/ir/SkSLVariable.h
index f16bf4b..b7cef10 100644
--- a/src/sksl/ir/SkSLVariable.h
+++ b/src/sksl/ir/SkSLVariable.h
@@ -37,7 +37,15 @@
     , fStorage(storage)
     , fInitialValue(initialValue)
     , fReadCount(0)
-    , fWriteCount(0) {}
+    , fWriteCount(initialValue ? 1 : 0) {}
+
+    ~Variable() override {
+        // can't destroy a variable while there are remaining references to it
+        if (fInitialValue) {
+            --fWriteCount;
+        }
+        ASSERT(!fReadCount && !fWriteCount);
+    }
 
     virtual String description() const override {
         return fModifiers.description() + fType.fName + " " + fName;

src/sksl/ir/SkSLVariableReference.h

diff --git a/src/sksl/ir/SkSLVariableReference.h b/src/sksl/ir/SkSLVariableReference.h
index ad54d43..e1f19ac 100644
--- a/src/sksl/ir/SkSLVariableReference.h
+++ b/src/sksl/ir/SkSLVariableReference.h
@@ -45,6 +45,9 @@
     }
 
     ~VariableReference() override {
+        if (fRefKind != kRead_RefKind) {
+            fVariable.fWriteCount--;
+        }
         if (fRefKind != kWrite_RefKind) {
             fVariable.fReadCount--;
         }

tests/SkSLErrorTest.cpp

diff --git a/tests/SkSLErrorTest.cpp b/tests/SkSLErrorTest.cpp
index 575fd74..b8de004 100644
--- a/tests/SkSLErrorTest.cpp
+++ b/tests/SkSLErrorTest.cpp
@@ -489,4 +489,12 @@
                  "error: 1: static switch contains non-static conditional break\n1 error\n");
 }
 
+DEF_TEST(SkSLInterfaceBlockScope, r) {
+    test_failure(r,
+                 "uniform testBlock {"
+                 "float x;"
+                 "} test[x];",
+                 "error: 1: unknown identifier 'x'\n1 error\n");
+}
+
 #endif

tests/SkSLSPIRVTest.cpp

diff --git a/tests/SkSLSPIRVTest.cpp b/tests/SkSLSPIRVTest.cpp
index ba4697d..f688d50 100644
--- a/tests/SkSLSPIRVTest.cpp
+++ b/tests/SkSLSPIRVTest.cpp
@@ -32,10 +32,12 @@
 
 DEF_TEST(SkSLBadOffset, r) {
     test_failure(r,
-                 "struct Bad { layout (offset = 5) int x; } bad; void main() { bad.x = 5; }",
+                 "struct Bad { layout (offset = 5) int x; } bad; void main() { bad.x = 5; "
+                 "sk_FragColor.r = float(bad.x); }",
                  "error: 1: offset of field 'x' must be a multiple of 4\n1 error\n");
     test_failure(r,
-                 "struct Bad { int x; layout (offset = 0) int y; } bad; void main() { bad.x = 5; }",
+                 "struct Bad { int x; layout (offset = 0) int y; } bad; void main() { bad.x = 5; "
+                 "sk_FragColor.r = float(bad.x); }",
                  "error: 1: offset of field 'y' must be at least 4\n1 error\n");
 }