Skip to content

Commit 1763649

Browse files
sam-githubtargos
authored andcommittedApr 11, 2020
src: sync access for report and openssl options
Audited usage of per-process OpenSSL and Report options, adding two required mutexes. Also documented existence and typical use of the per-process cli option mutex. PR-URL: #32618 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
1 parent 756a049 commit 1763649

File tree

3 files changed

+26
-5
lines changed

3 files changed

+26
-5
lines changed
 

‎src/node_crypto.cc

+2
Original file line numberDiff line numberDiff line change
@@ -993,6 +993,8 @@ static X509_STORE* NewRootCertStore() {
993993
if (*system_cert_path != '\0') {
994994
X509_STORE_load_locations(store, system_cert_path, nullptr);
995995
}
996+
997+
Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
996998
if (per_process::cli_options->ssl_openssl_cert_store) {
997999
X509_STORE_set_default_paths(store);
9981000
} else {

‎src/node_errors.cc

+7-1
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,13 @@ void OnFatalError(const char* location, const char* message) {
406406

407407
Isolate* isolate = Isolate::GetCurrent();
408408
Environment* env = Environment::GetCurrent(isolate);
409-
if (per_process::cli_options->report_on_fatalerror) {
409+
bool report_on_fatalerror;
410+
{
411+
Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
412+
report_on_fatalerror = per_process::cli_options->report_on_fatalerror;
413+
}
414+
415+
if (report_on_fatalerror) {
410416
report::TriggerNodeReport(
411417
isolate, env, message, "FatalError", "", Local<String>());
412418
}

‎src/node_options.h

+17-4
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,14 @@ class PerIsolateOptions : public Options {
193193

194194
class PerProcessOptions : public Options {
195195
public:
196+
// Options shouldn't be here unless they affect the entire process scope, and
197+
// that should avoided when possible.
198+
//
199+
// When an option is used during process initialization, it does not need
200+
// protection, but any use after that will likely require synchronization
201+
// using the node::per_process::cli_options_mutex, typically:
202+
//
203+
// Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
196204
std::shared_ptr<PerIsolateOptions> per_isolate { new PerIsolateOptions() };
197205

198206
std::string title;
@@ -213,7 +221,8 @@ class PerProcessOptions : public Options {
213221
std::string icu_data_dir;
214222
#endif
215223

216-
// TODO(addaleax): Some of these could probably be per-Environment.
224+
// Per-process because they affect singleton OpenSSL shared library state,
225+
// or are used once during process intialization.
217226
#if HAVE_OPENSSL
218227
std::string openssl_config;
219228
std::string tls_cipher_list = DEFAULT_CIPHER_LIST_CORE;
@@ -229,14 +238,18 @@ class PerProcessOptions : public Options {
229238
bool force_fips_crypto = false;
230239
#endif
231240
#endif
232-
std::string use_largepages = "off";
233-
bool trace_sigint = false;
234-
std::vector<std::string> cmdline;
241+
242+
// Per-process because reports can be triggered outside a known V8 context.
235243
bool report_on_fatalerror = false;
236244
bool report_compact = false;
237245
std::string report_directory;
238246
std::string report_filename;
239247

248+
// TODO(addaleax): Some of these could probably be per-Environment.
249+
std::string use_largepages = "off";
250+
bool trace_sigint = false;
251+
std::vector<std::string> cmdline;
252+
240253
inline PerIsolateOptions* get_per_isolate_options();
241254
void CheckOptions(std::vector<std::string>* errors) override;
242255
};

0 commit comments

Comments
 (0)
Please sign in to comment.