From b88ee545f24765a889592db5ca0011770d220203 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sun, 11 Dec 2022 13:54:15 +0100 Subject: [PATCH] src: make structuredClone work for process.env Fixes: https://github.com/nodejs/node/issues/45380 PR-URL: https://github.com/nodejs/node/pull/45698 Reviewed-By: Anna Henningsen Reviewed-By: Yagiz Nizipli --- src/env_properties.h | 2 ++ src/node_env_var.cc | 35 ++++++++++++++++++++++++++----- src/node_messaging.cc | 32 ++++++++++++++++++++++++++-- src/node_process.h | 4 ++-- src/node_realm.cc | 7 ++++--- src/util.h | 3 +++ test/parallel/test-process-env.js | 8 +++++++ 7 files changed, 79 insertions(+), 12 deletions(-) diff --git a/src/env_properties.h b/src/env_properties.h index 7282d1b4e50946..795c3bb5e9b49a 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -334,6 +334,8 @@ V(contextify_global_template, v8::ObjectTemplate) \ V(contextify_wrapper_template, v8::ObjectTemplate) \ V(compiled_fn_entry_template, v8::ObjectTemplate) \ + V(env_proxy_template, v8::ObjectTemplate) \ + V(env_proxy_ctor_template, v8::FunctionTemplate) \ V(dir_instance_template, v8::ObjectTemplate) \ V(fd_constructor_template, v8::ObjectTemplate) \ V(fdclose_constructor_template, v8::ObjectTemplate) \ diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 98f1ed07998e13..d6a8c803d605c2 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -13,7 +13,7 @@ using v8::Boolean; using v8::Context; using v8::DontDelete; using v8::DontEnum; -using v8::EscapableHandleScope; +using v8::FunctionTemplate; using v8::HandleScope; using v8::Integer; using v8::Isolate; @@ -316,6 +316,26 @@ Maybe KVStore::AssignFromObject(Local context, return Just(true); } +// TODO(bnoordhuis) Not super efficient but called infrequently. Not worth +// the trouble yet of specializing for RealEnvStore and MapKVStore. +Maybe KVStore::AssignToObject(v8::Isolate* isolate, + v8::Local context, + v8::Local object) { + HandleScope scope(isolate); + Local keys = Enumerate(isolate); + uint32_t keys_length = keys->Length(); + for (uint32_t i = 0; i < keys_length; i++) { + Local key; + Local value; + bool ok = keys->Get(context, i).ToLocal(&key); + ok = ok && key->IsString(); + ok = ok && Get(isolate, key.As()).ToLocal(&value); + ok = ok && object->Set(context, key, value).To(&ok); + if (!ok) return Nothing(); + } + return Just(true); +} + static void EnvGetter(Local property, const PropertyCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); @@ -436,9 +456,13 @@ static void EnvDefiner(Local property, } } -MaybeLocal CreateEnvVarProxy(Local context, Isolate* isolate) { - EscapableHandleScope scope(isolate); - Local env_proxy_template = ObjectTemplate::New(isolate); +void CreateEnvProxyTemplate(Isolate* isolate, IsolateData* isolate_data) { + HandleScope scope(isolate); + if (!isolate_data->env_proxy_template().IsEmpty()) return; + Local env_proxy_ctor_template = + FunctionTemplate::New(isolate); + Local env_proxy_template = + ObjectTemplate::New(isolate, env_proxy_ctor_template); env_proxy_template->SetHandler(NamedPropertyHandlerConfiguration( EnvGetter, EnvSetter, @@ -449,7 +473,8 @@ MaybeLocal CreateEnvVarProxy(Local context, Isolate* isolate) { nullptr, Local(), PropertyHandlerFlags::kHasNoSideEffect)); - return scope.EscapeMaybe(env_proxy_template->NewInstance(context)); + isolate_data->set_env_proxy_template(env_proxy_template); + isolate_data->set_env_proxy_ctor_template(env_proxy_ctor_template); } void RegisterEnvVarExternalReferences(ExternalReferenceRegistry* registry) { diff --git a/src/node_messaging.cc b/src/node_messaging.cc index f88270fc75de91..009ac0056c5486 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -42,6 +42,10 @@ namespace node { using BaseObjectList = std::vector>; +// Hack to have WriteHostObject inform ReadHostObject that the value +// should be treated as a regular JS object. Used to transfer process.env. +static const uint32_t kNormalObject = static_cast(-1); + BaseObject::TransferMode BaseObject::GetTransferMode() const { return BaseObject::TransferMode::kUntransferable; } @@ -98,8 +102,17 @@ class DeserializerDelegate : public ValueDeserializer::Delegate { uint32_t id; if (!deserializer->ReadUint32(&id)) return MaybeLocal(); - CHECK_LT(id, host_objects_.size()); - return host_objects_[id]->object(isolate); + if (id != kNormalObject) { + CHECK_LT(id, host_objects_.size()); + return host_objects_[id]->object(isolate); + } + EscapableHandleScope scope(isolate); + Local context = isolate->GetCurrentContext(); + Local object; + if (!deserializer->ReadValue(context).ToLocal(&object)) + return MaybeLocal(); + CHECK(object->IsObject()); + return scope.Escape(object.As()); } MaybeLocal GetSharedArrayBufferFromId( @@ -293,6 +306,21 @@ class SerializerDelegate : public ValueSerializer::Delegate { BaseObjectPtr { Unwrap(object) }); } + // Convert process.env to a regular object. + auto env_proxy_ctor_template = env_->env_proxy_ctor_template(); + if (!env_proxy_ctor_template.IsEmpty() && + env_proxy_ctor_template->HasInstance(object)) { + HandleScope scope(isolate); + // TODO(bnoordhuis) Prototype-less object in case process.env contains + // a "__proto__" key? process.env has a prototype with concomitant + // methods like toString(). It's probably confusing if that gets lost + // in transmission. + Local normal_object = Object::New(isolate); + env_->env_vars()->AssignToObject(isolate, env_->context(), normal_object); + serializer->WriteUint32(kNormalObject); // Instead of a BaseObject. + return serializer->WriteValue(env_->context(), normal_object); + } + ThrowDataCloneError(env_->clone_unsupported_type_str()); return Nothing(); } diff --git a/src/node_process.h b/src/node_process.h index 8abcd16ca606d6..8065378960887d 100644 --- a/src/node_process.h +++ b/src/node_process.h @@ -10,12 +10,12 @@ namespace node { class Environment; +class IsolateData; class MemoryTracker; class ExternalReferenceRegistry; class Realm; -v8::MaybeLocal CreateEnvVarProxy(v8::Local context, - v8::Isolate* isolate); +void CreateEnvProxyTemplate(v8::Isolate* isolate, IsolateData* isolate_data); // Most of the time, it's best to use `console.error` to write // to the process.stderr stream. However, in some cases, such as diff --git a/src/node_realm.cc b/src/node_realm.cc index 0a19c89f53a7f3..1ce94273fcdb01 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -275,9 +275,10 @@ MaybeLocal Realm::BootstrapNode() { } Local env_string = FIXED_ONE_BYTE_STRING(isolate_, "env"); - Local env_var_proxy; - if (!CreateEnvVarProxy(context(), isolate_).ToLocal(&env_var_proxy) || - process_object()->Set(context(), env_string, env_var_proxy).IsNothing()) { + Local env_proxy; + CreateEnvProxyTemplate(isolate_, env_->isolate_data()); + if (!env_->env_proxy_template()->NewInstance(context()).ToLocal(&env_proxy) || + process_object()->Set(context(), env_string, env_proxy).IsNothing()) { return MaybeLocal(); } diff --git a/src/util.h b/src/util.h index 7e02c232de4f1b..896a6df4d9196c 100644 --- a/src/util.h +++ b/src/util.h @@ -297,6 +297,9 @@ class KVStore { virtual std::shared_ptr Clone(v8::Isolate* isolate) const; virtual v8::Maybe AssignFromObject(v8::Local context, v8::Local entries); + v8::Maybe AssignToObject(v8::Isolate* isolate, + v8::Local context, + v8::Local object); static std::shared_ptr CreateMapKVStore(); }; diff --git a/test/parallel/test-process-env.js b/test/parallel/test-process-env.js index f0bff13d4c81f1..f20be5a194dbf9 100644 --- a/test/parallel/test-process-env.js +++ b/test/parallel/test-process-env.js @@ -105,6 +105,14 @@ if (common.isWindows) { assert.ok(keys.length > 0); } +// https://github.com/nodejs/node/issues/45380 +{ + const env = structuredClone(process.env); + // deepEqual(), not deepStrictEqual(), because of different prototypes. + // eslint-disable-next-line no-restricted-properties + assert.deepEqual(env, process.env); +} + // Setting environment variables on Windows with empty names should not cause // an assertion failure. // https://github.com/nodejs/node/issues/32920