Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations #27310

Closed
wants to merge 10 commits into from
Closed

Fix to #27211 - Refactor RealEnvStore methods to use libuv methods for env operations #27310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1501433
src: modified RealEnvStore::Get method to use libuv functions
devasci Apr 19, 2019
eef9b8b
src: modified RealEnvStore methods to use libuv functions
devasci Apr 19, 2019
7db9f03
src: Modified src/node_env_var.cc as per Joyee Cheung’s review comments
devasci Apr 19, 2019
6ae79f7
src: Modified to use 2 space for indentation.
devasci Apr 19, 2019
0271ee1
src: refactor RealEnvStore methods - review comments fixing - 1
devasci Apr 20, 2019
4d51c93
src: refactor RealEnvStore methods - review comments fixing - 2
devasci Apr 20, 2019
1d0aa6e
src: refactor RealEnvStore methods - review comments fixing - 2a
devasci Apr 20, 2019
b40d71e
src: refactor RealEnvStore methods - review comments fixing - 3
devasci Apr 22, 2019
88fedfc
src: refactor RealEnvStore methods - Joyee Cheung’s review remarks fixed
devasci Apr 24, 2019
614f357
src: handle windows hidden/read-only env keys in set/query methods
devasci Apr 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,8 +607,8 @@ class KVStore {
KVStore(KVStore&&) = delete;
KVStore& operator=(KVStore&&) = delete;

virtual v8::Local<v8::String> Get(v8::Isolate* isolate,
v8::Local<v8::String> key) const = 0;
virtual v8::MaybeLocal<v8::String> Get(v8::Isolate* isolate,
v8::Local<v8::String> key) const = 0;
virtual void Set(v8::Isolate* isolate,
v8::Local<v8::String> key,
v8::Local<v8::String> value) = 0;
Expand Down
3 changes: 2 additions & 1 deletion src/inspector_profiler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,8 @@ std::string GetCwd(Environment* env) {
void StartProfilers(Environment* env) {
Isolate* isolate = env->isolate();
Local<String> coverage_str = env->env_vars()->Get(
isolate, FIXED_ONE_BYTE_STRING(isolate, "NODE_V8_COVERAGE"));
isolate, FIXED_ONE_BYTE_STRING(isolate, "NODE_V8_COVERAGE"))
.FromMaybe(Local<String>());
if (!coverage_str.IsEmpty() && coverage_str->Length() > 0) {
CHECK_NULL(env->coverage_connection());
env->set_coverage_connection(std::make_unique<V8CoverageConnection>(env));
Expand Down
124 changes: 54 additions & 70 deletions src/node_env_var.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ using v8::Value;

class RealEnvStore final : public KVStore {
public:
Local<String> Get(Isolate* isolate, Local<String> key) const override;
MaybeLocal<String> Get(Isolate* isolate, Local<String> key) const override;
void Set(Isolate* isolate, Local<String> key, Local<String> value) override;
int32_t Query(Isolate* isolate, Local<String> key) const override;
void Delete(Isolate* isolate, Local<String> key) override;
Expand All @@ -45,7 +45,7 @@ class RealEnvStore final : public KVStore {

class MapKVStore final : public KVStore {
public:
Local<String> Get(Isolate* isolate, Local<String> key) const override;
MaybeLocal<String> Get(Isolate* isolate, Local<String> key) const override;
void Set(Isolate* isolate, Local<String> key, Local<String> value) override;
int32_t Query(Isolate* isolate, Local<String> key) const override;
void Delete(Isolate* isolate, Local<String> key) override;
Expand Down Expand Up @@ -79,93 +79,73 @@ void DateTimeConfigurationChangeNotification(Isolate* isolate, const T& key) {
}
}

Local<String> RealEnvStore::Get(Isolate* isolate,
Local<String> property) const {
MaybeLocal<String> RealEnvStore::Get(Isolate* isolate,
Local<String> property) const {
Mutex::ScopedLock lock(per_process::env_var_mutex);
#ifdef __POSIX__

node::Utf8Value key(isolate, property);
const char* val = getenv(*key);
if (val) {
return String::NewFromUtf8(isolate, val, NewStringType::kNormal)
.ToLocalChecked();
size_t init_sz = 256;
MaybeStackBuffer<char, 256> val;
int ret = uv_os_getenv(*key, *val, &init_sz);

if (ret == UV_ENOBUFS) {
// Buffer is not large enough, reallocate to the updated init_sz
// and fetch env value again.
val.AllocateSufficientStorage(init_sz);
ret = uv_os_getenv(*key, *val, &init_sz);
}
#else // _WIN32
node::TwoByteValue key(isolate, property);
WCHAR buffer[32767]; // The maximum size allowed for environment variables.
SetLastError(ERROR_SUCCESS);
DWORD result = GetEnvironmentVariableW(
reinterpret_cast<WCHAR*>(*key), buffer, arraysize(buffer));
// If result >= sizeof buffer the buffer was too small. That should never
// happen. If result == 0 and result != ERROR_SUCCESS the variable was not
// found.
if ((result > 0 || GetLastError() == ERROR_SUCCESS) &&
result < arraysize(buffer)) {
const uint16_t* two_byte_buffer = reinterpret_cast<const uint16_t*>(buffer);
v8::MaybeLocal<String> rc = String::NewFromTwoByte(
isolate, two_byte_buffer, NewStringType::kNormal);
if (rc.IsEmpty()) {
isolate->ThrowException(ERR_STRING_TOO_LONG(isolate));
return Local<String>();
}
return rc.ToLocalChecked();

if (ret >= 0) { // Env key value fetch success.
MaybeLocal<String> value_string =
String::NewFromUtf8(isolate, *val, NewStringType::kNormal, init_sz);
return value_string;
}
#endif
return Local<String>();

return MaybeLocal<String>();
}

void RealEnvStore::Set(Isolate* isolate,
Local<String> property,
Local<String> value) {
Mutex::ScopedLock lock(per_process::env_var_mutex);
#ifdef __POSIX__

node::Utf8Value key(isolate, property);
node::Utf8Value val(isolate, value);
setenv(*key, *val, 1);
#else // _WIN32
node::TwoByteValue key(isolate, property);
node::TwoByteValue val(isolate, value);
WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
// Environment variables that start with '=' are read-only.
if (key_ptr[0] != L'=') {
Copy link
Member

@joyeecheung joyeecheung Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this matters but I think we do need to keep this (with key[0] here).

SetEnvironmentVariableW(key_ptr, reinterpret_cast<WCHAR*>(*val));
}

devasci marked this conversation as resolved.
Show resolved Hide resolved
#ifdef _WIN32
if (key[0] == L'=') return;
#endif
uv_os_setenv(*key, *val);
DateTimeConfigurationChangeNotification(isolate, key);
}

int32_t RealEnvStore::Query(Isolate* isolate, Local<String> property) const {
Mutex::ScopedLock lock(per_process::env_var_mutex);
#ifdef __POSIX__

node::Utf8Value key(isolate, property);
if (getenv(*key)) return 0;
#else // _WIN32
node::TwoByteValue key(isolate, property);
WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
SetLastError(ERROR_SUCCESS);
if (GetEnvironmentVariableW(key_ptr, nullptr, 0) > 0 ||
GetLastError() == ERROR_SUCCESS) {
if (key_ptr[0] == L'=') {
Copy link
Member

@joyeecheung joyeecheung Apr 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly we need to keep this.

// Environment variables that start with '=' are hidden and read-only.
return static_cast<int32_t>(v8::ReadOnly) |
static_cast<int32_t>(v8::DontDelete) |
static_cast<int32_t>(v8::DontEnum);
}
return 0;
}
#ifdef _WIN32
if (key[0] == L'=')
return static_cast<int32_t>(v8::ReadOnly) |
static_cast<int32_t>(v8::DontDelete) |
static_cast<int32_t>(v8::DontEnum);
#endif
return -1;

char val[2];
size_t init_sz = sizeof(val);
int ret = uv_os_getenv(*key, val, &init_sz);

if (ret == UV_ENOENT) {
return -1;
}

return 0;
}

void RealEnvStore::Delete(Isolate* isolate, Local<String> property) {
Mutex::ScopedLock lock(per_process::env_var_mutex);
#ifdef __POSIX__

node::Utf8Value key(isolate, property);
unsetenv(*key);
#else
node::TwoByteValue key(isolate, property);
WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
SetEnvironmentVariableW(key_ptr, nullptr);
#endif
uv_os_unsetenv(*key);
DateTimeConfigurationChangeNotification(isolate, key);
}

Expand Down Expand Up @@ -231,19 +211,20 @@ std::shared_ptr<KVStore> KVStore::Clone(v8::Isolate* isolate) const {
for (uint32_t i = 0; i < keys_length; i++) {
Local<Value> key = keys->Get(context, i).ToLocalChecked();
CHECK(key->IsString());
copy->Set(isolate, key.As<String>(), Get(isolate, key.As<String>()));
copy->Set(isolate,
key.As<String>(),
Get(isolate, key.As<String>()).ToLocalChecked());
}
return copy;
}

Local<String> MapKVStore::Get(Isolate* isolate, Local<String> key) const {
MaybeLocal<String> MapKVStore::Get(Isolate* isolate, Local<String> key) const {
Mutex::ScopedLock lock(mutex_);
Utf8Value str(isolate, key);
auto it = map_.find(std::string(*str, str.length()));
if (it == map_.end()) return Local<String>();
return String::NewFromUtf8(isolate, it->second.data(),
NewStringType::kNormal, it->second.size())
.ToLocalChecked();
NewStringType::kNormal, it->second.size());
}

void MapKVStore::Set(Isolate* isolate, Local<String> key, Local<String> value) {
Expand Down Expand Up @@ -323,8 +304,11 @@ static void EnvGetter(Local<Name> property,
return info.GetReturnValue().SetUndefined();
}
CHECK(property->IsString());
info.GetReturnValue().Set(
env->env_vars()->Get(env->isolate(), property.As<String>()));
MaybeLocal<String> value_string =
env->env_vars()->Get(env->isolate(), property.As<String>());
if (!value_string.IsEmpty()) {
info.GetReturnValue().Set(value_string.ToLocalChecked());
}
}

static void EnvSetter(Local<Name> property,
Expand Down