diff options
author | Ben Noordhuis <info@bnoordhuis.nl> | 2022-12-11 13:54:15 +0100 |
---|---|---|
committer | Danielle Adams <adamzdanielle@gmail.com> | 2023-01-04 20:31:55 -0500 |
commit | f995a060ad14d54ba3e9068c824a6bf7f6a4595d (patch) | |
tree | 53a92b40e0500262b15710b0d5758ae8c1c77fe1 | |
parent | ea0e41ef5c47d60e22fb7279b47433c4ae9a2596 (diff) | |
download | node-new-f995a060ad14d54ba3e9068c824a6bf7f6a4595d.tar.gz |
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 <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
-rw-r--r-- | src/env_properties.h | 2 | ||||
-rw-r--r-- | src/node_env_var.cc | 35 | ||||
-rw-r--r-- | src/node_messaging.cc | 32 | ||||
-rw-r--r-- | src/node_process.h | 4 | ||||
-rw-r--r-- | src/node_realm.cc | 7 | ||||
-rw-r--r-- | src/util.h | 3 | ||||
-rw-r--r-- | 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 7282d1b4e5..795c3bb5e9 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 98f1ed0799..d6a8c803d6 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<bool> KVStore::AssignFromObject(Local<Context> context, return Just(true); } +// TODO(bnoordhuis) Not super efficient but called infrequently. Not worth +// the trouble yet of specializing for RealEnvStore and MapKVStore. +Maybe<bool> KVStore::AssignToObject(v8::Isolate* isolate, + v8::Local<v8::Context> context, + v8::Local<v8::Object> object) { + HandleScope scope(isolate); + Local<Array> keys = Enumerate(isolate); + uint32_t keys_length = keys->Length(); + for (uint32_t i = 0; i < keys_length; i++) { + Local<Value> key; + Local<String> value; + bool ok = keys->Get(context, i).ToLocal(&key); + ok = ok && key->IsString(); + ok = ok && Get(isolate, key.As<String>()).ToLocal(&value); + ok = ok && object->Set(context, key, value).To(&ok); + if (!ok) return Nothing<bool>(); + } + return Just(true); +} + static void EnvGetter(Local<Name> property, const PropertyCallbackInfo<Value>& info) { Environment* env = Environment::GetCurrent(info); @@ -436,9 +456,13 @@ static void EnvDefiner(Local<Name> property, } } -MaybeLocal<Object> CreateEnvVarProxy(Local<Context> context, Isolate* isolate) { - EscapableHandleScope scope(isolate); - Local<ObjectTemplate> 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<FunctionTemplate> env_proxy_ctor_template = + FunctionTemplate::New(isolate); + Local<ObjectTemplate> env_proxy_template = + ObjectTemplate::New(isolate, env_proxy_ctor_template); env_proxy_template->SetHandler(NamedPropertyHandlerConfiguration( EnvGetter, EnvSetter, @@ -449,7 +473,8 @@ MaybeLocal<Object> CreateEnvVarProxy(Local<Context> context, Isolate* isolate) { nullptr, Local<Value>(), 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 f88270fc75..009ac0056c 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -42,6 +42,10 @@ namespace node { using BaseObjectList = std::vector<BaseObjectPtr<BaseObject>>; +// 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<uint32_t>(-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<Object>(); - 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> context = isolate->GetCurrentContext(); + Local<Value> object; + if (!deserializer->ReadValue(context).ToLocal(&object)) + return MaybeLocal<Object>(); + CHECK(object->IsObject()); + return scope.Escape(object.As<Object>()); } MaybeLocal<SharedArrayBuffer> GetSharedArrayBufferFromId( @@ -293,6 +306,21 @@ class SerializerDelegate : public ValueSerializer::Delegate { BaseObjectPtr<BaseObject> { Unwrap<BaseObject>(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<Object> 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<bool>(); } diff --git a/src/node_process.h b/src/node_process.h index 8abcd16ca6..8065378960 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<v8::Object> CreateEnvVarProxy(v8::Local<v8::Context> 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 0a19c89f53..1ce94273fc 100644 --- a/src/node_realm.cc +++ b/src/node_realm.cc @@ -275,9 +275,10 @@ MaybeLocal<Value> Realm::BootstrapNode() { } Local<String> env_string = FIXED_ONE_BYTE_STRING(isolate_, "env"); - Local<Object> env_var_proxy; - if (!CreateEnvVarProxy(context(), isolate_).ToLocal(&env_var_proxy) || - process_object()->Set(context(), env_string, env_var_proxy).IsNothing()) { + Local<Object> 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<Value>(); } diff --git a/src/util.h b/src/util.h index 7e02c232de..896a6df4d9 100644 --- a/src/util.h +++ b/src/util.h @@ -297,6 +297,9 @@ class KVStore { virtual std::shared_ptr<KVStore> Clone(v8::Isolate* isolate) const; virtual v8::Maybe<bool> AssignFromObject(v8::Local<v8::Context> context, v8::Local<v8::Object> entries); + v8::Maybe<bool> AssignToObject(v8::Isolate* isolate, + v8::Local<v8::Context> context, + v8::Local<v8::Object> object); static std::shared_ptr<KVStore> CreateMapKVStore(); }; diff --git a/test/parallel/test-process-env.js b/test/parallel/test-process-env.js index f0bff13d4c..f20be5a194 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 |