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.

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

src: make copies of startup environment variables #11051

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
56 changes: 34 additions & 22 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ static const char* trace_enabled_categories = nullptr;

#if defined(NODE_HAVE_I18N_SUPPORT)
// Path to ICU data (for i18n / Intl)
static const char* icu_data_dir = nullptr;
static std::string icu_data_dir; // NOLINT(runtime/string)
#endif

// used by C++ modules as well
Expand Down Expand Up @@ -189,7 +189,7 @@ bool trace_warnings = false;
bool config_preserve_symlinks = false;

// Set in node.cc by ParseArgs when --redirect-warnings= is used.
const char* config_warning_file;
std::string config_warning_file; // NOLINT(runtime/string)

bool v8_initialized = false;

Expand Down Expand Up @@ -924,12 +924,21 @@ Local<Value> UVException(Isolate* isolate,


// Look up environment variable unless running as setuid root.
inline const char* secure_getenv(const char* key) {
inline bool SafeGetenv(const char* key, std::string* text) {
#ifndef _WIN32
if (getuid() != geteuid() || getgid() != getegid())
return nullptr;
// TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE)
// is non-zero on Linux.
if (getuid() != geteuid() || getgid() != getegid()) {
text->clear();
return false;
}
#endif
return getenv(key);
if (const char* value = getenv(key)) {
*text = value;
return true;
}
text->clear();
return false;
}


Expand Down Expand Up @@ -3089,11 +3098,11 @@ void SetupProcessObject(Environment* env,
#if defined(NODE_HAVE_I18N_SUPPORT) && defined(U_ICU_VERSION)
// ICU-related versions are now handled on the js side, see bootstrap_node.js

if (icu_data_dir != nullptr) {
if (!icu_data_dir.empty()) {
// Did the user attempt (via env var or parameter) to set an ICU path?
READONLY_PROPERTY(process,
"icu_data_dir",
OneByteString(env->isolate(), icu_data_dir));
OneByteString(env->isolate(), icu_data_dir.c_str()));
}
#endif

Expand Down Expand Up @@ -3741,7 +3750,7 @@ static void ParseArgs(int* argc,
#endif /* HAVE_OPENSSL */
#if defined(NODE_HAVE_I18N_SUPPORT)
} else if (strncmp(arg, "--icu-data-dir=", 15) == 0) {
icu_data_dir = arg + 15;
icu_data_dir.assign(arg, 15);
Copy link
Contributor

Choose a reason for hiding this comment

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

When converting openssl_config to std::string, I discovered this copies the first 15 chars of arg into target, it needs to be icu_data_dir.assign(arg + 15); or something of the like. @srl295 Since tests pass, this means that this command line option has no test coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ai, obvious bug in retrospect. #11255

#endif
} else if (strcmp(arg, "--expose-internals") == 0 ||
strcmp(arg, "--expose_internals") == 0) {
Expand Down Expand Up @@ -4228,13 +4237,14 @@ void Init(int* argc,
#endif

// Allow for environment set preserving symlinks.
if (auto preserve_symlinks = secure_getenv("NODE_PRESERVE_SYMLINKS")) {
config_preserve_symlinks = (*preserve_symlinks == '1');
{
std::string text;
config_preserve_symlinks =
SafeGetenv("NODE_PRESERVE_SYMLINKS", &text) && text[0] == '1';
}

if (auto redirect_warnings = secure_getenv("NODE_REDIRECT_WARNINGS")) {
config_warning_file = redirect_warnings;
}
if (config_warning_file.empty())
SafeGetenv("NODE_REDIRECT_WARNINGS", &config_warning_file);

// Parse a few arguments which are specific to Node.
int v8_argc;
Expand Down Expand Up @@ -4262,12 +4272,11 @@ void Init(int* argc,
#endif

#if defined(NODE_HAVE_I18N_SUPPORT)
if (icu_data_dir == nullptr) {
// if the parameter isn't given, use the env variable.
icu_data_dir = secure_getenv("NODE_ICU_DATA");
}
// If the parameter isn't given, use the env variable.
if (icu_data_dir.empty())
SafeGetenv("NODE_ICU_DATA", &icu_data_dir);
// Initialize ICU.
// If icu_data_dir is nullptr here, it will load the 'minimal' data.
// If icu_data_dir is empty here, it will load the 'minimal' data.
if (!i18n::InitializeICUDirectory(icu_data_dir)) {
FatalError(nullptr, "Could not initialize ICU "
"(check NODE_ICU_DATA or --icu-data-dir parameters)");
Expand Down Expand Up @@ -4532,8 +4541,11 @@ int Start(int argc, char** argv) {
Init(&argc, const_cast<const char**>(argv), &exec_argc, &exec_argv);

#if HAVE_OPENSSL
if (const char* extra = secure_getenv("NODE_EXTRA_CA_CERTS"))
crypto::UseExtraCaCerts(extra);
{
std::string extra_ca_certs;
if (SafeGetenv("NODE_EXTRA_CA_CERTS", &extra_ca_certs))
crypto::UseExtraCaCerts(extra_ca_certs);
}
#ifdef NODE_FIPS_MODE
// In the case of FIPS builds we should make sure
// the random source is properly initialized first.
Expand All @@ -4542,7 +4554,7 @@ int Start(int argc, char** argv) {
// V8 on Windows doesn't have a good source of entropy. Seed it from
// OpenSSL's pool.
V8::SetEntropySource(crypto::EntropySource);
#endif
#endif // HAVE_OPENSSL

v8_platform.Initialize(v8_thread_pool_size);
// Enable tracing when argv has --trace-events-enabled.
Expand Down
7 changes: 4 additions & 3 deletions src/node_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ void InitConfig(Local<Object> target,
if (config_preserve_symlinks)
READONLY_BOOLEAN_PROPERTY("preserveSymlinks");

if (config_warning_file != nullptr) {
if (!config_warning_file.empty()) {
Local<String> name = OneByteString(env->isolate(), "warningFile");
Local<String> value = String::NewFromUtf8(env->isolate(),
config_warning_file,
v8::NewStringType::kNormal)
config_warning_file.data(),
v8::NewStringType::kNormal,
config_warning_file.size())
.ToLocalChecked();
target->DefineOwnProperty(env->context(), name, value).FromJust();
}
Expand Down
12 changes: 6 additions & 6 deletions src/node_i18n.cc
Original file line number Diff line number Diff line change
Expand Up @@ -404,12 +404,8 @@ static void GetVersion(const FunctionCallbackInfo<Value>& args) {
}
}

bool InitializeICUDirectory(const char* icu_data_path) {
if (icu_data_path != nullptr) {
flag_icu_data_dir = true;
u_setDataDirectory(icu_data_path);
return true; // no error
} else {
bool InitializeICUDirectory(const std::string& path) {
if (path.empty()) {
UErrorCode status = U_ZERO_ERROR;
#ifdef NODE_HAVE_SMALL_ICU
// install the 'small' data.
Expand All @@ -418,6 +414,10 @@ bool InitializeICUDirectory(const char* icu_data_path) {
// no small data, so nothing to do.
#endif // !NODE_HAVE_SMALL_ICU
return (status == U_ZERO_ERROR);
} else {
flag_icu_data_dir = true;
u_setDataDirectory(path.c_str());
return true; // No error.
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/node_i18n.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "node.h"
#include <string>

#if defined(NODE_HAVE_I18N_SUPPORT)

Expand All @@ -13,7 +14,7 @@ extern bool flag_icu_data_dir;

namespace i18n {

bool InitializeICUDirectory(const char* icu_data_path);
bool InitializeICUDirectory(const std::string& path);

int32_t ToASCII(MaybeStackBuffer<char>* buf,
const char* input,
Expand Down
4 changes: 3 additions & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include <stdint.h>
#include <stdlib.h>

#include <string>

struct sockaddr;

// Variation on NODE_DEFINE_CONSTANT that sets a String value.
Expand Down Expand Up @@ -45,7 +47,7 @@ extern bool config_preserve_symlinks;
// Set in node.cc by ParseArgs when --redirect-warnings= is used.
// Used to redirect warning output to a file rather than sending
// it to stderr.
extern const char* config_warning_file;
extern std::string config_warning_file; // NOLINT(runtime/string)

// Tells whether it is safe to call v8::Isolate::GetCurrent().
extern bool v8_initialized;
Expand Down