forked from electron/electron
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
669b5d6
commit 136c5dc
Showing
9 changed files
with
792 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
112 changes: 112 additions & 0 deletions
112
patches/common/v8/0001-deps-cherry-pick-0483e9a-from-upstream-V8.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
From 0e090768de1844c493013d5e99bd903928aff2ab Mon Sep 17 00:00:00 2001 | ||
From: Joyee Cheung <joyeec9h3@gmail.com> | ||
Date: Tue, 6 Nov 2018 18:05:48 +0800 | ||
Subject: [PATCH 1/4] deps: cherry-pick 0483e9a from upstream V8 | ||
|
||
Original commit message: | ||
|
||
[api] Allow embedder to construct an Array from Local<Value>* | ||
|
||
Currently to obtain a v8::Array out of a C array or a std::vector, | ||
one needs to loop through the elements and call array->Set() multiple | ||
times, and these calls go into v8::Object::Set() which can be slow. | ||
This patch adds a new Array::New overload that converts a | ||
Local<Value>* with known size into a Local<Array>. | ||
|
||
Change-Id: I0a768f0e18eec51e78d58be455482ec6425ca188 | ||
Reviewed-on: https://chromium-review.googlesource.com/c/1317049 | ||
Reviewed-by: Yang Guo <yangguo@chromium.org> | ||
Reviewed-by: Adam Klein <adamk@chromium.org> | ||
Commit-Queue: Joyee Cheung <joyee@igalia.com> | ||
Cr-Commit-Position: refs/heads/master@{#57261} | ||
|
||
Refs: https://github.com/v8/v8/commit/0483e9a9abe77a73632fd85b9c0cd608efa9aa0d | ||
|
||
PR-URL: https://github.com/nodejs/node/pull/24125 | ||
Reviewed-By: Anna Henningsen <anna@addaleax.net> | ||
Reviewed-By: Yang Guo <yangguo@chromium.org> | ||
Reviewed-By: Gus Caplan <me@gus.host> | ||
Reviewed-By: Colin Ihrig <cjihrig@gmail.com> | ||
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> | ||
Reviewed-By: Refael Ackermann <refack@gmail.com> | ||
--- | ||
include/v8.h | 6 ++++++ | ||
src/api.cc | 17 +++++++++++++++++ | ||
test/cctest/test-api.cc | 16 ++++++++++++++++ | ||
3 files changed, 39 insertions(+) | ||
|
||
diff --git a/include/v8.h b/include/v8.h | ||
index a4bbe1b0c4..9b7be9fb93 100644 | ||
--- a/include/v8.h | ||
+++ b/include/v8.h | ||
@@ -3680,6 +3680,12 @@ class V8_EXPORT Array : public Object { | ||
*/ | ||
static Local<Array> New(Isolate* isolate, int length = 0); | ||
|
||
+ /** | ||
+ * Creates a JavaScript array out of a Local<Value> array in C++ | ||
+ * with a known length. | ||
+ */ | ||
+ static Local<Array> New(Isolate* isolate, Local<Value>* elements, | ||
+ size_t length); | ||
V8_INLINE static Array* Cast(Value* obj); | ||
private: | ||
Array(); | ||
diff --git a/src/api.cc b/src/api.cc | ||
index 3f62a23d43..4e233d96dc 100644 | ||
--- a/src/api.cc | ||
+++ b/src/api.cc | ||
@@ -6911,6 +6911,23 @@ Local<v8::Array> v8::Array::New(Isolate* isolate, int length) { | ||
return Utils::ToLocal(obj); | ||
} | ||
|
||
+Local<v8::Array> v8::Array::New(Isolate* isolate, Local<Value>* elements, | ||
+ size_t length) { | ||
+ i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate); | ||
+ i::Factory* factory = i_isolate->factory(); | ||
+ LOG_API(i_isolate, Array, New); | ||
+ ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate); | ||
+ int len = static_cast<int>(length); | ||
+ | ||
+ i::Handle<i::FixedArray> result = factory->NewFixedArray(len); | ||
+ for (int i = 0; i < len; i++) { | ||
+ i::Handle<i::Object> element = Utils::OpenHandle(*elements[i]); | ||
+ result->set(i, *element); | ||
+ } | ||
+ | ||
+ return Utils::ToLocal( | ||
+ factory->NewJSArrayWithElements(result, i::PACKED_ELEMENTS, len)); | ||
+} | ||
|
||
uint32_t v8::Array::Length() const { | ||
i::Handle<i::JSArray> obj = Utils::OpenHandle(this); | ||
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc | ||
index 9eb73fab7e..0d92508d24 100644 | ||
--- a/test/cctest/test-api.cc | ||
+++ b/test/cctest/test-api.cc | ||
@@ -5225,6 +5225,22 @@ THREADED_TEST(Array) { | ||
CHECK_EQ(27u, array->Length()); | ||
array = v8::Array::New(context->GetIsolate(), -27); | ||
CHECK_EQ(0u, array->Length()); | ||
+ | ||
+ std::vector<Local<Value>> vector = {v8_num(1), v8_num(2), v8_num(3)}; | ||
+ array = v8::Array::New(context->GetIsolate(), vector.data(), vector.size()); | ||
+ CHECK_EQ(vector.size(), array->Length()); | ||
+ CHECK_EQ(1, arr->Get(context.local(), 0) | ||
+ .ToLocalChecked() | ||
+ ->Int32Value(context.local()) | ||
+ .FromJust()); | ||
+ CHECK_EQ(2, arr->Get(context.local(), 1) | ||
+ .ToLocalChecked() | ||
+ ->Int32Value(context.local()) | ||
+ .FromJust()); | ||
+ CHECK_EQ(3, arr->Get(context.local(), 2) | ||
+ .ToLocalChecked() | ||
+ ->Int32Value(context.local()) | ||
+ .FromJust()); | ||
} | ||
|
||
|
||
-- | ||
2.14.3 (Apple Git-98) | ||
|
136 changes: 136 additions & 0 deletions
136
patches/common/v8/0002-deps-cherry-pick-b87d408-from-upstream-V8.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
From e36e9dde38caf3517890da2265e6dd9f127abe72 Mon Sep 17 00:00:00 2001 | ||
From: Peter Marshall <petermarshall@chromium.org> | ||
Date: Fri, 9 Nov 2018 13:06:07 +0100 | ||
Subject: [PATCH 2/4] deps: cherry-pick b87d408 from upstream V8 | ||
|
||
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: https://github.com/nodejs/node/pull/24272 | ||
Refs: | ||
https://github.com/v8/v8/commit/b87d408f65b9ab49a4d199e850d2358995deaeb2 | ||
Reviewed-By: Colin Ihrig <cjihrig@gmail.com> | ||
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> | ||
--- | ||
src/profiler/heap-profiler.cc | 9 ++++++++- | ||
src/profiler/heap-profiler.h | 2 ++ | ||
test/cctest/test-heap-profiler.cc | 42 +++++++++++++++++++++++++++++++++++++++ | ||
3 files changed, 52 insertions(+), 1 deletion(-) | ||
|
||
diff --git a/src/profiler/heap-profiler.cc b/src/profiler/heap-profiler.cc | ||
index 0978e76cff..58a8f3851f 100644 | ||
--- a/src/profiler/heap-profiler.cc | ||
+++ b/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/src/profiler/heap-profiler.h b/src/profiler/heap-profiler.h | ||
index acbdc6aa7a..1e3527765e 100644 | ||
--- a/src/profiler/heap-profiler.h | ||
+++ b/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/test/cctest/test-heap-profiler.cc b/test/cctest/test-heap-profiler.cc | ||
index 257ef1c723..f3c545fd83 100644 | ||
--- a/test/cctest/test-heap-profiler.cc | ||
+++ b/test/cctest/test-heap-profiler.cc | ||
@@ -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(); | ||
+} | ||
-- | ||
2.14.3 (Apple Git-98) | ||
|
Oops, something went wrong.