[wasm][streaming] Avoid UAF after context disposal

After a call to {StreamingDecoder::NotifyCompilationEnded}, no method on
the {StreamingProcessor} should be called any more. We were still
calling the {OnAbort} method later.

To make the semantics a bit more clear, we rename
{NotifyCompilationEnded} to {NotifyCompilationDiscarded}.

We also remove the {stream_finished_} field and reset the processor
instead, which will result in a nullptr access if we try to illegally
call any further methods.

R=ahaas@chromium.org

Bug: chromium:1403531, chromium:1399790, chromium:1400066
Change-Id: I4caef3801dfe9d653125efbd7bc9b5d13ce30dc7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4132966
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#85114}

src/wasm/module-compiler.cc

@@ -2131,9 +2131,7 @@ AsyncCompileJob::~AsyncCompileJob() {
   }
   // Tell the streaming decoder that the AsyncCompileJob is not available
   // anymore.
-  // TODO(ahaas): Is this notification really necessary? Check
-  // https://crbug.com/888170.
-  if (stream_) stream_->NotifyCompilationEnded();
+  if (stream_) stream_->NotifyCompilationDiscarded();
   CancelPendingForegroundTask();
   isolate_->global_handles()->Destroy(native_context_.location());
   isolate_->global_handles()->Destroy(incumbent_context_.location());
@@ -2815,9 +2813,6 @@ 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);
 
@@ -2836,8 +2831,8 @@ void AsyncStreamingProcessor::OnFinishedStream(
       // Clean up the temporary cache entry.
       GetWasmEngine()->StreamingCompilationFailed(prefix_hash_);
     }
+    // Calling {Failed} will invalidate the {AsyncCompileJob} and delete {this}.
     job_->Failed();
-    job_ = nullptr;
     return;
   }
 
@@ -2906,14 +2901,13 @@ void AsyncStreamingProcessor::OnFinishedStream(
           failed, std::move(job_->native_module_), job_->isolate_);
       cache_hit = prev_native_module != job_->native_module_.get();
     }
+    // We finally call {Failed} or {FinishCompile}, which will invalidate the
+    // {AsyncCompileJob} and delete {this}.
     if (failed) {
       job_->Failed();
     } else {
       job_->FinishCompile(cache_hit);
     }
-    // Calling either {Failed} or {FinishCompile} will invalidate the
-    // {AsyncCompileJob}.
-    job_ = nullptr;
   }
 }
 
@@ -2927,9 +2921,8 @@ void AsyncStreamingProcessor::OnAbort() {
     // Clean up the temporary cache entry.
     GetWasmEngine()->StreamingCompilationFailed(prefix_hash_);
   }
-  // {Abort} invalidates the {AsyncCompileJob}.
+  // {Abort} invalidates the {AsyncCompileJob}, which in turn deletes {this}.
   job_->Abort();
