Skip to content

Commit

Permalink
src: move per-process global variables into node::per_process
Browse files Browse the repository at this point in the history
So that it's easier to tell whether we are manipulating per-process
global states that may need to be treated with care to avoid races.

Also added comments about these variables and moved some of them
to a more suitable compilation unit:

- Move `v8_initialized` to `util.h` since it's only used in
  `util.cc` and `node.cc`
- Rename `process_mutex` to `tty_mutex` and move it into
  `node_errors.cc` since that's the only place it's used
  to guard the tty.
- Move `per_process_opts_mutex` and `per_process_opts`
  into `node_options.h` and rename them to
  `per_process::cli_options[_mutex]`
- Rename `node_isolate[_mutex]` to `per_process::main_isolate[_mutex]`

PR-URL: #25302
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
joyeecheung authored and BridgeAR committed Jan 17, 2019
1 parent 69d8e60 commit c6adf4b
Show file tree
Hide file tree
Showing 17 changed files with 126 additions and 92 deletions.
3 changes: 2 additions & 1 deletion src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ IsolateData::IsolateData(Isolate* isolate,
if (platform_ != nullptr)
platform_->RegisterIsolate(isolate_, event_loop);

options_.reset(new PerIsolateOptions(*per_process_opts->per_isolate));
options_.reset(
new PerIsolateOptions(*(per_process::cli_options->per_isolate)));

// Create string and private symbol properties as internalized one byte
// strings after the platform is properly initialized.
Expand Down
90 changes: 51 additions & 39 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -139,21 +139,31 @@ using v8::Undefined;
using v8::V8;
using v8::Value;

namespace per_process {
// Tells whether --prof is passed.
// TODO(joyeecheung): move env->options()->prof_process to
// per_process::cli_options.prof_process and use that instead.
static bool v8_is_profiling = false;

// Bit flag used to track security reverts (see node_revert.h)
unsigned int reverted = 0;
// TODO(joyeecheung): these are no longer necessary. Remove them.
// See: https://github.com/nodejs/node/pull/25302#discussion_r244924196
// Isolate on the main thread
static Mutex main_isolate_mutex;
static Isolate* main_isolate;

// node_revert.h
// Bit flag used to track security reverts.
unsigned int reverted_cve = 0;

// util.h
// Tells whether the per-process V8::Initialize() is called and
// if it is safe to call v8::Isolate::GetCurrent().
bool v8_initialized = false;

// node_internals.h
// process-relative uptime base, initialized at start-up
double prog_start_time;

Mutex per_process_opts_mutex;
std::shared_ptr<PerProcessOptions> per_process_opts {
new PerProcessOptions() };
static Mutex node_isolate_mutex;
static Isolate* node_isolate;
} // namespace per_process

// Ensures that __metadata trace events are only emitted
// when tracing is enabled.
Expand Down Expand Up @@ -269,14 +279,15 @@ static struct {
#endif // HAVE_INSPECTOR

void StartTracingAgent() {
if (per_process_opts->trace_event_categories.empty()) {
if (per_process::cli_options->trace_event_categories.empty()) {
tracing_file_writer_ = tracing_agent_->DefaultHandle();
} else {
tracing_file_writer_ = tracing_agent_->AddClient(
ParseCommaSeparatedSet(per_process_opts->trace_event_categories),
ParseCommaSeparatedSet(
per_process::cli_options->trace_event_categories),
std::unique_ptr<tracing::AsyncTraceWriter>(
new tracing::NodeTraceWriter(
per_process_opts->trace_event_file_pattern)),
per_process::cli_options->trace_event_file_pattern)),
tracing::Agent::kUseDefaultCategories);
}
}
Expand Down Expand Up @@ -504,7 +515,7 @@ const char* signo_string(int signo) {
}

void* ArrayBufferAllocator::Allocate(size_t size) {
if (zero_fill_field_ || per_process_opts->zero_fill_all_buffers)
if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers)
return UncheckedCalloc(size);
else
return UncheckedMalloc(size);
Expand Down Expand Up @@ -1254,12 +1265,12 @@ void ProcessArgv(std::vector<std::string>* args,
{
// TODO(addaleax): The mutex here should ideally be held during the
// entire function, but that doesn't play well with the exit() calls below.
Mutex::ScopedLock lock(per_process_opts_mutex);
Mutex::ScopedLock lock(per_process::cli_options_mutex);
options_parser::PerProcessOptionsParser::instance.Parse(
args,
exec_args,
&v8_args,
per_process_opts.get(),
per_process::cli_options.get(),
is_env ? kAllowedInEnvironment : kDisallowedInEnvironment,
&errors);
}
Expand All @@ -1271,20 +1282,20 @@ void ProcessArgv(std::vector<std::string>* args,
exit(9);
}

if (per_process_opts->print_version) {
if (per_process::cli_options->print_version) {
printf("%s\n", NODE_VERSION);
exit(0);
}

if (per_process_opts->print_v8_help) {
if (per_process::cli_options->print_v8_help) {
V8::SetFlagsFromString("--help", 6);
exit(0);
}

for (const std::string& cve : per_process_opts->security_reverts)
for (const std::string& cve : per_process::cli_options->security_reverts)
Revert(cve.c_str());

auto env_opts = per_process_opts->per_isolate->per_env;
auto env_opts = per_process::cli_options->per_isolate->per_env;
if (std::find(v8_args.begin(), v8_args.end(),
"--abort-on-uncaught-exception") != v8_args.end() ||
std::find(v8_args.begin(), v8_args.end(),
Expand All @@ -1297,14 +1308,14 @@ void ProcessArgv(std::vector<std::string>* args,
// behavior but it could also interfere with the user's intentions in ways
// we fail to anticipate. Dillema.
if (std::find(v8_args.begin(), v8_args.end(), "--prof") != v8_args.end()) {
v8_is_profiling = true;
per_process::v8_is_profiling = true;
}

#ifdef __POSIX__
// Block SIGPROF signals when sleeping in epoll_wait/kevent/etc. Avoids the
// performance penalty of frequent EINTR wakeups when the profiler is running.
// Only do this for v8.log profiling, as it breaks v8::CpuProfiler users.
if (v8_is_profiling) {
if (per_process::v8_is_profiling) {
uv_loop_configure(uv_default_loop(), UV_LOOP_BLOCK_SIGNAL, SIGPROF);
}
#endif
Expand Down Expand Up @@ -1333,7 +1344,7 @@ void ProcessArgv(std::vector<std::string>* args,
void Init(std::vector<std::string>* argv,
std::vector<std::string>* exec_argv) {
// Initialize prog_start_time to get relative uptime.
prog_start_time = static_cast<double>(uv_now(uv_default_loop()));
per_process::prog_start_time = static_cast<double>(uv_now(uv_default_loop()));

// Register built-in modules
binding::RegisterBuiltinModules();
Expand All @@ -1349,7 +1360,7 @@ void Init(std::vector<std::string>* argv,
#endif

std::shared_ptr<EnvironmentOptions> default_env_options =
per_process_opts->per_isolate->per_env;
per_process::cli_options->per_isolate->per_env;
{
std::string text;
default_env_options->pending_deprecation =
Expand Down Expand Up @@ -1378,7 +1389,7 @@ void Init(std::vector<std::string>* argv,
}

#if HAVE_OPENSSL
std::string* openssl_config = &per_process_opts->openssl_config;
std::string* openssl_config = &per_process::cli_options->openssl_config;
if (openssl_config->empty()) {
credentials::SafeGetenv("OPENSSL_CONF", openssl_config);
}
Expand Down Expand Up @@ -1412,16 +1423,17 @@ void Init(std::vector<std::string>* argv,
ProcessArgv(argv, exec_argv, false);

// Set the process.title immediately after processing argv if --title is set.
if (!per_process_opts->title.empty())
uv_set_process_title(per_process_opts->title.c_str());
if (!per_process::cli_options->title.empty())
uv_set_process_title(per_process::cli_options->title.c_str());

#if defined(NODE_HAVE_I18N_SUPPORT)
// If the parameter isn't given, use the env variable.
if (per_process_opts->icu_data_dir.empty())
credentials::SafeGetenv("NODE_ICU_DATA", &per_process_opts->icu_data_dir);
if (per_process::cli_options->icu_data_dir.empty())
credentials::SafeGetenv("NODE_ICU_DATA",
&per_process::cli_options->icu_data_dir);
// Initialize ICU.
// If icu_data_dir is empty here, it will load the 'minimal' data.
if (!i18n::InitializeICUDirectory(per_process_opts->icu_data_dir)) {
if (!i18n::InitializeICUDirectory(per_process::cli_options->icu_data_dir)) {
fprintf(stderr,
"%s: could not initialize ICU "
"(check NODE_ICU_DATA or --icu-data-dir parameters)\n",
Expand Down Expand Up @@ -1582,7 +1594,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
std::vector<std::string> args(argv, argv + argc);
std::vector<std::string> exec_args(exec_argv, exec_argv + exec_argc);
Environment* env = new Environment(isolate_data, context);
env->Start(args, exec_args, v8_is_profiling);
env->Start(args, exec_args, per_process::v8_is_profiling);
return env;
}

Expand Down Expand Up @@ -1656,7 +1668,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
Local<Context> context = NewContext(isolate);
Context::Scope context_scope(context);
Environment env(isolate_data, context);
env.Start(args, exec_args, v8_is_profiling);
env.Start(args, exec_args, per_process::v8_is_profiling);

const char* path = args.size() > 1 ? args[1].c_str() : nullptr;
StartInspector(&env, path);
Expand Down Expand Up @@ -1763,9 +1775,9 @@ inline int Start(uv_loop_t* event_loop,
return 12; // Signal internal error.

{
Mutex::ScopedLock scoped_lock(node_isolate_mutex);
CHECK_NULL(node_isolate);
node_isolate = isolate;
Mutex::ScopedLock scoped_lock(per_process::main_isolate_mutex);
CHECK_NULL(per_process::main_isolate);
per_process::main_isolate = isolate;
}

int exit_code;
Expand All @@ -1790,9 +1802,9 @@ inline int Start(uv_loop_t* event_loop,
}

{
Mutex::ScopedLock scoped_lock(node_isolate_mutex);
CHECK_EQ(node_isolate, isolate);
node_isolate = nullptr;
Mutex::ScopedLock scoped_lock(per_process::main_isolate_mutex);
CHECK_EQ(per_process::main_isolate, isolate);
per_process::main_isolate = nullptr;
}

isolate->Dispose();
Expand Down Expand Up @@ -1840,14 +1852,14 @@ int Start(int argc, char** argv) {
V8::SetEntropySource(crypto::EntropySource);
#endif // HAVE_OPENSSL

InitializeV8Platform(per_process_opts->v8_thread_pool_size);
InitializeV8Platform(per_process::cli_options->v8_thread_pool_size);
V8::Initialize();
performance::performance_v8_start = PERFORMANCE_NOW();
v8_initialized = true;
per_process::v8_initialized = true;
const int exit_code =
Start(uv_default_loop(), args, exec_args);
v8_platform.StopTracingAgent();
v8_initialized = false;
per_process::v8_initialized = false;
V8::Dispose();

// uv_run cannot be called from the time before the beforeExit callback
Expand Down
6 changes: 3 additions & 3 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ namespace node {
namespace {

inline void* BufferMalloc(size_t length) {
return per_process_opts->zero_fill_all_buffers ?
node::UncheckedCalloc(length) :
node::UncheckedMalloc(length);
return per_process::cli_options->zero_fill_all_buffers ?
node::UncheckedCalloc(length) :
node::UncheckedMalloc(length);
}

} // namespace
Expand Down
4 changes: 2 additions & 2 deletions src/node_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ static void Initialize(Local<Object> target,
#ifdef NODE_FIPS_MODE
READONLY_TRUE_PROPERTY(target, "fipsMode");
// TODO(addaleax): Use options parser variable instead.
if (per_process_opts->force_fips_crypto)
if (per_process::cli_options->force_fips_crypto)
READONLY_TRUE_PROPERTY(target, "fipsForced");
#endif

Expand Down Expand Up @@ -69,7 +69,7 @@ static void Initialize(Local<Object> target,

// TODO(addaleax): This seems to be an unused, private API. Remove it?
READONLY_STRING_PROPERTY(target, "icuDataDir",
per_process_opts->icu_data_dir);
per_process::cli_options->icu_data_dir);

#endif // NODE_HAVE_I18N_SUPPORT

Expand Down
7 changes: 4 additions & 3 deletions src/node_constants.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1234,9 +1234,10 @@ void DefineCryptoConstants(Local<Object> target) {
NODE_DEFINE_STRING_CONSTANT(target,
"defaultCoreCipherList",
DEFAULT_CIPHER_LIST_CORE);
NODE_DEFINE_STRING_CONSTANT(target,
"defaultCipherList",
per_process_opts->tls_cipher_list.c_str());
NODE_DEFINE_STRING_CONSTANT(
target,
"defaultCipherList",
per_process::cli_options->tls_cipher_list.c_str());

NODE_DEFINE_CONSTANT(target, TLS1_VERSION);
NODE_DEFINE_CONSTANT(target, TLS1_1_VERSION);
Expand Down
2 changes: 1 addition & 1 deletion src/node_credentials.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ bool SafeGetenv(const char* key, std::string* text) {
#endif

{
Mutex::ScopedLock lock(environ_mutex);
Mutex::ScopedLock lock(per_process::env_var_mutex);
if (const char* value = getenv(key)) {
*text = value;
return true;
Expand Down
17 changes: 8 additions & 9 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -769,7 +769,7 @@ static X509_STORE* NewRootCertStore() {
if (*system_cert_path != '\0') {
X509_STORE_load_locations(store, system_cert_path, nullptr);
}
if (per_process_opts->ssl_openssl_cert_store) {
if (per_process::cli_options->ssl_openssl_cert_store) {
X509_STORE_set_default_paths(store);
} else {
for (X509* cert : root_certs_vector) {
Expand Down Expand Up @@ -6153,16 +6153,15 @@ void InitCryptoOnce() {
OPENSSL_no_config();

// --openssl-config=...
if (!per_process_opts->openssl_config.empty()) {
if (!per_process::cli_options->openssl_config.empty()) {
OPENSSL_load_builtin_modules();
#ifndef OPENSSL_NO_ENGINE
ENGINE_load_builtin_engines();
#endif
ERR_clear_error();
CONF_modules_load_file(
per_process_opts->openssl_config.c_str(),
nullptr,
CONF_MFLAGS_DEFAULT_SECTION);
CONF_modules_load_file(per_process::cli_options->openssl_config.c_str(),
nullptr,
CONF_MFLAGS_DEFAULT_SECTION);
int err = ERR_get_error();
if (0 != err) {
fprintf(stderr,
Expand All @@ -6178,8 +6177,8 @@ void InitCryptoOnce() {
#ifdef NODE_FIPS_MODE
/* Override FIPS settings in cnf file, if needed. */
unsigned long err = 0; // NOLINT(runtime/int)
if (per_process_opts->enable_fips_crypto ||
per_process_opts->force_fips_crypto) {
if (per_process::cli_options->enable_fips_crypto ||
per_process::cli_options->force_fips_crypto) {
if (0 == FIPS_mode() && !FIPS_mode_set(1)) {
err = ERR_get_error();
}
Expand Down Expand Up @@ -6242,7 +6241,7 @@ void GetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
}

void SetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
CHECK(!per_process_opts->force_fips_crypto);
CHECK(!per_process::cli_options->force_fips_crypto);
Environment* env = Environment::GetCurrent(args);
const bool enabled = FIPS_mode();
bool enable;
Expand Down
Loading

0 comments on commit c6adf4b

Please sign in to comment.