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: nodejs#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]>
m5l14i11 · Apr 8, 2020 · fcde1de · fcde1de
1 parent e2ea73a
commit fcde1de
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 5 deletions.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -993,6 +993,8 @@ static X509_STORE* NewRootCertStore() {
   if (*system_cert_path != '\0') {
     X509_STORE_load_locations(store, system_cert_path, nullptr);
   }
+
+  Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
   if (per_process::cli_options->ssl_openssl_cert_store) {
     X509_STORE_set_default_paths(store);
   } else {

diff --git a/src/node_errors.cc b/src/node_errors.cc
@@ -406,7 +406,13 @@ void OnFatalError(const char* location, const char* message) {
 
   Isolate* isolate = Isolate::GetCurrent();
   Environment* env = Environment::GetCurrent(isolate);
-  if (per_process::cli_options->report_on_fatalerror) {
+  bool report_on_fatalerror;
+  {
+    Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
+    report_on_fatalerror = per_process::cli_options->report_on_fatalerror;
+  }
+
+  if (report_on_fatalerror) {
     report::TriggerNodeReport(
         isolate, env, message, "FatalError", "", Local<String>());
   }

diff --git a/src/node_options.h b/src/node_options.h
@@ -193,6 +193,14 @@ class PerIsolateOptions : public Options {
 
 class PerProcessOptions : public Options {
  public:
+  // Options shouldn't be here unless they affect the entire process scope, and
+  // that should avoided when possible.
+  //
+  // When an option is used during process initialization, it does not need
+  // protection, but any use after that will likely require synchronization
+  // using the node::per_process::cli_options_mutex, typically:
+  //
+  //     Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
   std::shared_ptr<PerIsolateOptions> per_isolate { new PerIsolateOptions() };
 
   std::string title;
@@ -213,7 +221,8 @@ class PerProcessOptions : public Options {
   std::string icu_data_dir;
 #endif
 
-  // TODO(addaleax): Some of these could probably be per-Environment.
+  // Per-process because they affect singleton OpenSSL shared library state,
+  // or are used once during process intialization.
 #if HAVE_OPENSSL
   std::string openssl_config;
   std::string tls_cipher_list = DEFAULT_CIPHER_LIST_CORE;
@@ -229,14 +238,18 @@ class PerProcessOptions : public Options {
   bool force_fips_crypto = false;
 #endif
 #endif
-  std::string use_largepages = "off";
-  bool trace_sigint = false;
-  std::vector<std::string> cmdline;
+
+  // Per-process because reports can be triggered outside a known V8 context.
   bool report_on_fatalerror = false;
   bool report_compact = false;
   std::string report_directory;
   std::string report_filename;
 
+  // TODO(addaleax): Some of these could probably be per-Environment.
+  std::string use_largepages = "off";
+  bool trace_sigint = false;
+  std::vector<std::string> cmdline;
+
   inline PerIsolateOptions* get_per_isolate_options();
   void CheckOptions(std::vector<std::string>* errors) override;
 };