src: make structuredClone work for process.env

Fixes: #45380
PR-URL: #45698
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

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)                          \

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) {

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>();
   }

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

src/node_realm.cc

@@ -243,9 +243,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>();
   }
 

src/util.h

@@ -298,6 +298,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();
 };

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