Skip to content

Commit

Permalink
deps: cherry-pick b87d408 from upstream V8
Browse files Browse the repository at this point in the history
Original commit message:

    [heap-profiler] Fix a use-after-free when snapshots are deleted

    If a caller starts the sampling heap profiler and takes a snapshot,
    and then deletes the snapshot before the sampling has completed, a
    use-after-free will occur on the StringsStorage pointer.

    The same issue applies for StartTrackingHeapObjects which shares the
    same StringsStorage object.

    Bug: v8:8373
    Change-Id: I5d69d60d3f9465f9dd3b3bef107c204e0fda0643
    Reviewed-on: https://chromium-review.googlesource.com/c/1301477
    Commit-Queue: Peter Marshall <petermarshall@chromium.org>
    Reviewed-by: Alexei Filippov <alph@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#57114}

PR-URL: nodejs#24272
Refs:
v8/v8@b87d408
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
  • Loading branch information
psmarshall authored and targos committed Dec 6, 2018
1 parent 0e09076 commit e36e9dd
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 2 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,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.2',
'v8_embedder_string': '-node.3',

##### V8 defaults for Node.js #####

Expand Down
9 changes: 8 additions & 1 deletion deps/v8/src/profiler/heap-profiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,14 @@ HeapProfiler::~HeapProfiler() = default;

void HeapProfiler::DeleteAllSnapshots() {
snapshots_.clear();
names_.reset(new StringsStorage());
MaybeClearStringsStorage();
}

void HeapProfiler::MaybeClearStringsStorage() {
if (snapshots_.empty() && !sampling_heap_profiler_ && !allocation_tracker_) {
names_.reset(new StringsStorage());
}
}

void HeapProfiler::RemoveSnapshot(HeapSnapshot* snapshot) {
snapshots_.erase(
Expand Down Expand Up @@ -126,6 +131,7 @@ bool HeapProfiler::StartSamplingHeapProfiler(

void HeapProfiler::StopSamplingHeapProfiler() {
sampling_heap_profiler_.reset();
MaybeClearStringsStorage();
}


Expand Down Expand Up @@ -159,6 +165,7 @@ void HeapProfiler::StopHeapObjectsTracking() {
ids_->StopHeapObjectsTracking();
if (allocation_tracker_) {
allocation_tracker_.reset();
MaybeClearStringsStorage();
heap()->RemoveHeapObjectAllocationTracker(this);
}
}
Expand Down
2 changes: 2 additions & 0 deletions deps/v8/src/profiler/heap-profiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ class HeapProfiler : public HeapObjectAllocationTracker {
v8::PersistentValueVector<v8::Object>* objects);

private:
void MaybeClearStringsStorage();

Heap* heap() const;

// Mapping from HeapObject addresses to objects' uids.
Expand Down
42 changes: 42 additions & 0 deletions deps/v8/test/cctest/test-heap-profiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3875,3 +3875,45 @@ TEST(WeakReference) {
const v8::HeapSnapshot* snapshot = heap_profiler->TakeHeapSnapshot();
CHECK(ValidateSnapshot(snapshot));
}

TEST(Bug8373_1) {
LocalContext env;
v8::HandleScope scope(env->GetIsolate());
v8::HeapProfiler* heap_profiler = env->GetIsolate()->GetHeapProfiler();

heap_profiler->StartSamplingHeapProfiler(100);

heap_profiler->TakeHeapSnapshot();
// Causes the StringsStorage to be deleted.
heap_profiler->DeleteAllHeapSnapshots();

// Triggers an allocation sample that tries to use the StringsStorage.
for (int i = 0; i < 2 * 1024; ++i) {
CompileRun(
"new Array(64);"
"new Uint8Array(16);");
}

heap_profiler->StopSamplingHeapProfiler();
}

TEST(Bug8373_2) {
LocalContext env;
v8::HandleScope scope(env->GetIsolate());
v8::HeapProfiler* heap_profiler = env->GetIsolate()->GetHeapProfiler();

heap_profiler->StartTrackingHeapObjects(true);

heap_profiler->TakeHeapSnapshot();
// Causes the StringsStorage to be deleted.
heap_profiler->DeleteAllHeapSnapshots();

// Triggers an allocations that try to use the StringsStorage.
for (int i = 0; i < 2 * 1024; ++i) {
CompileRun(
"new Array(64);"
"new Uint8Array(16);");
}

heap_profiler->StopTrackingHeapObjects();
}

0 comments on commit e36e9dd

Please sign in to comment.