From 13fd9bc368fa498cb0bb954389d3a48171d841b7 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Tue, 13 Jun 2023 12:16:14 +0200 Subject: [PATCH 1/4] fix: window.open causing occasional Node.js crashes --- ...x_harden_blink_scriptstate_maybefrom.patch | 5 +-- shell/common/node_bindings.cc | 8 ++--- shell/common/node_bindings.h | 36 +++++++++++++------ shell/renderer/electron_renderer_client.cc | 5 +-- shell/renderer/web_worker_observer.cc | 13 ++++--- spec/chromium-spec.ts | 22 ++++++++++++ 6 files changed, 67 insertions(+), 22 deletions(-) diff --git a/patches/chromium/fix_harden_blink_scriptstate_maybefrom.patch b/patches/chromium/fix_harden_blink_scriptstate_maybefrom.patch index c31a05d8dd6cf..8af443435c122 100644 --- a/patches/chromium/fix_harden_blink_scriptstate_maybefrom.patch +++ b/patches/chromium/fix_harden_blink_scriptstate_maybefrom.patch @@ -40,13 +40,14 @@ accessing uninitialized lower indexes can return garbage values that cannot be n Refer to v8::EmbedderDataSlot::store_aligned_pointer for context. diff --git a/gin/public/gin_embedders.h b/gin/public/gin_embedders.h -index 8d7c5631fd8f1499c67384286f0e3c4037673b32..6a7491bc27334f6d1b1175eaa472c888e2b35b5e 100644 +index 8d7c5631fd8f1499c67384286f0e3c4037673b32..99b2e2f63be8a46c5546dd53bc9b05e8c54e857c 100644 --- a/gin/public/gin_embedders.h +++ b/gin/public/gin_embedders.h -@@ -18,6 +18,7 @@ namespace gin { +@@ -18,6 +18,8 @@ namespace gin { enum GinEmbedder : uint16_t { kEmbedderNativeGin, kEmbedderBlink, ++ kEmbedderElectron, + kEmbedderBlinkTag, kEmbedderPDFium, kEmbedderFuchsia, diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 830e355b807c8..583a7a33345cf 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -33,7 +33,6 @@ #include "shell/common/gin_helper/locker.h" #include "shell/common/gin_helper/microtasks_scope.h" #include "shell/common/mac/main_application_bundle.h" -#include "shell/common/node_includes.h" #include "third_party/blink/renderer/bindings/core/v8/v8_initializer.h" // nogncheck #include "third_party/electron_node/src/debug_utils.h" @@ -518,8 +517,9 @@ node::Environment* NodeBindings::CreateEnvironment( args.insert(args.begin() + 1, init_script); - if (!isolate_data_) - isolate_data_ = node::CreateIsolateData(isolate, uv_loop_, platform); + auto* isolate_data = node::CreateIsolateData(isolate, uv_loop_, platform); + context->SetAlignedPointerInEmbedderData(kElectronContextEmbedderDataIndex, + static_cast(isolate_data)); node::Environment* env; uint64_t flags = node::EnvironmentFlags::kDefaultFlags | @@ -550,7 +550,7 @@ node::Environment* NodeBindings::CreateEnvironment( { v8::TryCatch try_catch(isolate); env = node::CreateEnvironment( - isolate_data_, context, args, exec_args, + static_cast(isolate_data), context, args, exec_args, static_cast(flags)); if (try_catch.HasCaught()) { diff --git a/shell/common/node_bindings.h b/shell/common/node_bindings.h index f8df9cb90876d..cb0dd998c9354 100644 --- a/shell/common/node_bindings.h +++ b/shell/common/node_bindings.h @@ -13,6 +13,9 @@ #include "base/memory/raw_ptr.h" #include "base/memory/raw_ptr_exclusion.h" #include "base/memory/weak_ptr.h" +#include "gin/public/context_holder.h" +#include "gin/public/gin_embedders.h" +#include "shell/common/node_includes.h" #include "uv.h" // NOLINT(build/include_directory) #include "v8/include/v8.h" @@ -20,14 +23,14 @@ namespace base { class SingleThreadTaskRunner; } -namespace node { -class Environment; -class MultiIsolatePlatform; -class IsolateData; -} // namespace node - namespace electron { +// Choose a reasonable unique index that's higher than any Blink uses +// and thus unlikely to collide with an existing index. +static constexpr int kElectronContextEmbedderDataIndex = + static_cast(gin::kPerContextDataStartIndex) + + static_cast(gin::kEmbedderElectron); + // A helper class to manage uv_handle_t types, e.g. uv_async_t. // // As per the uv docs: "uv_close() MUST be called on each handle before @@ -108,11 +111,24 @@ class NodeBindings { // Notify embed thread to start polling after environment is loaded. void StartPolling(); - // Gets/sets the per isolate data. - void set_isolate_data(node::IsolateData* isolate_data) { - isolate_data_ = isolate_data; + // Clears the PerIsolateData. + void clear_isolate_data(v8::Local context) { + context->SetAlignedPointerInEmbedderData(kElectronContextEmbedderDataIndex, + nullptr); + } + + node::IsolateData* isolate_data(v8::Local context) const { + if (context->GetNumberOfEmbedderDataFields() <= + kElectronContextEmbedderDataIndex) { + return nullptr; + } + auto* isolate_data = static_cast( + context->GetAlignedPointerFromEmbedderData( + kElectronContextEmbedderDataIndex)); + CHECK(isolate_data); + CHECK(isolate_data->event_loop()); + return isolate_data; } - node::IsolateData* isolate_data() const { return isolate_data_; } // Gets/sets the environment to wrap uv loop. void set_uv_env(node::Environment* env) { uv_env_ = env; } diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index 6affa21323c72..181babee8f30c 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/shell/renderer/electron_renderer_client.cc @@ -6,6 +6,7 @@ #include "base/command_line.h" #include "base/containers/contains.h" +#include "base/debug/stack_trace.h" #include "content/public/renderer/render_frame.h" #include "electron/buildflags/buildflags.h" #include "net/http/http_request_headers.h" @@ -144,8 +145,8 @@ void ElectronRendererClient::WillReleaseScriptContext( node::FreeEnvironment(env); if (node_bindings_->uv_env() == nullptr) { - node::FreeIsolateData(node_bindings_->isolate_data()); - node_bindings_->set_isolate_data(nullptr); + node::FreeIsolateData(node_bindings_->isolate_data(context)); + node_bindings_->clear_isolate_data(context); } microtask_queue->set_microtasks_policy(old_policy); diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc index 2ee0d387cc5b8..f657e5412307a 100644 --- a/shell/renderer/web_worker_observer.cc +++ b/shell/renderer/web_worker_observer.cc @@ -46,13 +46,18 @@ WebWorkerObserver::~WebWorkerObserver() { // Node.js expects `kExplicit` microtasks policy and will run microtasks // checkpoints after every call into JavaScript. Since we use a different // policy in the renderer - switch to `kExplicit` - v8::MicrotaskQueue* microtask_queue = - node_bindings_->uv_env()->context()->GetMicrotaskQueue(); + auto* env = node_bindings_->uv_env(); + v8::MicrotaskQueue* microtask_queue = env->context()->GetMicrotaskQueue(); auto old_policy = microtask_queue->microtasks_policy(); DCHECK_EQ(microtask_queue->GetMicrotasksScopeDepth(), 0); microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit); - node::FreeEnvironment(node_bindings_->uv_env()); - node::FreeIsolateData(node_bindings_->isolate_data()); + + node::FreeEnvironment(env); + if (node_bindings_->uv_env() == nullptr) { + node::FreeIsolateData(node_bindings_->isolate_data(env->context())); + node_bindings_->clear_isolate_data(env->context()); + } + microtask_queue->set_microtasks_policy(old_policy); } diff --git a/spec/chromium-spec.ts b/spec/chromium-spec.ts index c075642847483..1c8c7039e975e 100644 --- a/spec/chromium-spec.ts +++ b/spec/chromium-spec.ts @@ -1114,6 +1114,28 @@ describe('chromium features', () => { expect(frameName).to.equal('__proto__'); }); + it('works when used in conjunction with the vm module', async () => { + const w = new BrowserWindow({ + show: false, + webPreferences: { + nodeIntegration: true, + contextIsolation: false + } + }); + + await w.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html')); + + const { contextObject } = await w.webContents.executeJavaScript(`(async () => { + const vm = require('node:vm'); + const contextObject = { count: 1, type: 'gecko' }; + window.open(''); + vm.runInNewContext('count += 1; type = "chameleon";', contextObject); + return { contextObject }; + })()`); + + expect(contextObject).to.deep.equal({ count: 2, type: 'chameleon' }); + }); + // FIXME(nornagon): I'm not sure this ... ever was correct? xit('inherit options of parent window', async () => { const w = new BrowserWindow({ show: false, width: 123, height: 456 }); From 0701dd38b420e41235946932c9c6b85e969abdc3 Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Fri, 14 Jul 2023 17:38:16 +0900 Subject: [PATCH 2/4] chore: always free isolate data --- shell/renderer/electron_renderer_client.cc | 6 ++-- shell/renderer/web_worker_observer.cc | 35 ++++++++++------------ 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index 181babee8f30c..a6d68e930fff7 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/shell/renderer/electron_renderer_client.cc @@ -144,10 +144,8 @@ void ElectronRendererClient::WillReleaseScriptContext( microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit); node::FreeEnvironment(env); - if (node_bindings_->uv_env() == nullptr) { - node::FreeIsolateData(node_bindings_->isolate_data(context)); - node_bindings_->clear_isolate_data(context); - } + node::FreeIsolateData(node_bindings_->isolate_data(context)); + node_bindings_->clear_isolate_data(context); microtask_queue->set_microtasks_policy(old_policy); diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc index f657e5412307a..8e54907908949 100644 --- a/shell/renderer/web_worker_observer.cc +++ b/shell/renderer/web_worker_observer.cc @@ -41,25 +41,7 @@ WebWorkerObserver::WebWorkerObserver() electron_bindings_( std::make_unique(node_bindings_->uv_loop())) {} -WebWorkerObserver::~WebWorkerObserver() { - // Destroying the node environment will also run the uv loop, - // Node.js expects `kExplicit` microtasks policy and will run microtasks - // checkpoints after every call into JavaScript. Since we use a different - // policy in the renderer - switch to `kExplicit` - auto* env = node_bindings_->uv_env(); - v8::MicrotaskQueue* microtask_queue = env->context()->GetMicrotaskQueue(); - auto old_policy = microtask_queue->microtasks_policy(); - DCHECK_EQ(microtask_queue->GetMicrotasksScopeDepth(), 0); - microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit); - - node::FreeEnvironment(env); - if (node_bindings_->uv_env() == nullptr) { - node::FreeIsolateData(node_bindings_->isolate_data(env->context())); - node_bindings_->clear_isolate_data(env->context()); - } - - microtask_queue->set_microtasks_policy(old_policy); -} +WebWorkerObserver::~WebWorkerObserver() = default; void WebWorkerObserver::WorkerScriptReadyForEvaluation( v8::Local worker_context) { @@ -100,6 +82,21 @@ void WebWorkerObserver::ContextWillDestroy(v8::Local context) { if (env) gin_helper::EmitEvent(env->isolate(), env->process_object(), "exit"); + // Destroying the node environment will also run the uv loop, + // Node.js expects `kExplicit` microtasks policy and will run microtasks + // checkpoints after every call into JavaScript. Since we use a different + // policy in the renderer - switch to `kExplicit` + v8::MicrotaskQueue* microtask_queue = context->GetMicrotaskQueue(); + auto old_policy = microtask_queue->microtasks_policy(); + DCHECK_EQ(microtask_queue->GetMicrotasksScopeDepth(), 0); + microtask_queue->set_microtasks_policy(v8::MicrotasksPolicy::kExplicit); + + node::FreeEnvironment(env); + node::FreeIsolateData(node_bindings_->isolate_data(context)); + node_bindings_->clear_isolate_data(context); + + microtask_queue->set_microtasks_policy(old_policy); + if (lazy_tls->Get()) lazy_tls->Set(nullptr); } From a0287c4ecb1e7be1f778757d5d9161d61376ea5c Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Fri, 14 Jul 2023 17:46:27 +0900 Subject: [PATCH 3/4] chore: clear pending ticks in worker thread --- shell/renderer/web_worker_observer.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/shell/renderer/web_worker_observer.cc b/shell/renderer/web_worker_observer.cc index 8e54907908949..d738a10acfa84 100644 --- a/shell/renderer/web_worker_observer.cc +++ b/shell/renderer/web_worker_observer.cc @@ -97,6 +97,9 @@ void WebWorkerObserver::ContextWillDestroy(v8::Local context) { microtask_queue->set_microtasks_policy(old_policy); + // ElectronBindings is tracking node environments. + electron_bindings_->EnvironmentDestroyed(env); + if (lazy_tls->Get()) lazy_tls->Set(nullptr); } From 0337db2408b5dde7fae5619bed3b91cbf47fbfce Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Fri, 14 Jul 2023 23:43:26 +0900 Subject: [PATCH 4/4] fix: UAF crash when creating WebWorkerObserver --- shell/renderer/electron_renderer_client.cc | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/shell/renderer/electron_renderer_client.cc b/shell/renderer/electron_renderer_client.cc index a6d68e930fff7..04b9b0e03bb9a 100644 --- a/shell/renderer/electron_renderer_client.cc +++ b/shell/renderer/electron_renderer_client.cc @@ -158,19 +158,20 @@ void ElectronRendererClient::WorkerScriptReadyForEvaluationOnWorkerThread( // We do not create a Node.js environment in service or shared workers // owing to an inability to customize sandbox policies in these workers // given that they're run out-of-process. + // Also avoid creating a Node.js environment for worklet global scope + // created on the main thread. auto* ec = blink::ExecutionContext::From(context); - if (ec->IsServiceWorkerGlobalScope() || ec->IsSharedWorkerGlobalScope()) + if (ec->IsServiceWorkerGlobalScope() || ec->IsSharedWorkerGlobalScope() || + ec->IsMainThreadWorkletGlobalScope()) return; // This won't be correct for in-process child windows with webPreferences // that have a different value for nodeIntegrationInWorker if (base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kNodeIntegrationInWorker)) { - // WorkerScriptReadyForEvaluationOnWorkerThread can be invoked multiple - // times for the same thread, so we need to create a new observer each time - // this happens. We use a ThreadLocalOwnedPointer to ensure that the old - // observer for a given thread gets destructed when swapping with the new - // observer in WebWorkerObserver::Create. + auto* current = WebWorkerObserver::GetCurrent(); + if (current) + return; WebWorkerObserver::Create()->WorkerScriptReadyForEvaluation(context); } } @@ -178,7 +179,8 @@ void ElectronRendererClient::WorkerScriptReadyForEvaluationOnWorkerThread( void ElectronRendererClient::WillDestroyWorkerContextOnWorkerThread( v8::Local context) { auto* ec = blink::ExecutionContext::From(context); - if (ec->IsServiceWorkerGlobalScope() || ec->IsSharedWorkerGlobalScope()) + if (ec->IsServiceWorkerGlobalScope() || ec->IsSharedWorkerGlobalScope() || + ec->IsMainThreadWorkletGlobalScope()) return; // TODO(loc): Note that this will not be correct for in-process child windows