Skip to content

Commit

Permalink
src: make structuredClone work for process.env
Browse files Browse the repository at this point in the history
Fixes: #45380
PR-URL: #45698
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
  • Loading branch information
bnoordhuis authored and danielleadams committed Jan 3, 2023
1 parent 1019209 commit b88ee54
Show file tree
Hide file tree
Showing 7 changed files with 79 additions and 12 deletions.
2 changes: 2 additions & 0 deletions src/env_properties.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
35 changes: 30 additions & 5 deletions src/node_env_var.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand Down
32 changes: 30 additions & 2 deletions src/node_messaging.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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>();
}
Expand Down
4 changes: 2 additions & 2 deletions src/node_process.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions src/node_realm.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
}

Expand Down
3 changes: 3 additions & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
};
Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-process-env.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit b88ee54

Please sign in to comment.