Fix use-after-free error discovered by the fuzzer.

When eliminating a CFG node, we now flag its exit nodes; if our
optimization pass reaches one of those flagged nodes, we stop the
current optimization process in its tracks and initiate a rescan.

We do NOT recursively mark the exits of the exit nodes, so this fix is
reliant on the CFG being ordered in a non-chaotic fashion, but in
practice this seems to be sufficient for the CFGs we generate today.

Change-Id: I892805361c5f4297e02146f37a759dfda83f5488
Bug: oss-fuzz:26942
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/331597
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

src/sksl/SkSLCFGGenerator.cpp

@@ -58,11 +58,11 @@ BlockId CFG::newIsolatedBlock() {
 }
 
 void CFG::addExit(BlockId from, BlockId to) {
+    BasicBlock::ExitArray& exits = fBlocks[from].fExits;
+    if (std::find(exits.begin(), exits.end(), to) == exits.end()) {
+        exits.push_back(to);
+    }
     if (from == 0 || fBlocks[from].fIsReachable) {
-        BasicBlock::ExitArray& exits = fBlocks[from].fExits;
-        if (std::find(exits.begin(), exits.end(), to) == exits.end()) {
-            exits.push_back(to);
-        }
         fBlocks[to].fIsReachable = true;
     }
 }

src/sksl/SkSLCompiler.cpp

@@ -1456,6 +1456,7 @@ bool Compiler::scanCFG(FunctionDefinition& f, ProgramUsage* usage) {
     // check for dead code & undefined variables, perform constant propagation
     OptimizationContext optimizationContext;
     optimizationContext.fUsage = usage;
+    SkBitSet eliminatedBlockIds(cfg.fBlocks.size());
     do {
         if (optimizationContext.fNeedsRescan) {
             cfg = CFGGenerator().getCFG(f);
@@ -1463,23 +1464,37 @@ bool Compiler::scanCFG(FunctionDefinition& f, ProgramUsage* usage) {
             optimizationContext.fNeedsRescan = false;
         }
 
+        eliminatedBlockIds.reset();
         optimizationContext.fUpdated = false;
-        bool first = true;
-        for (BasicBlock& b : cfg.fBlocks) {
-            if (!first && !b.fIsReachable) {
+
+        for (BlockId blockId = 0; blockId < cfg.fBlocks.size(); ++blockId) {
+            if (eliminatedBlockIds.test(blockId)) {
+                // We reached a block ID that might have been eliminated. Be cautious and rescan.
+                optimizationContext.fUpdated = true;
+                optimizationContext.fNeedsRescan = true;
+                break;
+            }
+
+            BasicBlock& b = cfg.fBlocks[blockId];
+            if (blockId > 0 && !b.fIsReachable) {
                 // Block was reachable before optimization, but has since become unreachable. In
                 // addition to being dead code, it's broken - since control flow can't reach it, no
                 // prior variable definitions can reach it, and therefore variables might look to
                 // have not been properly assigned. Kill it by replacing all statements with Nops.
                 for (BasicBlock::Node& node : b.fNodes) {
                     if (node.isStatement() && !(*node.statement())->is<Nop>()) {
+                        // Eliminating a node runs the risk of eliminating that node's exits as
+                        // well. Keep track of this and do a rescan if we are about to access one
+                        // of these.
+                        for (BlockId id : b.fExits) {
+                            eliminatedBlockIds.set(id);
+                        }
                         node.setStatement(std::make_unique<Nop>(), usage);
                         madeChanges = true;
                     }
                 }
                 continue;
             }
-            first = false;
             DefinitionMap definitions = b.fBefore;
 
             for (auto iter = b.fNodes.begin(); iter != b.fNodes.end() &&

src/utils/SkBitSet.h

@@ -12,6 +12,7 @@
 #include "include/private/SkTemplates.h"
 #include "src/core/SkMathPriv.h"
 #include <climits>
+#include <cstring>
 #include <limits>
 #include <memory>
 
@@ -42,12 +43,26 @@ public:
         *this->chunkFor(index) |= ChunkMaskFor(index);
     }
 
-    /** Set the value of the index-th bit to false.  */
+    /** Sets every bit in the bitset to true. */
+    void set() {
+        Chunk* chunks = fChunks.get();
+        const size_t numChunks = NumChunksFor(fSize);
+        std::memset(chunks, 0xFF, sizeof(Chunk) * numChunks);
+    }
+
+    /** Set the value of the index-th bit to false. */
     void reset(size_t index) {
         SkASSERT(index < fSize);
         *this->chunkFor(index) &= ~ChunkMaskFor(index);
     }
 
+    /** Sets every bit in the bitset to false. */
+    void reset() {
+        Chunk* chunks = fChunks.get();
+        const size_t numChunks = NumChunksFor(fSize);
+        std::memset(chunks, 0, sizeof(Chunk) * numChunks);
+    }
+
     bool test(size_t index) const {
         SkASSERT(index < fSize);
         return SkToBool(*this->chunkFor(index) & ChunkMaskFor(index));

tests/BitSetTest.cpp

@@ -51,4 +51,12 @@ DEF_TEST(BitSet, reporter) {
     REPORTER_ASSERT(reporter, set1.test(12345) == true);
     REPORTER_ASSERT(reporter, set1.test(22) == false);
     REPORTER_ASSERT(reporter, set0.test(35) == true);
+
+    set0.reset();
+    REPORTER_ASSERT(reporter, !set0.findFirst());
+    REPORTER_ASSERT(reporter, set0.test(1234) == false);
+
+    set0.set();
+    REPORTER_ASSERT(reporter, !set0.findFirstUnset());
+    REPORTER_ASSERT(reporter, set0.test(5678) == true);
 }

tests/sksl/shared/golden/DeadIfStatement.glsl

@@ -1,3 +1,4 @@
-### Compilation failed:
-
 
+void main() {
+    return;
+}

tests/sksl/shared/golden/DeadIfStatement.metal

@@ -1,3 +1,14 @@
-### Compilation failed:
-
-
+#include <metal_stdlib>
+#include <simd/simd.h>
+using namespace metal;
+struct Inputs {
+};
+struct Outputs {
+    float4 sk_FragColor [[color(0)]];
+};
+fragment Outputs fragmentMain(Inputs _in [[stage_in]], bool _frontFacing [[front_facing]], float4 _fragCoord [[position]]) {
+    Outputs _outputStruct;
+    thread Outputs* _out = &_outputStruct;
+    return;
+    return *_out;
+}