Skip to content

Commit 9db9e7e

Browse files
joyeecheungaddaleax
joyeecheung
authored and
addaleax
committed
src: move per-process global variables into node::per_process
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
1 parent ae2d1f0 commit 9db9e7e

17 files changed

+125
-91
lines changed

‎src/env.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,8 @@ IsolateData::IsolateData(Isolate* isolate,
7979
if (platform_ != nullptr)
8080
platform_->RegisterIsolate(isolate_, event_loop);
8181

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

8485
// Create string and private symbol properties as internalized one byte
8586
// strings after the platform is properly initialized.

‎src/node.cc‎

Lines changed: 51 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -139,21 +139,31 @@ using v8::Undefined;
139139
using v8::V8;
140140
using v8::Value;
141141

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

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

154+
// node_revert.h
155+
// Bit flag used to track security reverts.
156+
unsigned int reverted_cve = 0;
157+
158+
// util.h
159+
// Tells whether the per-process V8::Initialize() is called and
160+
// if it is safe to call v8::Isolate::GetCurrent().
147161
bool v8_initialized = false;
148162

163+
// node_internals.h
149164
// process-relative uptime base, initialized at start-up
150165
double prog_start_time;
151-
152-
Mutex per_process_opts_mutex;
153-
std::shared_ptr<PerProcessOptions> per_process_opts {
154-
new PerProcessOptions() };
155-
static Mutex node_isolate_mutex;
156-
static Isolate* node_isolate;
166+
} // namespace per_process
157167

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

271281
void StartTracingAgent() {
272-
if (per_process_opts->trace_event_categories.empty()) {
282+
if (per_process::cli_options->trace_event_categories.empty()) {
273283
tracing_file_writer_ = tracing_agent_->DefaultHandle();
274284
} else {
275285
tracing_file_writer_ = tracing_agent_->AddClient(
276-
ParseCommaSeparatedSet(per_process_opts->trace_event_categories),
286+
ParseCommaSeparatedSet(
287+
per_process::cli_options->trace_event_categories),
277288
std::unique_ptr<tracing::AsyncTraceWriter>(
278289
new tracing::NodeTraceWriter(
279-
per_process_opts->trace_event_file_pattern)),
290+
per_process::cli_options->trace_event_file_pattern)),
280291
tracing::Agent::kUseDefaultCategories);
281292
}
282293
}
@@ -500,7 +511,7 @@ const char* signo_string(int signo) {
500511
}
501512

