src: refactor and harden ProcessEmitWarning()

- Handle exceptions when getting `process.emitWarning` or when
  calling it properly
- Add `Maybe<bool>` to the return type, like the V8 API uses it
  to indicate failure conditions
- Update call sites to account for that and clean up/return to JS
  when encountering an error
- Add an internal `ProcessEmitDeprecationWarning()` sibling
  for use in #17417,
  with common code extracted to a helper function
- Allow the warning to contain non-Latin-1 characters. Since the
  message will usually be a template string containing data passed
  from the user, this is the right thing to do.
- Add tests for the failure modes (except string creation failures)
  and UTF-8 warning messages.

PR-URL: #17420
Refs: #17417
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>

src/env.h

@@ -133,6 +133,7 @@ class ModuleWrap;
   V(dns_txt_string, "TXT")                                                    \
   V(domain_string, "domain")                                                  \
   V(emit_string, "emit")                                                      \
+  V(emit_warning_string, "emitWarning")                                       \
   V(exchange_string, "exchange")                                              \
   V(enumerable_string, "enumerable")                                          \
   V(idle_string, "idle")                                                      \

src/node.cc

@@ -148,12 +148,15 @@ using v8::HandleScope;
 using v8::HeapStatistics;
 using v8::Integer;
 using v8::Isolate;
+using v8::Just;
 using v8::Local;
 using v8::Locker;
+using v8::Maybe;
 using v8::MaybeLocal;
 using v8::Message;
 using v8::Name;
 using v8::NamedPropertyHandlerConfiguration;
+using v8::Nothing;
 using v8::Null;
 using v8::Number;
 using v8::Object;
@@ -2605,8 +2608,11 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
   }
   if (mp->nm_version == -1) {
     if (env->EmitNapiWarning()) {
-      ProcessEmitWarning(env, "N-API is an experimental feature and could "
-                         "change at any time.");
+      if (ProcessEmitWarning(env, "N-API is an experimental feature and could "
+                                  "change at any time.").IsNothing()) {
+        dlib.Close();
+        return;
+      }
     }
   } else if (mp->nm_version != NODE_MODULE_VERSION) {
     char errmsg[1024];
@@ -2753,33 +2759,83 @@ static void OnMessage(Local<Message> message, Local<Value> error) {
   FatalException(Isolate::GetCurrent(), error, message);
 }
 
+static Maybe<bool> ProcessEmitWarningGeneric(Environment* env,
+                                             const char* warning,
+                                             const char* type = nullptr,
+                                             const char* code = nullptr) {
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
+
+  Local<Object> process = env->process_object();
+  Local<Value> emit_warning;
+  if (!process->Get(env->context(),
+                    env->emit_warning_string()).ToLocal(&emit_warning)) {
+    return Nothing<bool>();
+  }
+
+  if (!emit_warning->IsFunction()) return Just(false);
+
+  int argc = 0;
+  Local<Value> args[3];  // warning, type, code
+
+  // The caller has to be able to handle a failure anyway, so we might as well
+  // do proper error checking for string creation.
+  if (!String::NewFromUtf8(env->isolate(),
+                           warning,
+                           v8::NewStringType::kNormal).ToLocal(&args[argc++])) {
+    return Nothing<bool>();
+  }
+  if (type != nullptr) {
+    if (!String::NewFromOneByte(env->isolate(),
+                                reinterpret_cast<const uint8_t*>(type),
+                                v8::NewStringType::kNormal)
+                                    .ToLocal(&args[argc++])) {
+      return Nothing<bool>();
+    }
+    if (code != nullptr &&
+        !String::NewFromOneByte(env->isolate(),
+                                reinterpret_cast<const uint8_t*>(code),
+                                v8::NewStringType::kNormal)
+                                    .ToLocal(&args[argc++])) {
+      return Nothing<bool>();
+    }
+  }
+
+  // MakeCallback() unneeded because emitWarning is internal code, it calls
+  // process.emit('warning', ...), but does so on the nextTick.
+  if (emit_warning.As<Function>()->Call(env->context(),
+                                        process,
+                                        argc,
+                                        args).IsEmpty()) {
+    return Nothing<bool>();
+  }
+  return Just(true);
+}
+
+
 // Call process.emitWarning(str), fmt is a snprintf() format string
-void ProcessEmitWarning(Environment* env, const char* fmt, ...) {
+Maybe<bool> ProcessEmitWarning(Environment* env, const char* fmt, ...) {
   char warning[1024];
   va_list ap;
 
   va_start(ap, fmt);
   vsnprintf(warning, sizeof(warning), fmt, ap);
   va_end(ap);
 
-  HandleScope handle_scope(env->isolate());
-  Context::Scope context_scope(env->context());
-
-  Local<Object> process = env->process_object();
-  MaybeLocal<Value> emit_warning = process->Get(env->context(),
-      FIXED_ONE_BYTE_STRING(env->isolate(), "emitWarning"));
-  Local<Value> arg = node::OneByteString(env->isolate(), warning);
-
-  Local<Value> f;
+  return ProcessEmitWarningGeneric(env, warning);
+}
 
-  if (!emit_warning.ToLocal(&f)) return;
-  if (!f->IsFunction()) return;
 
-  // MakeCallback() unneeded, because emitWarning is internal code, it calls
-  // process.emit('warning', ..), but does so on the nextTick.
-  f.As<v8::Function>()->Call(process, 1, &arg);
+Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
+                                          const char* warning,
+                                          const char* deprecation_code) {
+  return ProcessEmitWarningGeneric(env,
+                                   warning,
+                                   "DeprecationWarning",
+                                   deprecation_code);
 }
 
+
 static bool PullFromCache(Environment* env,
                           const FunctionCallbackInfo<Value>& args,
                           Local<String> module,

src/node_crypto.cc

@@ -1069,8 +1069,12 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
                                            root_cert_store,
                                            extra_root_certs_file.c_str());
       if (err) {
+        // We do not call back into JS after this line anyway, so ignoring
+        // the return value of ProcessEmitWarning does not affect how a
+        // possible exception would be propagated.
         ProcessEmitWarning(sc->env(),
-                           "Ignoring extra certs from `%s`, load failed: %s\n",
+                           "Ignoring extra certs from `%s`, "
+                           "load failed: %s\n",
                            extra_root_certs_file.c_str(),
                            ERR_error_string(err, nullptr));
       }
@@ -3625,7 +3629,10 @@ void CipherBase::Init(const char* cipher_type,
   int mode = EVP_CIPHER_CTX_mode(ctx_);
   if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE ||
       mode == EVP_CIPH_CCM_MODE)) {
-    ProcessEmitWarning(env(), "Use Cipheriv for counter mode of %s",
+    // Ignore the return value (i.e. possible exception) because we are
+    // not calling back into JS anyway.
+    ProcessEmitWarning(env(),
+                       "Use Cipheriv for counter mode of %s",
                        cipher_type);
   }
 

src/node_internals.h

@@ -287,7 +287,10 @@ class FatalTryCatch : public v8::TryCatch {
   Environment* env_;
 };
 
-void ProcessEmitWarning(Environment* env, const char* fmt, ...);
+v8::Maybe<bool> ProcessEmitWarning(Environment* env, const char* fmt, ...);
+v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
+                                              const char* warning,
+                                              const char* deprecation_code);
 
 void FillStatsArray(double* fields, const uv_stat_t* s);
 