-  job_ = nullptr;
 }
 
 bool AsyncStreamingProcessor::Deserialize(
@@ -2956,9 +2949,8 @@ bool AsyncStreamingProcessor::Deserialize(
       job_->isolate_->global_handles()->Create(*result.ToHandleChecked());
   job_->native_module_ = job_->module_object_->shared_native_module();
   job_->wire_bytes_ = ModuleWireBytes(job_->native_module_->wire_bytes());
+  // Calling {FinishCompile} deletes the {AsyncCompileJob} and {this}.
   job_->FinishCompile(false);
-  // Calling {FinishCompile} invalidates the {AsyncCompileJob}.
-  job_ = nullptr;
   return true;
 }
 

src/wasm/streaming-decoder.cc

@@ -28,16 +28,18 @@ class V8_EXPORT_PRIVATE AsyncStreamingDecoder : public StreamingDecoder {
   AsyncStreamingDecoder(const AsyncStreamingDecoder&) = delete;
   AsyncStreamingDecoder& operator=(const AsyncStreamingDecoder&) = delete;
 
-  // The buffer passed into OnBytesReceived is owned by the caller.
   void OnBytesReceived(base::Vector<const uint8_t> bytes) override;
 
   void Finish(bool can_use_compiled_module) override;
 
   void Abort() override;
 
-  // Notify the StreamingDecoder that compilation ended and the
-  // StreamingProcessor should not be called anymore.
-  void NotifyCompilationEnded() override { Fail(); }
+  void NotifyCompilationDiscarded() override {
+    auto& active_processor = processor_ ? processor_ : failed_processor_;
+    active_processor.reset();
+    DCHECK_NULL(processor_);
+    DCHECK_NULL(failed_processor_);
+  }
 
   void NotifyNativeModuleCreated(
       const std::shared_ptr<NativeModule>& native_module) override;
@@ -194,6 +196,8 @@ class V8_EXPORT_PRIVATE AsyncStreamingDecoder : public StreamingDecoder {
   }
 
   void Fail() {
+    // {Fail} cannot be called after {Finish}, {Abort}, {Fail}, or
+    // {NotifyCompilationDiscarded}.
     DCHECK_EQ(processor_ == nullptr, failed_processor_ != nullptr);
     if (processor_ != nullptr) failed_processor_ = std::move(processor_);
     DCHECK_NULL(processor_);
@@ -209,14 +213,14 @@ class V8_EXPORT_PRIVATE AsyncStreamingDecoder : public StreamingDecoder {
 
   // As long as we did not detect an invalid module, {processor_} will be set.
   // On failure, the pointer is transferred to {failed_processor_} and will only
-  // be used for a final callback once all bytes have arrived.
+  // be used for a final callback once all bytes have arrived. Finally, both
+  // {processor_} and {failed_processor_} will be null.
   std::unique_ptr<StreamingProcessor> processor_;
-  std::unique_ptr<StreamingProcessor> failed_processor_{nullptr};
+  std::unique_ptr<StreamingProcessor> failed_processor_;
   std::unique_ptr<DecodingState> state_;
   std::vector<std::shared_ptr<SectionBuffer>> section_buffers_;
   bool code_section_processed_ = false;
   uint32_t module_offset_ = 0;
-  bool stream_finished_ = false;
 
   // TODO(clemensb): Avoid holding the wire bytes live twice (here and in the
   // section buffers).
@@ -256,8 +260,9 @@ size_t AsyncStreamingDecoder::DecodingState::ReadBytes(
 
 void AsyncStreamingDecoder::Finish(bool can_use_compiled_module) {
   TRACE_STREAMING("Finish\n");
-  CHECK(!stream_finished_);
-  stream_finished_ = true;
+  // {Finish} cannot be called after {Finish}, {Abort}, {Fail}, or
+  // {NotifyCompilationDiscarded}.
+  CHECK_EQ(processor_ == nullptr, failed_processor_ != nullptr);
   if (ok() && deserializing()) {
     // Try to deserialize the module from wire bytes and module bytes.
     if (can_use_compiled_module &&
@@ -284,19 +289,22 @@ void AsyncStreamingDecoder::Finish(bool can_use_compiled_module) {
   base::OwnedVector<const uint8_t> bytes_copy =
       base::OwnedVector<uint8_t>::Of(full_wire_bytes_);
 
-  if (ok()) {
-    processor_->OnFinishedStream(std::move(bytes_copy), false);
-  } else {
-    failed_processor_->OnFinishedStream(std::move(bytes_copy), true);
-  }
+  // Calling {OnFinishedStream} calls out to JS. Avoid further callbacks (by
+  // aborting the stream) by resetting the processor field before calling
+  // {OnFinishedStream}.
+  const bool failed = !ok();
+  std::unique_ptr<StreamingProcessor> processor =
+      failed ? std::move(failed_processor_) : std::move(processor_);
+  processor->OnFinishedStream(std::move(bytes_copy), failed);
 }
 
 void AsyncStreamingDecoder::Abort() {
   TRACE_STREAMING("Abort\n");
-  if (stream_finished_) return;
-  stream_finished_ = true;
+  // Ignore {Abort} after {Finish}.
+  if (!processor_ && !failed_processor_) return;
   Fail();
   failed_processor_->OnAbort();
+  failed_processor_.reset();
 }
 
 namespace {

src/wasm/streaming-decoder.h

@@ -79,9 +79,9 @@ class V8_EXPORT_PRIVATE StreamingDecoder {
 
   virtual void Abort() = 0;
 
-  // Notify the StreamingDecoder that compilation ended and the
+  // Notify the StreamingDecoder that the job was discarded and the
   // StreamingProcessor should not be called anymore.
-  virtual void NotifyCompilationEnded() = 0;
+  virtual void NotifyCompilationDiscarded() = 0;
 
   // Caching support.
   // Sets the callback that is called after a new chunk of the module is tiered

src/wasm/sync-streaming-decoder.cc

@@ -77,7 +77,7 @@ class V8_EXPORT_PRIVATE SyncStreamingDecoder : public StreamingDecoder {
     buffer_.clear();
   }
 
-  void NotifyCompilationEnded() override { buffer_.clear(); }
+  void NotifyCompilationDiscarded() override { buffer_.clear(); }
 
   void NotifyNativeModuleCreated(
       const std::shared_ptr<NativeModule>&) override {

src/wasm/wasm-js.cc

@@ -3181,6 +3181,19 @@ void WasmJs::InstallConditionalFeatures(Isolate* isolate,
                                         Handle<Context> context) {
   // This space left blank for future origin trials.
 }
+
+namespace wasm {
+// static
+std::unique_ptr<WasmStreaming> StartStreamingForTesting(
+    Isolate* isolate,
+    std::shared_ptr<wasm::CompilationResultResolver> resolver) {
+  return std::make_unique<WasmStreaming>(
+      std::make_unique<WasmStreaming::WasmStreamingImpl>(
+          reinterpret_cast<v8::Isolate*>(isolate), "StartStreamingForTesting",
+          resolver));
+}
+}  // namespace wasm
+
 #undef ASSIGN
 #undef EXTRACT_THIS
 

src/wasm/wasm-js.h

@@ -9,16 +9,25 @@
 #ifndef V8_WASM_WASM_JS_H_
 #define V8_WASM_WASM_JS_H_
 
+#include <memory>
+
 #include "src/common/globals.h"
 
 namespace v8 {
-namespace internal {
+class WasmStreaming;
+}  // namespace v8
+
+namespace v8::internal {
 class Context;
 template <typename T>
 class Handle;
 
 namespace wasm {
+class CompilationResultResolver;
 class StreamingDecoder;
+
+V8_EXPORT_PRIVATE std::unique_ptr<WasmStreaming> StartStreamingForTesting(
+    Isolate*, std::shared_ptr<wasm::CompilationResultResolver>);
 }  // namespace wasm
 
 // Exposes a WebAssembly API to JavaScript through the V8 API.
@@ -31,7 +40,6 @@ class WasmJs {
       Isolate* isolate, Handle<Context> context);
 };
 
-}  // namespace internal
-}  // namespace v8
+}  // namespace v8::internal
 
 #endif  // V8_WASM_WASM_JS_H_

test/cctest/cctest.status

@@ -519,9 +519,7 @@
 # TODO(v8:7777): Change this once wasm is supported in jitless mode.
 ['not has_webassembly or variant == jitless', {
   'test-api/TurboAsmDisablesDetach': [SKIP],
-  'test-api/WasmI32AtomicWaitCallback': [SKIP],
-  'test-api/WasmI64AtomicWaitCallback': [SKIP],
-  'test-api/WasmSetJitCodeEventHandler': [SKIP],
+  'test-api/Wasm*': [SKIP],
   'test-api-array-buffer/ArrayBuffer_NonDetachableWasDetached': [SKIP],
   'test-backing-store/Run_WasmModule_Buffer_Externalized_Regression_UseAfterFree': [SKIP],
   'test-c-wasm-entry/*': [SKIP],

test/cctest/test-api.cc

@@ -80,6 +80,7 @@
 #include "test/common/flag-utils.h"
 
 #if V8_ENABLE_WEBASSEMBLY
+#include "src/wasm/wasm-engine.h"
 #include "test/cctest/wasm/wasm-run-utils.h"
 #include "test/common/wasm/test-signatures.h"
 #include "test/common/wasm/wasm-macro-gen.h"
@@ -22993,8 +22994,7 @@ void SourceURLHelper(v8::Isolate* isolate, const char* source_text,
                             Local<Value>(),       // source map URL
                             false,                // is opaque
                             false,                // is WASM
-                            true                  // is ES Module
-    );
+                            true);                // is ES Module
     v8::ScriptCompiler::Source source(source_str, origin, nullptr);
 
     Local<v8::Module> module =
@@ -29500,3 +29500,37 @@ UNINITIALIZED_TEST(OOMDetailsAreMovableAndCopyable) {
 UNINITIALIZED_TEST(JitCodeEventIsMovableAndCopyable) {
   TestCopyAndMoveConstructionAndAssignment<v8::JitCodeEvent>();
 }
+
+#if V8_ENABLE_WEBASSEMBLY
+TEST(WasmAbortStreamingAfterContextDisposal) {
+  // This is a regression test for https://crbug.com/1403531.
+
+  class Resolver final : public i::wasm::CompilationResultResolver {
+   public:
+    void OnCompilationSucceeded(
+        i::Handle<i::WasmModuleObject> result) override {
+      UNREACHABLE();
+    }
+    void OnCompilationFailed(i::Handle<i::Object> error_reason) override {
+      UNREACHABLE();
+    }
+  };
+
+  auto resolver = std::make_shared<Resolver>();
+
+  std::unique_ptr<v8::WasmStreaming> wasm_streaming;
+  v8::Isolate* isolate = CcTest::isolate();
+  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  {
+    v8::HandleScope scope(isolate);
+    LocalContext context;
+
+    wasm_streaming =
+        i::wasm::StartStreamingForTesting(i_isolate, std::move(resolver));
+    isolate->ContextDisposedNotification(false);
+  }
+
+  wasm_streaming->Abort({});
+  wasm_streaming.reset();
+}
+#endif  // V8_ENABLE_WEBASSEMBLY