From 9b3e66263b95fb47e0d226972c3fc7c4141fb742 Mon Sep 17 00:00:00 2001
From: Clemens Backes <clemensb@chromium.org>
Date: Thu, 15 Dec 2022 18:34:27 +0100
Subject: [PATCH] [wasm] Add more checks around streaming compilation

We see failures in the wild when finishing streaming wasm compilation.
This CL adds CHECKs that we do not accidentally finish or abort a job
multiple times, since many methods actually delete the
{AsyncCompileJob}, so calling such methods twice would lead to a
use-after-free.

R=jkummerow@chromium.org

Bug: chromium:1399790, chromium:1400066
Change-Id: I0b83b1e402444afd4444638d5c9a2fd31a78b056
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4110762
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84905}
---
 src/wasm/module-compiler.cc   | 13 ++++++++++++-
 src/wasm/streaming-decoder.cc |  2 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc
index 4fb4d71aa6..03f5decbf1 100644
--- a/src/wasm/module-compiler.cc
+++ b/src/wasm/module-compiler.cc
@@ -2292,7 +2292,7 @@ void AsyncCompileJob::FinishCompile(bool is_after_cache_hit) {
 
 void AsyncCompileJob::Failed() {
   // {job} keeps the {this} pointer alive.
-  std::shared_ptr<AsyncCompileJob> job =
+  std::unique_ptr<AsyncCompileJob> job =
       GetWasmEngine()->RemoveCompileJob(this);
 
   // Revalidate the whole module to produce a deterministic error message.
@@ -2837,6 +2837,9 @@ void AsyncStreamingProcessor::OnFinishedStream(
     if (validate_functions_job_data_.found_error) after_error = true;
   }
 
+  // Check that we did not abort or finish the job before.
+  CHECK(job_);
+
   job_->wire_bytes_ = ModuleWireBytes(bytes.as_vector());
   job_->bytes_copy_ = std::move(bytes);
 
@@ -2856,6 +2859,7 @@ void AsyncStreamingProcessor::OnFinishedStream(
       GetWasmEngine()->StreamingCompilationFailed(prefix_hash_);
     }
     job_->Failed();
+    job_ = nullptr;
     return;
   }
 
@@ -2929,6 +2933,9 @@ void AsyncStreamingProcessor::OnFinishedStream(
     } else {
       job_->FinishCompile(cache_hit);
     }
+    // Calling either {Failed} or {FinishCompile} will invalidate the
+    // {AsyncCompileJob}.
+    job_ = nullptr;
   }
 }
 
@@ -2942,7 +2949,9 @@ void AsyncStreamingProcessor::OnAbort() {
     // Clean up the temporary cache entry.
     GetWasmEngine()->StreamingCompilationFailed(prefix_hash_);
   }
+  // {Abort} invalidates the {AsyncCompileJob}.
   job_->Abort();
+  job_ = nullptr;
 }
 
 bool AsyncStreamingProcessor::Deserialize(
@@ -2970,6 +2979,8 @@ bool AsyncStreamingProcessor::Deserialize(
   job_->native_module_ = job_->module_object_->shared_native_module();
   job_->wire_bytes_ = ModuleWireBytes(job_->native_module_->wire_bytes());
   job_->FinishCompile(false);
+  // Calling {FinishCompile} invalidates the {AsyncCompileJob}.
+  job_ = nullptr;
   return true;
 }
 
diff --git a/src/wasm/streaming-decoder.cc b/src/wasm/streaming-decoder.cc
index 18126a6d96..218c91698c 100644
--- a/src/wasm/streaming-decoder.cc
+++ b/src/wasm/streaming-decoder.cc
@@ -256,7 +256,7 @@ size_t AsyncStreamingDecoder::DecodingState::ReadBytes(
 
 void AsyncStreamingDecoder::Finish(bool can_use_compiled_module) {
   TRACE_STREAMING("Finish\n");
-  DCHECK(!stream_finished_);
+  CHECK(!stream_finished_);
   stream_finished_ = true;
   if (ok() && deserializing()) {
     // Try to deserialize the module from wire bytes and module bytes.