From e5db178ab6ced4bd2fb315457dc1e3cabf9d379f Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Tue, 5 Jul 2022 08:28:22 -0700 Subject: [PATCH] feat: enable v8 sandboxed pointers (#34724) * feat: enable v8 sandboxed pointers * update breaking-changes.md * update zero-fill patch benchmarks showed the function call was slower --- build/args/all.gn | 4 - docs/breaking-changes.md | 7 + .../node/support_v8_sandboxed_pointers.patch | 170 ++++-------------- patches/v8/.patches | 1 - .../force_disable_v8_sandboxed_pointers.patch | 24 --- 5 files changed, 41 insertions(+), 165 deletions(-) delete mode 100644 patches/v8/force_disable_v8_sandboxed_pointers.patch diff --git a/build/args/all.gn b/build/args/all.gn index 9a3fee65b8..f1de439d83 100644 --- a/build/args/all.gn +++ b/build/args/all.gn @@ -45,7 +45,3 @@ enable_cet_shadow_stack = false # V8 in the browser process. # Ref: https://source.chromium.org/chromium/chromium/src/+/45fba672185aae233e75d6ddc81ea1e0b30db050:v8/BUILD.gn;l=281 is_cfi = false - -# TODO(nornagon): this is disabled until node.js's internals can be made -# compatible with sandboxed pointers. -v8_enable_sandboxed_pointers = false diff --git a/docs/breaking-changes.md b/docs/breaking-changes.md index ad74ddcb1c..c6c511b32b 100644 --- a/docs/breaking-changes.md +++ b/docs/breaking-changes.md @@ -14,6 +14,13 @@ This document uses the following convention to categorize breaking changes: ## Planned Breaking API Changes (20.0) +### Behavior Changed: V8 Memory Cage enabled + +The V8 memory cage has been enabled, which has implications for native modules +which wrap non-V8 memory with `ArrayBuffer` or `Buffer`. See the [blog post +about the V8 memory cage](https://www.electronjs.org/blog/v8-memory-cage) for +more details. + ### API Changed: `webContents.printToPDF()` `webContents.printToPDF()` has been modified to conform to [`Page.printToPDF`](https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-printToPDF) in the Chrome DevTools Protocol. This has been changes in order to diff --git a/patches/node/support_v8_sandboxed_pointers.patch b/patches/node/support_v8_sandboxed_pointers.patch index 08a61216ab..76c60ed36b 100644 --- a/patches/node/support_v8_sandboxed_pointers.patch +++ b/patches/node/support_v8_sandboxed_pointers.patch @@ -6,84 +6,27 @@ Subject: support V8 sandboxed pointers This refactors several allocators to allocate within the V8 memory cage, allowing them to be compatible with the V8_SANDBOXED_POINTERS feature. -diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js -index 4c459b58b5a048d9d8a4f15f4011e7cce68089f4..6fb4c8d4567aee5b313ad621ea42699a196f18c7 100644 ---- a/lib/internal/bootstrap/pre_execution.js -+++ b/lib/internal/bootstrap/pre_execution.js -@@ -14,7 +14,6 @@ const { - getOptionValue, - getEmbedderOptions, - } = require('internal/options'); --const { reconnectZeroFillToggle } = require('internal/buffer'); - const { - defineOperation, - emitExperimentalWarning, -@@ -26,10 +25,6 @@ const { ERR_MANIFEST_ASSERT_INTEGRITY } = require('internal/errors').codes; - const assert = require('internal/assert'); - - function prepareMainThreadExecution(expandArgv1 = false) { -- // TODO(joyeecheung): this is also necessary for workers when they deserialize -- // this toggle from the snapshot. -- reconnectZeroFillToggle(); -- - // Patch the process object with legacy properties and normalizations - patchProcessObject(expandArgv1); - setupTraceCategoryState(); -diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js -index bd38cf48a7fc6e8d61d8f11fa15c34aee182cbe3..1aa071cdc071dcdaf5c3b4bed0d3d76e5871731d 100644 ---- a/lib/internal/buffer.js -+++ b/lib/internal/buffer.js -@@ -30,7 +30,7 @@ const { - hexWrite, - ucs2Write, - utf8Write, -- getZeroFillToggle -+ setZeroFillToggle - } = internalBinding('buffer'); - const { - untransferable_object_private_symbol, -@@ -1055,24 +1055,15 @@ function markAsUntransferable(obj) { - // in C++. - // |zeroFill| can be undefined when running inside an isolate where we - // do not own the ArrayBuffer allocator. Zero fill is always on in that case. --let zeroFill = getZeroFillToggle(); - function createUnsafeBuffer(size) { -- zeroFill[0] = 0; -+ setZeroFillToggle(false); - try { - return new FastBuffer(size); - } finally { -- zeroFill[0] = 1; -+ setZeroFillToggle(true) - } - } - --// The connection between the JS land zero fill toggle and the --// C++ one in the NodeArrayBufferAllocator gets lost if the toggle --// is deserialized from the snapshot, because V8 owns the underlying --// memory of this toggle. This resets the connection. --function reconnectZeroFillToggle() { -- zeroFill = getZeroFillToggle(); --} -- - module.exports = { - FastBuffer, - addBufferPrototypeMethods, -@@ -1080,5 +1071,4 @@ module.exports = { - createUnsafeBuffer, - readUInt16BE, - readUInt32BE, -- reconnectZeroFillToggle - }; diff --git a/src/api/environment.cc b/src/api/environment.cc -index 2abf5994405e8da2a04d1b23b75ccd3658398474..024d612a04d83583b397549589d994e32cf0107f 100644 +index 2abf5994405e8da2a04d1b23b75ccd3658398474..b06e8529bb8ca2fa6d7f0735531bbbf39da6af12 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc -@@ -83,16 +83,16 @@ MaybeLocal PrepareStackTraceCallback(Local context, +@@ -80,19 +80,27 @@ MaybeLocal PrepareStackTraceCallback(Local context, + return result; + } + ++NodeArrayBufferAllocator::NodeArrayBufferAllocator() { ++ zero_fill_field_ = static_cast(allocator_->Allocate(sizeof(*zero_fill_field_))); ++} ++ ++NodeArrayBufferAllocator::~NodeArrayBufferAllocator() { ++ allocator_->Free(zero_fill_field_, sizeof(*zero_fill_field_)); ++} ++ void* NodeArrayBufferAllocator::Allocate(size_t size) { void* ret; - if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers) +- if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers) - ret = UncheckedCalloc(size); ++ if (*zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers) + ret = allocator_->Allocate(size); else - ret = UncheckedMalloc(size); @@ -99,7 +42,7 @@ index 2abf5994405e8da2a04d1b23b75ccd3658398474..024d612a04d83583b397549589d994e3 if (LIKELY(ret != nullptr)) total_mem_usage_.fetch_add(size, std::memory_order_relaxed); return ret; -@@ -100,7 +100,7 @@ void* NodeArrayBufferAllocator::AllocateUninitialized(size_t size) { +@@ -100,7 +108,7 @@ void* NodeArrayBufferAllocator::AllocateUninitialized(size_t size) { void* NodeArrayBufferAllocator::Reallocate( void* data, size_t old_size, size_t size) { @@ -108,7 +51,7 @@ index 2abf5994405e8da2a04d1b23b75ccd3658398474..024d612a04d83583b397549589d994e3 if (LIKELY(ret != nullptr) || UNLIKELY(size == 0)) total_mem_usage_.fetch_add(size - old_size, std::memory_order_relaxed); return ret; -@@ -108,7 +108,7 @@ void* NodeArrayBufferAllocator::Reallocate( +@@ -108,7 +116,7 @@ void* NodeArrayBufferAllocator::Reallocate( void NodeArrayBufferAllocator::Free(void* data, size_t size) { total_mem_usage_.fetch_sub(size, std::memory_order_relaxed); @@ -209,65 +152,6 @@ index c431159e6f77f8c86844bcadb86012b056d03372..9f57ac58d826cb0aae422ddca54e2136 v8::Local ToArrayBuffer(Environment* env); -diff --git a/src/node_buffer.cc b/src/node_buffer.cc -index 215bd8003aabe17e43ac780c723cfe971b437eae..eb00eb6f592e20f3c17a529f30b09673774eb1c1 100644 ---- a/src/node_buffer.cc -+++ b/src/node_buffer.cc -@@ -1175,33 +1175,14 @@ void SetBufferPrototype(const FunctionCallbackInfo& args) { - env->set_buffer_prototype_object(proto); - } - --void GetZeroFillToggle(const FunctionCallbackInfo& args) { -+void SetZeroFillToggle(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - NodeArrayBufferAllocator* allocator = env->isolate_data()->node_allocator(); - Local ab; -- // It can be a nullptr when running inside an isolate where we -- // do not own the ArrayBuffer allocator. -- if (allocator == nullptr) { -- // Create a dummy Uint32Array - the JS land can only toggle the C++ land -- // setting when the allocator uses our toggle. With this the toggle in JS -- // land results in no-ops. -- ab = ArrayBuffer::New(env->isolate(), sizeof(uint32_t)); -- } else { -+ if (allocator != nullptr) { - uint32_t* zero_fill_field = allocator->zero_fill_field(); -- std::unique_ptr backing = -- ArrayBuffer::NewBackingStore(zero_fill_field, -- sizeof(*zero_fill_field), -- [](void*, size_t, void*) {}, -- nullptr); -- ab = ArrayBuffer::New(env->isolate(), std::move(backing)); -+ *zero_fill_field = args[0]->BooleanValue(env->isolate()); - } -- -- ab->SetPrivate( -- env->context(), -- env->untransferable_object_private_symbol(), -- True(env->isolate())).Check(); -- -- args.GetReturnValue().Set(Uint32Array::New(ab, 0, 1)); - } - - void DetachArrayBuffer(const FunctionCallbackInfo& args) { -@@ -1310,7 +1291,7 @@ void Initialize(Local target, - env->SetMethod(target, "ucs2Write", StringWrite); - env->SetMethod(target, "utf8Write", StringWrite); - -- env->SetMethod(target, "getZeroFillToggle", GetZeroFillToggle); -+ env->SetMethod(target, "setZeroFillToggle", SetZeroFillToggle); - } - - } // anonymous namespace -@@ -1350,7 +1331,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { - registry->Register(StringWrite); - registry->Register(StringWrite); - registry->Register(StringWrite); -- registry->Register(GetZeroFillToggle); -+ registry->Register(SetZeroFillToggle); - - registry->Register(DetachArrayBuffer); - registry->Register(CopyArrayBuffer); diff --git a/src/node_i18n.cc b/src/node_i18n.cc index c537a247f55ff070da1988fc8b7309b5692b5c18..59bfb597849cd5a94800d6c83b238ef77245243e 100644 --- a/src/node_i18n.cc @@ -282,12 +166,26 @@ index c537a247f55ff070da1988fc8b7309b5692b5c18..59bfb597849cd5a94800d6c83b238ef7 return ret; diff --git a/src/node_internals.h b/src/node_internals.h -index d37be23cd63e82d4040777bd0e17ed449ec0b15b..0b66996f11c66800a7e21ee84fa101450b856227 100644 +index d37be23cd63e82d4040777bd0e17ed449ec0b15b..eb84760593ff5fb5aa6a8104e8714099f24a67a0 100644 --- a/src/node_internals.h +++ b/src/node_internals.h -@@ -118,6 +118,8 @@ class NodeArrayBufferAllocator : public ArrayBufferAllocator { +@@ -97,7 +97,9 @@ bool InitializePrimordials(v8::Local context); + + class NodeArrayBufferAllocator : public ArrayBufferAllocator { + public: +- inline uint32_t* zero_fill_field() { return &zero_fill_field_; } ++ NodeArrayBufferAllocator(); ++ ~NodeArrayBufferAllocator() override; ++ inline uint32_t* zero_fill_field() { return zero_fill_field_; } + + void* Allocate(size_t size) override; // Defined in src/node.cc + void* AllocateUninitialized(size_t size) override; +@@ -116,8 +118,10 @@ class NodeArrayBufferAllocator : public ArrayBufferAllocator { + } + private: - uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land. +- uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land. ++ uint32_t* zero_fill_field_ = nullptr; // Boolean but exposed as uint32 to JS land. std::atomic total_mem_usage_ {0}; + + std::unique_ptr allocator_{v8::ArrayBuffer::Allocator::NewDefaultAllocator()}; diff --git a/patches/v8/.patches b/patches/v8/.patches index 9df51315c6..0e16c5eabc 100644 --- a/patches/v8/.patches +++ b/patches/v8/.patches @@ -10,4 +10,3 @@ revert_fix_cppgc_removed_deleted_cstors_in_cppheapcreateparams.patch revert_runtime_dhceck_terminating_exception_in_microtasks.patch chore_disable_is_execution_terminating_dcheck.patch build_remove_legacy_oom_error_callback.patch -force_disable_v8_sandboxed_pointers.patch diff --git a/patches/v8/force_disable_v8_sandboxed_pointers.patch b/patches/v8/force_disable_v8_sandboxed_pointers.patch deleted file mode 100644 index ed88bbb205..0000000000 --- a/patches/v8/force_disable_v8_sandboxed_pointers.patch +++ /dev/null @@ -1,24 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Jeremy Rose -Date: Thu, 23 Jun 2022 16:49:28 -0700 -Subject: force disable v8 sandboxed pointers - -temporarily, until we can fix node to be compatible - -diff --git a/BUILD.gn b/BUILD.gn -index 71ee04624868f23a2e204b5ff1e973b45e2f1202..6aa33fdd3c767eb0045db918d2535fe35c98ca4b 100644 ---- a/BUILD.gn -+++ b/BUILD.gn -@@ -506,9 +506,9 @@ if (v8_enable_sandbox == "") { - } - - # Enable sandboxed pointers when the sandbox is enabled. --if (v8_enable_sandbox) { -- v8_enable_sandboxed_pointers = true --} -+#if (v8_enable_sandbox) { -+# v8_enable_sandboxed_pointers = true -+#} - - # Enable all available sandbox features if sandbox future is enabled. - if (v8_enable_sandbox_future) {