Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
addaleax authored and MylesBorins committed Oct 14, 2020
1 parent 0c87776 commit b8529a7
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 6 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Expand Up @@ -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 #####

Expand Down
3 changes: 2 additions & 1 deletion deps/v8/src/api/api.cc
Expand Up @@ -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.
Expand Down
12 changes: 9 additions & 3 deletions deps/v8/src/profiler/heap-profiler.cc
Expand Up @@ -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;

Expand All @@ -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());
}
}
Expand Down Expand Up @@ -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());
Expand All @@ -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);
Expand Down Expand Up @@ -138,10 +142,12 @@ void HeapProfiler::StopHeapObjectsTracking() {
}
}

int HeapProfiler::GetSnapshotsCount() {
int HeapProfiler::GetSnapshotsCount() const {
return static_cast<int>(snapshots_.size());
}

bool HeapProfiler::IsTakingSnapshot() const { return is_taking_snapshot_; }

HeapSnapshot* HeapProfiler::GetSnapshot(int index) {
return snapshots_.at(index).get();
}
Expand Down
4 changes: 3 additions & 1 deletion deps/v8/src/profiler/heap-profiler.h
Expand Up @@ -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<Object> obj);
SnapshotObjectId GetSnapshotObjectId(NativeObject obj);
Expand Down Expand Up @@ -93,6 +94,7 @@ class HeapProfiler : public HeapObjectAllocationTracker {
std::unique_ptr<StringsStorage> names_;
std::unique_ptr<AllocationTracker> allocation_tracker_;
bool is_tracking_object_moves_;
bool is_taking_snapshot_;
base::Mutex profiler_mutex_;
std::unique_ptr<SamplingHeapProfiler> sampling_heap_profiler_;
std::vector<std::pair<v8::HeapProfiler::BuildEmbedderGraphCallback, void*>>
Expand Down
37 changes: 37 additions & 0 deletions deps/v8/test/cctest/test-heap-profiler.cc
Expand Up @@ -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<v8::Object> handle;

{
struct WeakData {
const v8::HeapSnapshot* snapshot;
int* gc_calls;
v8::Global<v8::Object>* 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<WeakData>& data) {
std::unique_ptr<WeakData> weakdata{data.GetParameter()};
const_cast<v8::HeapSnapshot*>(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);
}

0 comments on commit b8529a7

Please sign in to comment.