deps: cherry-pick b87d408 from upstream V8

psmarshall · danbev · commit a8847aa5e009 · 2018-11-12T07:24:09.000+01:00
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: #24272 Refs: v8/v8@b87d408 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
diff --git a/common.gypi b/common.gypi
@@ -32,7 +32,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.10',
+    'v8_embedder_string': '-node.11',
 
     # Enable disassembler for `--print-code` v8 options
     'v8_enable_disassembler': 1,
diff --git a/deps/v8/src/profiler/heap-profiler.cc b/deps/v8/src/profiler/heap-profiler.cc
@@ -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(
@@ -126,6 +131,7 @@ bool HeapProfiler::StartSamplingHeapProfiler(
 
 void HeapProfiler::StopSamplingHeapProfiler() {
   sampling_heap_profiler_.reset();
+  MaybeClearStringsStorage();
 }
 
 
@@ -159,6 +165,7 @@ void HeapProfiler::StopHeapObjectsTracking() {
   ids_->StopHeapObjectsTracking();
   if (allocation_tracker_) {
     allocation_tracker_.reset();
+    MaybeClearStringsStorage();
     heap()->RemoveHeapObjectAllocationTracker(this);
   }
 }
diff --git a/deps/v8/src/profiler/heap-profiler.h b/deps/v8/src/profiler/heap-profiler.h
@@ -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.
diff --git a/deps/v8/test/cctest/test-heap-profiler.cc b/deps/v8/test/cctest/test-heap-profiler.cc
@@ -3900,3 +3900,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();
+}

Original file line number	Diff line number	Diff line change
`@@ -23,9 +23,14 @@ HeapProfiler::~HeapProfiler() = default;`
`23`	`23`
`24`	`24`	`void HeapProfiler::DeleteAllSnapshots() {`
`25`	`25`	`snapshots_.clear();`
`26`		`- names_.reset(new StringsStorage());`
	`26`	`+ MaybeClearStringsStorage();`
`27`	`27`	`}`
`28`	`28`
	`29`	`+void HeapProfiler::MaybeClearStringsStorage() {`
	`30`	`+ if (snapshots_.empty() && !sampling_heap_profiler_ && !allocation_tracker_) {`
	`31`	`+ names_.reset(new StringsStorage());`
	`32`	`+ }`
	`33`	`+}`
`29`	`34`
`30`	`35`	`void HeapProfiler::RemoveSnapshot(HeapSnapshot* snapshot) {`
`31`	`36`	`snapshots_.erase(`
`@@ -126,6 +131,7 @@ bool HeapProfiler::StartSamplingHeapProfiler(`
`126`	`131`
`127`	`132`	`void HeapProfiler::StopSamplingHeapProfiler() {`
`128`	`133`	`sampling_heap_profiler_.reset();`
	`134`	`+ MaybeClearStringsStorage();`
`129`	`135`	`}`
`130`	`136`
`131`	`137`
`@@ -159,6 +165,7 @@ void HeapProfiler::StopHeapObjectsTracking() {`
`159`	`165`	`ids_->StopHeapObjectsTracking();`
`160`	`166`	`if (allocation_tracker_) {`
`161`	`167`	`allocation_tracker_.reset();`
	`168`	`+ MaybeClearStringsStorage();`
`162`	`169`	`heap()->RemoveHeapObjectAllocationTracker(this);`
`163`	`170`	`}`
`164`	`171`	`}`