From c0d3fc28ecf8500859f87a8dd2b4deb7ac813fa9 Mon Sep 17 00:00:00 2001
From: Sam Roberts <vieuxtech@gmail.com>
Date: Thu, 2 Apr 2020 09:49:32 -0700
Subject: [PATCH] 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: https://github.com/nodejs/node/pull/32618
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
---
 src/node_crypto.cc |  2 ++
 src/node_errors.cc |  8 +++++++-
 src/node_options.h | 21 +++++++++++++++++----
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 9e6f76baaf3c1e..d318a2a87e4afd 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -973,6 +973,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
index 590562cccffbc3..9bae27550cec1b 100644
--- 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
index 2c7a5ec61b8905..be577703740548 100644
--- a/src/node_options.h
+++ b/src/node_options.h
@@ -194,6 +194,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;
@@ -215,7 +223,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;
@@ -231,14 +240,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;
 };