FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: V8: cherry-pick 3176bfd447a9 · nodejs/node@b8529a7 · GitHub

/ node Public

Commit b8529a7

Browse files
addaleax
authored and
MylesBorins
committed
deps: V8: cherry-pick 3176bfd447a9
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 <ulan@chromium.org> Commit-Queue: Ulan Degenbaev <ulan@chromium.org> Cr-Commit-Position: refs/heads/master@{#70445} Refs: v8/v8@3176bfd PR-URL: #35612 Refs: #35559 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
1 parent 0c87776 commit b8529a7

5 files changed

Lines changed: 52 additions & 6 deletions

File tree

‎common.gypi‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636

3737
# Reset this number to 0 on major V8 upgrades.
3838
# Increment by one for each non-official patch applied to deps/v8.
39-
'v8_embedder_string': '-node.16',
39+
'v8_embedder_string': '-node.17',
4040

4141
##### V8 defaults for Node.js #####
4242

‎deps/v8/src/api/api.cc‎

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

1074610746
void HeapSnapshot::Delete() {
1074710747
i::Isolate* isolate = ToInternal(this)->profiler()->isolate();
10748-
if (isolate->heap_profiler()->GetSnapshotsCount() > 1) {
10748+
if (isolate->heap_profiler()->GetSnapshotsCount() > 1 ||
10749+
isolate->heap_profiler()->IsTakingSnapshot()) {
1074910750
ToInternal(this)->Delete();
1075010751
} else {
1075110752
// If this is the last snapshot, clean up all accessory data as well.

‎deps/v8/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
}

‎deps/v8/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*>>

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

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

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

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL