Skip to content

Commit 3176bfd

Browse files
addaleaxCommit Bot
addaleax
authored and
Commit Bot
committed
[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 <[email protected]> Commit-Queue: Ulan Degenbaev <[email protected]> Cr-Commit-Position: refs/heads/master@{#70445}
1 parent d76abfe commit 3176bfd

4 files changed

Lines changed: 51 additions & 5 deletions

File tree

‎src/api/api.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10896,7 +10896,8 @@ static i::HeapSnapshot* ToInternal(const HeapSnapshot* snapshot) {
1089610896

1089710897
void HeapSnapshot::Delete() {
1089810898
i::Isolate* isolate = ToInternal(this)->profiler()->isolate();
10899-
if (isolate->heap_profiler()->GetSnapshotsCount() > 1) {
10899+
if (isolate->heap_profiler()->GetSnapshotsCount() > 1 ||
10900+
isolate->heap_profiler()->IsTakingSnapshot()) {
1090010901
ToInternal(this)->Delete();
1090110902
} else {
1090210903
// If this is the last snapshot, clean up all accessory data as well.

‎src/profiler/heap-profiler.cc‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ namespace internal {
1818
HeapProfiler::HeapProfiler(Heap* heap)
1919
: ids_(new HeapObjectsMap(heap)),
2020
names_(new StringsStorage()),
21-
is_tracking_object_moves_(false) {}
21+
is_tracking_object_moves_(false),
22+
is_taking_snapshot_(false) {}
2223

2324
HeapProfiler::~HeapProfiler() = default;
2425

@@ -28,7 +29,8 @@ void HeapProfiler::DeleteAllSnapshots() {
2829
}
2930

3031
void HeapProfiler::MaybeClearStringsStorage() {
31-
if (snapshots_.empty() && !sampling_heap_profiler_ && !allocation_tracker_) {
32+
if (snapshots_.empty() && !sampling_heap_profiler_ && !allocation_tracker_ &&
33+
!is_taking_snapshot_) {
3234
names_.reset(new StringsStorage());
3335
}
3436
}
@@ -66,6 +68,7 @@ HeapSnapshot* HeapProfiler::TakeSnapshot(
6668
v8::ActivityControl* control,
6769
v8::HeapProfiler::ObjectNameResolver* resolver,
6870
bool treat_global_objects_as_roots) {
71+
is_taking_snapshot_ = true;
6972
HeapSnapshot* result = new HeapSnapshot(this, treat_global_objects_as_roots);
7073
{
7174
HeapSnapshotGenerator generator(result, control, resolver, heap());
@@ -78,6 +81,7 @@ HeapSnapshot* HeapProfiler::TakeSnapshot(
7881
}
7982
ids_->RemoveDeadEntries();
8083
is_tracking_object_moves_ = true;
84+
is_taking_snapshot_ = false;
8185

8286
heap()->isolate()->debug()->feature_tracker()->Track(
8387
DebugFeatureTracker::kHeapSnapshot);
@@ -138,10 +142,12 @@ void HeapProfiler::StopHeapObjectsTracking() {
138142
}
139143
}
140144

141-
int HeapProfiler::GetSnapshotsCount() {
145+
int HeapProfiler::GetSnapshotsCount() const {
142146
return static_cast<int>(snapshots_.size());
143147
}
144148

149+
bool HeapProfiler::IsTakingSnapshot() const { return is_taking_snapshot_; }
150+
145151
HeapSnapshot* HeapProfiler::GetSnapshot(int index) {
146152
return snapshots_.at(index).get();
147153
}

‎src/profiler/heap-profiler.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,8 @@ class HeapProfiler : public HeapObjectAllocationTracker {
4949

5050
SnapshotObjectId PushHeapObjectsStats(OutputStream* stream,
5151
int64_t* timestamp_us);
52-
int GetSnapshotsCount();
52+
int GetSnapshotsCount() const;
53+
bool IsTakingSnapshot() const;
5354
HeapSnapshot* GetSnapshot(int index);
5455
SnapshotObjectId GetSnapshotObjectId(Handle<Object> obj);
5556
SnapshotObjectId GetSnapshotObjectId(NativeObject obj);
@@ -93,6 +94,7 @@ class HeapProfiler : public HeapObjectAllocationTracker {
9394
std::unique_ptr<StringsStorage> names_;
9495
std::unique_ptr<AllocationTracker> allocation_tracker_;
9596
bool is_tracking_object_moves_;
97+
bool is_taking_snapshot_;
9698
base::Mutex profiler_mutex_;
9799
std::unique_ptr<SamplingHeapProfiler> sampling_heap_profiler_;
98100
std::vector<std::pair<v8::HeapProfiler::BuildEmbedderGraphCallback, void*>>

‎test/cctest/test-heap-profiler.cc‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4121,3 +4121,40 @@ TEST(Bug8373_2) {
41214121

41224122
heap_profiler->StopTrackingHeapObjects();
41234123
}
4124+
4125+
TEST(HeapSnapshotDeleteDuringTakeSnapshot) {
4126+
// Check that a heap snapshot can be deleted during GC while another one
4127+
// is being taken.
4128+
4129+
LocalContext env;
4130+
v8::HandleScope scope(env->GetIsolate());
4131+
v8::HeapProfiler* heap_profiler = env->GetIsolate()->GetHeapProfiler();
4132+
int gc_calls = 0;
4133+
v8::Global<v8::Object> handle;
4134+
4135+
{
4136+
struct WeakData {
4137+
const v8::HeapSnapshot* snapshot;
4138+
int* gc_calls;
4139+
v8::Global<v8::Object>* handle;
4140+
};
4141+
WeakData* data =
4142+
new WeakData{heap_profiler->TakeHeapSnapshot(), &gc_calls, &handle};
4143+
4144+
v8::HandleScope scope(env->GetIsolate());
4145+
handle.Reset(env->GetIsolate(), v8::Object::New(env->GetIsolate()));
4146+
handle.SetWeak(
4147+
data,
4148+
[](const v8::WeakCallbackInfo<WeakData>& data) {
4149+
std::unique_ptr<WeakData> weakdata{data.GetParameter()};
4150+
const_cast<v8::HeapSnapshot*>(weakdata->snapshot)->Delete();
4151+
++*weakdata->gc_calls;
4152+
weakdata->handle->Reset();
4153+
},
4154+
v8::WeakCallbackType::kParameter);
4155+
}
4156+
CHECK_EQ(gc_calls, 0);
4157+
4158+
CHECK(ValidateSnapshot(heap_profiler->TakeHeapSnapshot()));
4159+
CHECK_EQ(gc_calls, 1);
4160+
}

0 commit comments

Comments
 (0)