502513
void* ArrayBufferAllocator::Allocate(size_t size) {
503-
if (zero_fill_field_ || per_process_opts->zero_fill_all_buffers)
514+
if (zero_fill_field_ || per_process::cli_options->zero_fill_all_buffers)
504515
return UncheckedCalloc(size);
505516
else
506517
return UncheckedMalloc(size);
@@ -1276,12 +1287,12 @@ void ProcessArgv(std::vector<std::string>* args,
12761287
{
12771288
// TODO(addaleax): The mutex here should ideally be held during the
12781289
// entire function, but that doesn't play well with the exit() calls below.
1279-
Mutex::ScopedLock lock(per_process_opts_mutex);
1290+
Mutex::ScopedLock lock(per_process::cli_options_mutex);
12801291
options_parser::PerProcessOptionsParser::instance.Parse(
12811292
args,
12821293
exec_args,
12831294
&v8_args,
1284-
per_process_opts.get(),
1295+
per_process::cli_options.get(),
12851296
is_env ? kAllowedInEnvironment : kDisallowedInEnvironment,
12861297
&errors);
12871298
}
@@ -1293,20 +1304,20 @@ void ProcessArgv(std::vector<std::string>* args,
12931304
exit(9);
12941305
}
12951306

1296-
if (per_process_opts->print_version) {
1307+
if (per_process::cli_options->print_version) {
12971308
printf("%s\n", NODE_VERSION);
12981309
exit(0);
12991310
}
13001311

1301-
if (per_process_opts->print_v8_help) {
1312+
if (per_process::cli_options->print_v8_help) {
13021313
V8::SetFlagsFromString("--help", 6);
13031314
exit(0);
13041315
}
13051316

1306-
for (const std::string& cve : per_process_opts->security_reverts)
1317+
for (const std::string& cve : per_process::cli_options->security_reverts)
13071318
Revert(cve.c_str());
13081319

1309-
auto env_opts = per_process_opts->per_isolate->per_env;
1320+
auto env_opts = per_process::cli_options->per_isolate->per_env;
13101321
if (std::find(v8_args.begin(), v8_args.end(),
13111322
"--abort-on-uncaught-exception") != v8_args.end() ||
13121323
std::find(v8_args.begin(), v8_args.end(),
@@ -1319,14 +1330,14 @@ void ProcessArgv(std::vector<std::string>* args,
13191330
// behavior but it could also interfere with the user's intentions in ways
13201331
// we fail to anticipate. Dillema.
13211332
if (std::find(v8_args.begin(), v8_args.end(), "--prof") != v8_args.end()) {
1322-
v8_is_profiling = true;
1333+
per_process::v8_is_profiling = true;
13231334
}
13241335

13251336
#ifdef __POSIX__
13261337
// Block SIGPROF signals when sleeping in epoll_wait/kevent/etc. Avoids the
13271338
// performance penalty of frequent EINTR wakeups when the profiler is running.
13281339
// Only do this for v8.log profiling, as it breaks v8::CpuProfiler users.
1329-
if (v8_is_profiling) {
1340+
if (per_process::v8_is_profiling) {
13301341
uv_loop_configure(uv_default_loop(), UV_LOOP_BLOCK_SIGNAL, SIGPROF);
13311342
}
13321343
#endif
@@ -1355,7 +1366,7 @@ void ProcessArgv(std::vector<std::string>* args,
13551366
void Init(std::vector<std::string>* argv,
13561367
std::vector<std::string>* exec_argv) {
13571368
// Initialize prog_start_time to get relative uptime.
1358-
prog_start_time = static_cast<double>(uv_now(uv_default_loop()));
1369+
per_process::prog_start_time = static_cast<double>(uv_now(uv_default_loop()));
13591370

13601371
// Register built-in modules
13611372
binding::RegisterBuiltinModules();
@@ -1371,7 +1382,7 @@ void Init(std::vector<std::string>* argv,
13711382
#endif
13721383

13731384
std::shared_ptr<EnvironmentOptions> default_env_options =
1374-
per_process_opts->per_isolate->per_env;
1385+
per_process::cli_options->per_isolate->per_env;
13751386
{
13761387
std::string text;
13771388
default_env_options->pending_deprecation =
@@ -1400,7 +1411,7 @@ void Init(std::vector<std::string>* argv,
14001411
}
14011412

14021413
#if HAVE_OPENSSL
1403-
std::string* openssl_config = &per_process_opts->openssl_config;
1414+
std::string* openssl_config = &per_process::cli_options->openssl_config;
14041415
if (openssl_config->empty()) {
14051416
credentials::SafeGetenv("OPENSSL_CONF", openssl_config);
14061417
}
@@ -1434,16 +1445,17 @@ void Init(std::vector<std::string>* argv,
14341445
ProcessArgv(argv, exec_argv, false);
14351446

14361447
// Set the process.title immediately after processing argv if --title is set.
1437-
if (!per_process_opts->title.empty())
1438-
uv_set_process_title(per_process_opts->title.c_str());
1448+
if (!per_process::cli_options->title.empty())
1449+
uv_set_process_title(per_process::cli_options->title.c_str());
14391450

14401451
#if defined(NODE_HAVE_I18N_SUPPORT)
14411452
// If the parameter isn't given, use the env variable.
1442-
if (per_process_opts->icu_data_dir.empty())
1443-
credentials::SafeGetenv("NODE_ICU_DATA", &per_process_opts->icu_data_dir);
1453+
if (per_process::cli_options->icu_data_dir.empty())
1454+
credentials::SafeGetenv("NODE_ICU_DATA",
1455+
&per_process::cli_options->icu_data_dir);
14441456
// Initialize ICU.
14451457
// If icu_data_dir is empty here, it will load the 'minimal' data.
1446-
if (!i18n::InitializeICUDirectory(per_process_opts->icu_data_dir)) {
1458+
if (!i18n::InitializeICUDirectory(per_process::cli_options->icu_data_dir)) {
14471459
fprintf(stderr,
14481460
"%s: could not initialize ICU "
14491461
"(check NODE_ICU_DATA or --icu-data-dir parameters)\n",
@@ -1604,7 +1616,7 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
16041616
std::vector<std::string> args(argv, argv + argc);
16051617
std::vector<std::string> exec_args(exec_argv, exec_argv + exec_argc);
16061618
Environment* env = new Environment(isolate_data, context);
1607-
env->Start(args, exec_args, v8_is_profiling);
1619+
env->Start(args, exec_args, per_process::v8_is_profiling);
16081620
return env;
16091621
}
16101622

@@ -1678,7 +1690,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
16781690
Local<Context> context = NewContext(isolate);
16791691
Context::Scope context_scope(context);
16801692
Environment env(isolate_data, context);
1681-
env.Start(args, exec_args, v8_is_profiling);
1693+
env.Start(args, exec_args, per_process::v8_is_profiling);
16821694

16831695
const char* path = args.size() > 1 ? args[1].c_str() : nullptr;
16841696
StartInspector(&env, path);
@@ -1785,9 +1797,9 @@ inline int Start(uv_loop_t* event_loop,
17851797
return 12; // Signal internal error.
17861798

17871799
{
1788-
Mutex::ScopedLock scoped_lock(node_isolate_mutex);
1789-
CHECK_NULL(node_isolate);
1790-
node_isolate = isolate;
1800+
Mutex::ScopedLock scoped_lock(per_process::main_isolate_mutex);
1801+
CHECK_NULL(per_process::main_isolate);
1802+
per_process::main_isolate = isolate;
17911803
}
17921804

17931805
int exit_code;
@@ -1812,9 +1824,9 @@ inline int Start(uv_loop_t* event_loop,
18121824
}
18131825

18141826
{
1815-
Mutex::ScopedLock scoped_lock(node_isolate_mutex);
1816-
CHECK_EQ(node_isolate, isolate);
1817-
node_isolate = nullptr;
1827+
Mutex::ScopedLock scoped_lock(per_process::main_isolate_mutex);
1828+
CHECK_EQ(per_process::main_isolate, isolate);
1829+
per_process::main_isolate = nullptr;
18181830
}
18191831

18201832
isolate->Dispose();
@@ -1862,14 +1874,14 @@ int Start(int argc, char** argv) {
18621874
V8::SetEntropySource(crypto::EntropySource);
18631875
#endif // HAVE_OPENSSL
18641876

1865-
InitializeV8Platform(per_process_opts->v8_thread_pool_size);
1877+
InitializeV8Platform(per_process::cli_options->v8_thread_pool_size);
18661878
V8::Initialize();
18671879
performance::performance_v8_start = PERFORMANCE_NOW();
1868-
v8_initialized = true;
1880+
per_process::v8_initialized = true;
18691881
const int exit_code =
18701882
Start(uv_default_loop(), args, exec_args);
18711883
v8_platform.StopTracingAgent();
1872-
v8_initialized = false;
1884+
per_process::v8_initialized = false;
18731885
V8::Dispose();
18741886

18751887
// uv_run cannot be called from the time before the beforeExit callback

‎src/node_buffer.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,9 @@ namespace node {
5959
namespace {
6060

6161
inline void* BufferMalloc(size_t length) {
62-
return per_process_opts->zero_fill_all_buffers ?
63-
node::UncheckedCalloc(length) :
64-
node::UncheckedMalloc(length);
62+
return per_process::cli_options->zero_fill_all_buffers ?
63+
node::UncheckedCalloc(length) :
64+
node::UncheckedMalloc(length);
6565
}
6666

6767
} // namespace

‎src/node_config.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ static void Initialize(Local<Object> target,
4141
#ifdef NODE_FIPS_MODE
4242
READONLY_TRUE_PROPERTY(target, "fipsMode");
4343
// TODO(addaleax): Use options parser variable instead.
44-
if (per_process_opts->force_fips_crypto)
44+
if (per_process::cli_options->force_fips_crypto)
4545
READONLY_TRUE_PROPERTY(target, "fipsForced");
4646
#endif
4747

‎src/node_constants.cc‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1234,9 +1234,10 @@ void DefineCryptoConstants(Local<Object> target) {
12341234
NODE_DEFINE_STRING_CONSTANT(target,
12351235
"defaultCoreCipherList",
12361236
DEFAULT_CIPHER_LIST_CORE);
1237-
NODE_DEFINE_STRING_CONSTANT(target,
1238-
"defaultCipherList",
1239-
per_process_opts->tls_cipher_list.c_str());
1237+
NODE_DEFINE_STRING_CONSTANT(
1238+
target,
1239+
"defaultCipherList",
1240+
per_process::cli_options->tls_cipher_list.c_str());
12401241

12411242
NODE_DEFINE_CONSTANT(target, TLS1_VERSION);
12421243
NODE_DEFINE_CONSTANT(target, TLS1_1_VERSION);

‎src/node_credentials.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ bool SafeGetenv(const char* key, std::string* text) {
3838
#endif
3939

4040
{
41-
Mutex::ScopedLock lock(environ_mutex);
41+
Mutex::ScopedLock lock(per_process::env_var_mutex);
4242
if (const char* value = getenv(key)) {
4343
*text = value;
4444
return true;

‎src/node_crypto.cc‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -787,7 +787,7 @@ static X509_STORE* NewRootCertStore() {
787787
if (*system_cert_path != '\0') {
788788
X509_STORE_load_locations(store, system_cert_path, nullptr);
789789
}
790-
if (per_process_opts->ssl_openssl_cert_store) {
790+
if (per_process::cli_options->ssl_openssl_cert_store) {
791791
X509_STORE_set_default_paths(store);
792792
} else {
793793
for (X509* cert : root_certs_vector) {
@@ -6183,16 +6183,15 @@ void InitCryptoOnce() {
61836183
OPENSSL_no_config();
61846184

61856185
// --openssl-config=...
6186-
if (!per_process_opts->openssl_config.empty()) {
6186+
if (!per_process::cli_options->openssl_config.empty()) {
61876187
OPENSSL_load_builtin_modules();
61886188
#ifndef OPENSSL_NO_ENGINE
61896189
ENGINE_load_builtin_engines();
61906190
#endif
61916191
ERR_clear_error();
6192-
CONF_modules_load_file(
6193-
per_process_opts->openssl_config.c_str(),
6194-
nullptr,
6195-
CONF_MFLAGS_DEFAULT_SECTION);
6192+
CONF_modules_load_file(per_process::cli_options->openssl_config.c_str(),
6193+
nullptr,
6194+
CONF_MFLAGS_DEFAULT_SECTION);
61966195
int err = ERR_get_error();
61976196
if (0 != err) {
61986197
fprintf(stderr,
@@ -6208,8 +6207,8 @@ void InitCryptoOnce() {
62086207
#ifdef NODE_FIPS_MODE
62096208
/* Override FIPS settings in cnf file, if needed. */
62106209
unsigned long err = 0; // NOLINT(runtime/int)
6211-
if (per_process_opts->enable_fips_crypto ||
6212-
per_process_opts->force_fips_crypto) {
6210+
if (per_process::cli_options->enable_fips_crypto ||
6211+
per_process::cli_options->force_fips_crypto) {
62136212
if (0 == FIPS_mode() && !FIPS_mode_set(1)) {
62146213
err = ERR_get_error();
62156214
}
@@ -6272,7 +6271,7 @@ void GetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
62726271
}
62736272

62746273
void SetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
6275-
CHECK(!per_process_opts->force_fips_crypto);
6274+
CHECK(!per_process::cli_options->force_fips_crypto);
62766275
Environment* env = Environment::GetCurrent(args);
62776276
const bool enabled = FIPS_mode();
62786277
bool enable = args[0]->BooleanValue(env->isolate());

‎src/node_env_var.cc‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,17 @@ using v8::PropertyCallbackInfo;
2525
using v8::String;
2626
using v8::Value;
2727

28+
namespace per_process {
29+
Mutex env_var_mutex;
30+
} // namespace per_process
31+
2832
static void EnvGetter(Local<Name> property,
2933
const PropertyCallbackInfo<Value>& info) {
3034
Isolate* isolate = info.GetIsolate();
3135
if (property->IsSymbol()) {
3236
return info.GetReturnValue().SetUndefined();
3337
}
34-
Mutex::ScopedLock lock(environ_mutex);
38+
Mutex::ScopedLock lock(per_process::env_var_mutex);
3539
#ifdef __POSIX__
3640
node::Utf8Value key(isolate, property);
3741
const char* val = getenv(*key);
@@ -80,7 +84,7 @@ static void EnvSetter(Local<Name> property,
8084
return;
8185
}
8286

83-
Mutex::ScopedLock lock(environ_mutex);
87+
Mutex::ScopedLock lock(per_process::env_var_mutex);
8488
#ifdef __POSIX__
8589
node::Utf8Value key(info.GetIsolate(), property);
8690
node::Utf8Value val(info.GetIsolate(), value);
@@ -100,7 +104,7 @@ static void EnvSetter(Local<Name> property,
100104

101105
static void EnvQuery(Local<Name> property,
102106
const PropertyCallbackInfo<Integer>& info) {
103-
Mutex::ScopedLock lock(environ_mutex);
107+
Mutex::ScopedLock lock(per_process::env_var_mutex);
104108
int32_t rc = -1; // Not found unless proven otherwise.
105109
if (property->IsString()) {
106110
#ifdef __POSIX__
@@ -127,7 +131,7 @@ static void EnvQuery(Local<Name> property,
127131

128132
static void EnvDeleter(Local<Name> property,
129133
const PropertyCallbackInfo<Boolean>& info) {
130-
Mutex::ScopedLock lock(environ_mutex);
134+
Mutex::ScopedLock lock(per_process::env_var_mutex);
131135
if (property->IsString()) {
132136
#ifdef __POSIX__
133137
node::Utf8Value key(info.GetIsolate(), property);
@@ -148,7 +152,7 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
148152
Environment* env = Environment::GetCurrent(info);
149153
Isolate* isolate = env->isolate();
150154

151-
Mutex::ScopedLock lock(environ_mutex);
155+
Mutex::ScopedLock lock(per_process::env_var_mutex);
152156
Local<Array> envarr;
153157
int env_size = 0;
154158
#ifdef __POSIX__

0 commit comments

Comments
 (0)