From b8529a7104fb481257d4fc0e3c86bd64d38b20e9 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 12 Oct 2020 15:01:58 +0200 Subject: [PATCH] deps: V8: cherry-pick 3176bfd447a9 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original commit message: [heap-profiler] Fix crash when a snapshot deleted while taking one Fix a crash/hang that occurred when deleting a snapshot during the GC that is part of taking another one. Specifically, when deleting the only other snapshot in such a situation, the `v8::HeapSnapshot::Delete()` method sees that there is only one (complete) snapshot at that point, and decides that it is okay to perform “delete all snapshots” instead of just deleting the requested one. That resets the internal string lookup table of the heap profiler, but the new snapshot that is currently in progress still holds references to the old string lookup table, leading to a use-after-free segfault or infinite loop. Fix this by guarding against resetting the string table while another heap snapshot is being taken, and add a test that would crash before this fix. This can be triggered in Node.js by repeatedly calling `v8.getHeapSnapshot()`, which provides heap snapshots as weakly held host objects. Change-Id: If9ac3728bf79114000982f1e7bb05e8034299e3c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2464823 Reviewed-by: Ulan Degenbaev Commit-Queue: Ulan Degenbaev Cr-Commit-Position: refs/heads/master@{#70445} Refs: https://github.com/v8/v8/commit/3176bfd447a909fa9608687fe3eabcf62ec7b2eb PR-URL: https://github.com/nodejs/node/pull/35612 Refs: https://github.com/nodejs/node/issues/35559 Reviewed-By: Michaël Zasso Reviewed-By: Colin Ihrig Reviewed-By: Richard Lau Reviewed-By: Jiawen Geng Reviewed-By: Rich Trott Reviewed-By: Gireesh Punathil Reviewed-By: Gerhard Stöbich --- common.gypi | 2 +- deps/v8/src/api/api.cc | 3 +- deps/v8/src/profiler/heap-profiler.cc | 12 ++++++-- deps/v8/src/profiler/heap-profiler.h | 4 ++- deps/v8/test/cctest/test-heap-profiler.cc | 37 +++++++++++++++++++++++ 5 files changed, 52 insertions(+), 6 deletions(-) diff --git a/common.gypi b/common.gypi index 33b1325eb513ea..4745bb5ac77639 100644 --- a/common.gypi +++ b/common.gypi @@ -36,7 +36,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.16', + 'v8_embedder_string': '-node.17', ##### V8 defaults for Node.js ##### diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc index 169c9bd7f90cea..3b6226b0f438d3 100644 --- a/deps/v8/src/api/api.cc +++ b/deps/v8/src/api/api.cc @@ -10745,7 +10745,8 @@ static i::HeapSnapshot* ToInternal(const HeapSnapshot* snapshot) { void HeapSnapshot::Delete() { i::Isolate* isolate = ToInternal(this)->profiler()->isolate(); - if (isolate->heap_profiler()->GetSnapshotsCount() > 1) { + if (isolate->heap_profiler()->GetSnapshotsCount() > 1 || + isolate->heap_profiler()->IsTakingSnapshot()) { ToInternal(this)->Delete(); } else { // If this is the last snapshot, clean up all accessory data as well. diff --git a/deps/v8/src/profiler/heap-profiler.cc b/deps/v8/src/profiler/heap-profiler.cc index f063fd1f476ad3..fc9bd00f47dd0f 100644 --- a/deps/v8/src/profiler/heap-profiler.cc +++ b/deps/v8/src/profiler/heap-profiler.cc @@ -18,7 +18,8 @@ namespace internal { HeapProfiler::HeapProfiler(Heap* heap) : ids_(new HeapObjectsMap(heap)), names_(new StringsStorage()), - is_tracking_object_moves_(false) {} + is_tracking_object_moves_(false), + is_taking_snapshot_(false) {} HeapProfiler::~HeapProfiler() = default; @@ -28,7 +29,8 @@ void HeapProfiler::DeleteAllSnapshots() { } void HeapProfiler::MaybeClearStringsStorage() { - if (snapshots_.empty() && !sampling_heap_profiler_ && !allocation_tracker_) { + if (snapshots_.empty() && !sampling_heap_profiler_ && !allocation_tracker_ && + !is_taking_snapshot_) { names_.reset(new StringsStorage()); } } @@ -66,6 +68,7 @@ HeapSnapshot* HeapProfiler::TakeSnapshot( v8::ActivityControl* control, v8::HeapProfiler::ObjectNameResolver* resolver, bool treat_global_objects_as_roots) { + is_taking_snapshot_ = true; HeapSnapshot* result = new HeapSnapshot(this, treat_global_objects_as_roots); { HeapSnapshotGenerator generator(result, control, resolver, heap()); @@ -78,6 +81,7 @@ HeapSnapshot* HeapProfiler::TakeSnapshot( } ids_->RemoveDeadEntries(); is_tracking_object_moves_ = true; + is_taking_snapshot_ = false; heap()->isolate()->debug()->feature_tracker()->Track( DebugFeatureTracker::kHeapSnapshot); @@ -138,10 +142,12 @@ void HeapProfiler::StopHeapObjectsTracking() { } } -int HeapProfiler::GetSnapshotsCount() { +int HeapProfiler::GetSnapshotsCount() const { return static_cast(snapshots_.size()); } +bool HeapProfiler::IsTakingSnapshot() const { return is_taking_snapshot_; } + HeapSnapshot* HeapProfiler::GetSnapshot(int index) { return snapshots_.at(index).get(); } diff --git a/deps/v8/src/profiler/heap-profiler.h b/deps/v8/src/profiler/heap-profiler.h index 0d964b5c5d606a..21d9bb8fcfd815 100644 --- a/deps/v8/src/profiler/heap-profiler.h +++ b/deps/v8/src/profiler/heap-profiler.h @@ -49,7 +49,8 @@ class HeapProfiler : public HeapObjectAllocationTracker { SnapshotObjectId PushHeapObjectsStats(OutputStream* stream, int64_t* timestamp_us); - int GetSnapshotsCount(); + int GetSnapshotsCount() const; + bool IsTakingSnapshot() const; HeapSnapshot* GetSnapshot(int index); SnapshotObjectId GetSnapshotObjectId(Handle obj); SnapshotObjectId GetSnapshotObjectId(NativeObject obj); @@ -93,6 +94,7 @@ class HeapProfiler : public HeapObjectAllocationTracker { std::unique_ptr names_; std::unique_ptr allocation_tracker_; bool is_tracking_object_moves_; + bool is_taking_snapshot_; base::Mutex profiler_mutex_; std::unique_ptr sampling_heap_profiler_; std::vector> diff --git a/deps/v8/test/cctest/test-heap-profiler.cc b/deps/v8/test/cctest/test-heap-profiler.cc index 63c88ae9b2b686..8a04ec4bc54c2d 100644 --- a/deps/v8/test/cctest/test-heap-profiler.cc +++ b/deps/v8/test/cctest/test-heap-profiler.cc @@ -4126,3 +4126,40 @@ TEST(Bug8373_2) { heap_profiler->StopTrackingHeapObjects(); } + +TEST(HeapSnapshotDeleteDuringTakeSnapshot) { + // Check that a heap snapshot can be deleted during GC while another one + // is being taken. + + LocalContext env; + v8::HandleScope scope(env->GetIsolate()); + v8::HeapProfiler* heap_profiler = env->GetIsolate()->GetHeapProfiler(); + int gc_calls = 0; + v8::Global handle; + + { + struct WeakData { + const v8::HeapSnapshot* snapshot; + int* gc_calls; + v8::Global* handle; + }; + WeakData* data = + new WeakData{heap_profiler->TakeHeapSnapshot(), &gc_calls, &handle}; + + v8::HandleScope scope(env->GetIsolate()); + handle.Reset(env->GetIsolate(), v8::Object::New(env->GetIsolate())); + handle.SetWeak( + data, + [](const v8::WeakCallbackInfo& data) { + std::unique_ptr weakdata{data.GetParameter()}; + const_cast(weakdata->snapshot)->Delete(); + ++*weakdata->gc_calls; + weakdata->handle->Reset(); + }, + v8::WeakCallbackType::kParameter); + } + CHECK_EQ(gc_calls, 0); + + CHECK(ValidateSnapshot(heap_profiler->TakeHeapSnapshot())); + CHECK_EQ(gc_calls, 1); +}