test/parallel/test-process-emit-warning-from-native.js

@@ -0,0 +1,46 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+if (common.hasFipsCrypto)
+  common.skip('crypto.createCipher() is not supported in FIPS mode');
+
+const crypto = require('crypto');
+const key = '0123456789';
+
+{
+  common.expectWarning('Warning',
+                       'Use Cipheriv for counter mode of aes-256-gcm');
+
+  // Emits regular warning expected by expectWarning()
+  crypto.createCipher('aes-256-gcm', key);
+}
+
+const realEmitWarning = process.emitWarning;
+
+{
+  // It's a good idea to make this overridable from userland.
+  process.emitWarning = () => { throw new Error('foo'); };
+  assert.throws(() => {
+    crypto.createCipher('aes-256-gcm', key);
+  }, /^Error: foo$/);
+}
+
+{
+  Object.defineProperty(process, 'emitWarning', {
+    get() { throw new Error('bar'); },
+    configurable: true
+  });
+  assert.throws(() => {
+    crypto.createCipher('aes-256-gcm', key);
+  }, /^Error: bar$/);
+}
+
+// Reset back to default after the test.
+Object.defineProperty(process, 'emitWarning', {
+  value: realEmitWarning,
+  configurable: true,
+  writable: true
+});

test/parallel/test-tls-env-bad-extra-ca.js

@@ -18,7 +18,7 @@ if (process.env.CHILD) {
 
 const env = Object.assign({}, process.env, {
   CHILD: 'yes',
-  NODE_EXTRA_CA_CERTS: `${fixtures.fixturesDir}/no-such-file-exists`,
+  NODE_EXTRA_CA_CERTS: `${fixtures.fixturesDir}/no-such-file-exists-🐢`,
 });
 
 const opts = {
@@ -32,8 +32,12 @@ fork(__filename, opts)
     assert.strictEqual(status, 0, 'client did not succeed in connecting');
   }))
   .on('close', common.mustCall(function() {
-    const re = /Warning: Ignoring extra certs from.*no-such-file-exists.* load failed:.*No such file or directory/;
-    assert(re.test(stderr), stderr);
+    // TODO(addaleax): Make `SafeGetenv` work like `process.env`
+    // encoding-wise
+    if (!common.isWindows) {
+      const re = /Warning: Ignoring extra certs from.*no-such-file-exists-🐢.* load failed:.*No such file or directory/;
+      assert(re.test(stderr), stderr);
+    }
   }))
   .stderr.setEncoding('utf8').on('data', function(str) {
     stderr += str;