From 8750f45fe7f69c8d87cef9589a2bb0bbddc921d3 Mon Sep 17 00:00:00 2001 From: Kirtan Date: Mon, 28 Sep 2026 14:11:31 +0530 Subject: [PATCH 1/2] deps: V8: cherry-pick 68210d500a Original commit message: [wasm] Add CHECKs for wrapper refcount We're seeing some crash reports that could be explained by "zombie" wrappers (freed by one code path while still in use by another, which then turns into UAF). To confirm or reject this hypothesis and shed some light on *which* code path might free wrappers too eagerly, this patch adds a few CHECKs. Bug: 407003348 Change-Id: Ic2b527acafdd57aadd51703041c1ab65f7bf7b2c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6415929 Auto-Submit: Jakob Kummerow Commit-Queue: Jakob Kummerow Reviewed-by: Matthias Liedtke Commit-Queue: Matthias Liedtke Cr-Commit-Position: refs/heads/main@{#99572} Refs: https://github.com/v8/v8/commit/68210d500a82c84db83000654be83904e93bdba4 --- common.gypi | 2 +- deps/v8/src/wasm/wasm-code-manager.cc | 3 +++ deps/v8/src/wasm/wasm-code-manager.h | 1 + deps/v8/src/wasm/wasm-engine.cc | 6 ------ deps/v8/src/wasm/wasm-import-wrapper-cache.cc | 3 +++ 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/common.gypi b/common.gypi index 1effebe59bd4..59eb8582cb69 100644 --- a/common.gypi +++ b/common.gypi @@ -42,7 +42,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.53', + 'v8_embedder_string': '-node.54', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/wasm/wasm-code-manager.cc b/deps/v8/src/wasm/wasm-code-manager.cc index 489a30d7aed5..71dbdcd7c92a 100644 --- a/deps/v8/src/wasm/wasm-code-manager.cc +++ b/deps/v8/src/wasm/wasm-code-manager.cc @@ -2776,6 +2776,9 @@ void NativeModule::FreeCode(base::Vector codes) { // Free the {WasmCode} objects. This will also unregister trap handler data. for (WasmCode* code : codes) { DCHECK_EQ(1, owned_code_.count(code->instruction_start())); + // TODO(407003348): Drop these checks if they don't trigger in the wild. + CHECK(code->is_dying()); + CHECK_EQ(code->ref_count_.load(std::memory_order_acquire), 0); owned_code_.erase(code->instruction_start()); } // Remove debug side tables for all removed code objects, after releasing our diff --git a/deps/v8/src/wasm/wasm-code-manager.h b/deps/v8/src/wasm/wasm-code-manager.h index 49258f516d22..d775fa7f37b1 100644 --- a/deps/v8/src/wasm/wasm-code-manager.h +++ b/deps/v8/src/wasm/wasm-code-manager.h @@ -293,6 +293,7 @@ class V8_EXPORT_PRIVATE WasmCode final { // loop to evaluate again what needs to be done. undo_mark_as_dying(); } + DCHECK_LT(1, old_count); if (ref_count_.compare_exchange_weak(old_count, old_count - 1, std::memory_order_acq_rel)) { return false; diff --git a/deps/v8/src/wasm/wasm-engine.cc b/deps/v8/src/wasm/wasm-engine.cc index 7724979eb01b..f7a0e392a59d 100644 --- a/deps/v8/src/wasm/wasm-engine.cc +++ b/deps/v8/src/wasm/wasm-engine.cc @@ -1890,17 +1890,11 @@ void WasmEngine::FreeDeadCodeLocked(const DeadCodeMap& dead_code, const std::vector& code_vec = dead_code_entry.second; TRACE_CODE_GC("Freeing %zu code object%s of module %p.\n", code_vec.size(), code_vec.size() == 1 ? "" : "s", native_module); -#if DEBUG - for (WasmCode* code : code_vec) DCHECK(code->is_dying()); -#endif // DEBUG native_module->FreeCode(base::VectorOf(code_vec)); } if (dead_wrappers.size()) { TRACE_CODE_GC("Freeing %zu wrapper%s.\n", dead_wrappers.size(), dead_wrappers.size() == 1 ? "" : "s"); -#if DEBUG - for (WasmCode* code : dead_wrappers) DCHECK(code->is_dying()); -#endif // DEBUG GetWasmImportWrapperCache()->Free(dead_wrappers); } } diff --git a/deps/v8/src/wasm/wasm-import-wrapper-cache.cc b/deps/v8/src/wasm/wasm-import-wrapper-cache.cc index 447195d1a616..c1cdbc384374 100644 --- a/deps/v8/src/wasm/wasm-import-wrapper-cache.cc +++ b/deps/v8/src/wasm/wasm-import-wrapper-cache.cc @@ -218,6 +218,9 @@ void WasmImportWrapperCache::Free(std::vector& wrappers) { } code_allocator_->FreeCode(base::VectorOf(wrappers)); for (WasmCode* wrapper : wrappers) { + // TODO(407003348): Drop these checks if they don't trigger in the wild. + CHECK(wrapper->is_dying()); + CHECK_EQ(wrapper->ref_count_.load(std::memory_order_acquire), 0); delete wrapper; } // Make sure nobody tries to access stale pointers. From 0385bee44f3d7c31a1523394d5a324f101bd7b7f Mon Sep 17 00:00:00 2001 From: Kirtan Date: Mon, 28 Sep 2026 14:13:21 +0530 Subject: [PATCH 2/2] deps: V8: cherry-pick 9b8ca54d5a Original commit message: [wasm] Fix lookup of wrappers marked "is_dying" We must not add a dying wrapper to the current WasmCodeRefScope. Doing so creates a possibility that a wrapper that's about to be deleted by another thread (after going through a Wasm Code GC cycle) will be deleted again by that WasmCodeRefScope. To fix this, this patch merges the "is_dying" bit and the refcount into a single bit field that can be updated atomically, and updates the WasmImportWrapperCache to use the new, more careful machinery. This definitely fixes issue 409379692, hopefully also 407003348. Bug: 407003348 Change-Id: Ib83e9df034e8ddf3590828aea86741779ef92ab9 Fixed: 409379692 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6450127 Reviewed-by: Matthias Liedtke Auto-Submit: Jakob Kummerow Commit-Queue: Jakob Kummerow Cr-Commit-Position: refs/heads/main@{#99765} Refs: https://github.com/v8/v8/commit/9b8ca54d5a6b6c615eb7ad6d9e2bdca7f8cb1a81 --- common.gypi | 2 +- deps/v8/src/wasm/wasm-code-manager.cc | 17 +++- deps/v8/src/wasm/wasm-code-manager.h | 89 +++++++++++-------- deps/v8/src/wasm/wasm-import-wrapper-cache.cc | 18 ++-- 4 files changed, 76 insertions(+), 50 deletions(-) diff --git a/common.gypi b/common.gypi index 59eb8582cb69..46ae0a4d4b93 100644 --- a/common.gypi +++ b/common.gypi @@ -42,7 +42,7 @@ # Reset this number to 0 on major V8 upgrades. # Increment by one for each non-official patch applied to deps/v8. - 'v8_embedder_string': '-node.54', + 'v8_embedder_string': '-node.55', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/wasm/wasm-code-manager.cc b/deps/v8/src/wasm/wasm-code-manager.cc index 71dbdcd7c92a..249408d5c170 100644 --- a/deps/v8/src/wasm/wasm-code-manager.cc +++ b/deps/v8/src/wasm/wasm-code-manager.cc @@ -2776,9 +2776,10 @@ void NativeModule::FreeCode(base::Vector codes) { // Free the {WasmCode} objects. This will also unregister trap handler data. for (WasmCode* code : codes) { DCHECK_EQ(1, owned_code_.count(code->instruction_start())); - // TODO(407003348): Drop these checks if they don't trigger in the wild. - CHECK(code->is_dying()); - CHECK_EQ(code->ref_count_.load(std::memory_order_acquire), 0); + // TODO(407003348): Drop this check if it doesn't trigger in the wild. + CHECK_EQ(WasmCode::refcount( + code->ref_count_bitfield_.load(std::memory_order_acquire)), + 0); owned_code_.erase(code->instruction_start()); } // Remove debug side tables for all removed code objects, after releasing our @@ -2998,6 +2999,16 @@ void WasmCodeRefScope::AddRef(WasmCode* code) { code->IncRef(); } +// static +WasmCode* WasmCodeRefScope::AddRefIfNotDying(WasmCode* code) { + DCHECK_NOT_NULL(code); + WasmCodeRefScope* current_scope = current_code_refs_scope; + DCHECK_NOT_NULL(current_scope); + if (!code->IncRefIfNotDying()) return nullptr; + current_scope->code_ptrs_.push_back(code); + return code; +} + void WasmCodeLookupCache::Flush() { for (int i = 0; i < kWasmCodeLookupCacheSize; i++) cache_[i].pc.store(kNullAddress, std::memory_order_release); diff --git a/deps/v8/src/wasm/wasm-code-manager.h b/deps/v8/src/wasm/wasm-code-manager.h index d775fa7f37b1..38a3cc28f4e1 100644 --- a/deps/v8/src/wasm/wasm-code-manager.h +++ b/deps/v8/src/wasm/wasm-code-manager.h @@ -261,41 +261,51 @@ class V8_EXPORT_PRIVATE WasmCode final { ~WasmCode(); void IncRef() { - [[maybe_unused]] int old_val = - ref_count_.fetch_add(1, std::memory_order_acq_rel); - DCHECK_LE(1, old_val); - DCHECK_GT(kMaxInt, old_val); + [[maybe_unused]] uint32_t old_field = + ref_count_bitfield_.fetch_add(1, std::memory_order_acq_rel); + DCHECK_LE(1, refcount(old_field)); + DCHECK_GT(kMaxInt, refcount(old_field)); + } + + // Returns true if the refcount was incremented, false if {this->is_dying()}. + bool IncRefIfNotDying() { + uint32_t old_field = ref_count_bitfield_.load(std::memory_order_acquire); + while (true) { + if (is_dying(old_field)) return false; + if (ref_count_bitfield_.compare_exchange_weak( + old_field, old_field + 1, std::memory_order_acq_rel)) { + return true; + } + } } // Decrement the ref count. Returns whether this code becomes dead and needs // to be freed. V8_WARN_UNUSED_RESULT bool DecRef() { - int old_count = ref_count_.load(std::memory_order_acquire); + uint32_t old_field = ref_count_bitfield_.load(std::memory_order_acquire); while (true) { - DCHECK_LE(1, old_count); - if (V8_UNLIKELY(old_count == 1)) { - if (is_dying()) { + DCHECK_LE(1, refcount(old_field)); + if (V8_UNLIKELY(refcount(old_field) == 1)) { + if (is_dying(old_field)) { // The code was already on the path to deletion, only temporary // C++ references to it are left. Decrement the refcount, and // return true if it drops to zero. return DecRefOnDeadCode(); } // Otherwise, the code enters the path to destruction now. - mark_as_dying(); - old_count = ref_count_.load(std::memory_order_acquire); - if (V8_LIKELY(old_count == 1)) { + if (ref_count_bitfield_.compare_exchange_weak( + old_field, old_field | kIsDyingMask, + std::memory_order_acq_rel)) { // No other thread got in the way. Commit to the decision. DecRefOnPotentiallyDeadCode(); return false; } - // Another thread managed to increment the refcount again, just - // before we set the "dying" bit. So undo that, and resume the - // loop to evaluate again what needs to be done. - undo_mark_as_dying(); + // Another thread interfered. Re-evaluate what to do. + continue; } - DCHECK_LT(1, old_count); - if (ref_count_.compare_exchange_weak(old_count, old_count - 1, - std::memory_order_acq_rel)) { + DCHECK_LT(1, refcount(old_field)); + if (ref_count_bitfield_.compare_exchange_weak( + old_field, old_field - 1, std::memory_order_acq_rel)) { return false; } } @@ -304,16 +314,18 @@ class V8_EXPORT_PRIVATE WasmCode final { // Decrement the ref count on code that is known to be in use (i.e. the ref // count cannot drop to zero here). void DecRefOnLiveCode() { - [[maybe_unused]] int old_count = - ref_count_.fetch_sub(1, std::memory_order_acq_rel); - DCHECK_LE(2, old_count); + [[maybe_unused]] uint32_t old_bitfield_value = + ref_count_bitfield_.fetch_sub(1, std::memory_order_acq_rel); + DCHECK_LE(2, refcount(old_bitfield_value)); } // Decrement the ref count on code that is known to be dead, even though there // might still be C++ references. Returns whether this drops the last // reference and the code needs to be freed. V8_WARN_UNUSED_RESULT bool DecRefOnDeadCode() { - return ref_count_.fetch_sub(1, std::memory_order_acq_rel) == 1; + uint32_t old_bitfield_value = + ref_count_bitfield_.fetch_sub(1, std::memory_order_acq_rel); + return refcount(old_bitfield_value) == 1; } // Decrement the ref count on a set of {WasmCode} objects, potentially @@ -322,9 +334,9 @@ class V8_EXPORT_PRIVATE WasmCode final { // Called by the WasmEngine when it shuts down for code it thinks is // probably dead (i.e. is in the "potentially_dead_code_" set). Wrapped - // in a method only because {ref_count_} is private. + // in a method only because {ref_count_bitfield_} is private. void DcheckRefCountIsOne() { - DCHECK_EQ(1, ref_count_.load(std::memory_order_acquire)); + DCHECK_EQ(1, refcount(ref_count_bitfield_.load(std::memory_order_acquire))); } // Returns the last source position before {offset}. @@ -341,7 +353,15 @@ class V8_EXPORT_PRIVATE WasmCode final { return ForDebuggingField::decode(flags_); } - bool is_dying() const { return dying_.load(std::memory_order_acquire); } + bool is_dying() const { + return is_dying(ref_count_bitfield_.load(std::memory_order_acquire)); + } + static bool is_dying(uint32_t bit_field_value) { + return (bit_field_value & kIsDyingMask) != 0; + } + static uint32_t refcount(uint32_t bit_field_value) { + return bit_field_value & ~kIsDyingMask; + } // Returns {true} for Liftoff code that sets up a feedback vector slot in its // stack frame. @@ -428,11 +448,6 @@ class V8_EXPORT_PRIVATE WasmCode final { // for consideration in the next Code GC cycle. V8_NOINLINE void DecRefOnPotentiallyDeadCode(); - void mark_as_dying() { dying_.store(true, std::memory_order_release); } - // This is rarely necessary to mitigate a race condition. See the comment - // at its (only) call site. - void undo_mark_as_dying() { dying_.store(false, std::memory_order_release); } - NativeModule* const native_module_ = nullptr; uint8_t* const instructions_; const uint64_t signature_hash_; @@ -477,10 +492,6 @@ class V8_EXPORT_PRIVATE WasmCode final { using ForDebuggingField = ExecutionTierField::Next; using FrameHasFeedbackSlotField = ForDebuggingField::Next; - // Will be set to {true} the first time this code object is considered - // "potentially dead" (to be confirmed by the next Wasm Code GC cycle). - std::atomic dying_{false}; - // WasmCode is ref counted. Counters are held by: // 1) The jump table / code table. // 2) {WasmCodeRefScope}s. @@ -491,7 +502,11 @@ class V8_EXPORT_PRIVATE WasmCode final { // it's being used. Once the ref count drops to zero (i.e. after being removed // from (3) and all (2)), the code object is deleted and the memory for the // machine code is freed. - std::atomic ref_count_{1}; + // The topmost bit is used to indicate that the code is in (3). It is stored + // in this same field to avoid race conditions between atomic updates to + // that state and the refcount. + static constexpr uint32_t kIsDyingMask = 0x8000'0000u; + std::atomic ref_count_bitfield_{1}; }; WasmCode::Kind GetCodeKind(const WasmCompilationResult& result); @@ -1238,6 +1253,10 @@ class V8_EXPORT_PRIVATE V8_NODISCARD WasmCodeRefScope { // Register a {WasmCode} reference in the current {WasmCodeRefScope}. Fails if // there is no current scope. static void AddRef(WasmCode*); + // Same, but conditional: + // - if the {code} is marked as dying, do nothing, return nullptr. + // - otherwise add a ref and return {code}. + static WasmCode* AddRefIfNotDying(WasmCode* code); private: WasmCodeRefScope* const previous_scope_; diff --git a/deps/v8/src/wasm/wasm-import-wrapper-cache.cc b/deps/v8/src/wasm/wasm-import-wrapper-cache.cc index c1cdbc384374..8d6c439380f2 100644 --- a/deps/v8/src/wasm/wasm-import-wrapper-cache.cc +++ b/deps/v8/src/wasm/wasm-import-wrapper-cache.cc @@ -146,9 +146,7 @@ WasmCode* WasmImportWrapperCache::FindWrapper(WasmCodePointer call_target) { GetProcessWideWasmCodePointerTable()->GetEntrypointWithoutSignatureCheck( call_target)); if (iter == codes_.end()) return nullptr; - WasmCodeRefScope::AddRef(iter->second); - if (iter->second->is_dying()) return nullptr; - return iter->second; + return WasmCodeRefScope::AddRefIfNotDying(iter->second); } WasmCode* WasmImportWrapperCache::CompileWasmImportCallWrapper( @@ -165,8 +163,8 @@ WasmCode* WasmImportWrapperCache::CompileWasmImportCallWrapper( // again whether another thread has just created the wrapper. wasm_code = cache_scope[key]; if (wasm_code) { - WasmCodeRefScope::AddRef(wasm_code); - if (!wasm_code->is_dying()) return wasm_code; + wasm_code = WasmCodeRefScope::AddRefIfNotDying(wasm_code); + if (wasm_code) return wasm_code; } wasm_code = cache_scope.AddWrapper(key, std::move(result), @@ -218,9 +216,9 @@ void WasmImportWrapperCache::Free(std::vector& wrappers) { } code_allocator_->FreeCode(base::VectorOf(wrappers)); for (WasmCode* wrapper : wrappers) { - // TODO(407003348): Drop these checks if they don't trigger in the wild. - CHECK(wrapper->is_dying()); - CHECK_EQ(wrapper->ref_count_.load(std::memory_order_acquire), 0); + // TODO(407003348): Drop this check if it doesn't trigger in the wild. + CHECK_EQ(wrapper->ref_count_bitfield_.load(std::memory_order_acquire), + WasmCode::kIsDyingMask); delete wrapper; } // Make sure nobody tries to access stale pointers. @@ -235,9 +233,7 @@ WasmCode* WasmImportWrapperCache::MaybeGet(ImportCallKind kind, auto it = entry_map_.find({kind, type_index, expected_arity, suspend}); if (it == entry_map_.end()) return nullptr; - WasmCodeRefScope::AddRef(it->second); - if (it->second->is_dying()) return nullptr; - return it->second; + return WasmCodeRefScope::AddRefIfNotDying(it->second); } WasmCode* WasmImportWrapperCache::Lookup(Address pc) const {