From 2bb2ed1c765ed410f4866bfc6a23d509c21b9d1e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Dec 2017 00:04:56 +0100 Subject: [PATCH 1/6] src: refactor and harden `ProcessEmitWarning()` - Handle exceptions when getting `process.emitWarning` or when calling it properly - Add `Maybe` 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 https://github.com/nodejs/node/pull/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. Refs: https://github.com/nodejs/node/pull/17417 --- src/env.h | 1 + src/node.cc | 90 +++++++++++++++---- src/node_crypto.cc | 16 ++-- src/node_internals.h | 5 +- .../test-process-emit-warning-from-native.js | 44 +++++++++ test/parallel/test-tls-env-bad-extra-ca.js | 4 +- 6 files changed, 134 insertions(+), 26 deletions(-) create mode 100644 test/parallel/test-process-emit-warning-from-native.js diff --git a/src/env.h b/src/env.h index 47723baa7859ff..ba1558a13d5ac7 100644 --- a/src/env.h +++ b/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") \ diff --git a/src/node.cc b/src/node.cc index ed2f0c14ff3c59..de030d441ebc09 100644 --- a/src/node.cc +++ b/src/node.cc @@ -147,12 +147,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; @@ -2277,8 +2280,11 @@ static void DLOpen(const FunctionCallbackInfo& 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]; @@ -2425,8 +2431,62 @@ static void OnMessage(Local message, Local error) { FatalException(Isolate::GetCurrent(), error, message); } +static Maybe 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 process = env->process_object(); + Local emit_warning; + if (!process->Get(env->context(), + env->emit_warning_string()).ToLocal(&emit_warning)) { + return Nothing(); + } + + if (!emit_warning->IsFunction()) return Just(false); + + int i = 0; + Local 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[i++])) { + return Nothing(); + } + if (type != nullptr) { + if (!String::NewFromOneByte(env->isolate(), + reinterpret_cast(type), + v8::NewStringType::kInternalized) + .ToLocal(&args[i++])) { + return Nothing(); + } + if (code != nullptr && + !String::NewFromOneByte(env->isolate(), + reinterpret_cast(code), + v8::NewStringType::kInternalized) + .ToLocal(&args[i++])) { + return Nothing(); + } + } + + // MakeCallback() unneeded because emitWarning is internal code, it calls + // process.emit('warning', ...), but does so on the nextTick. + if (emit_warning.As()->Call(env->context(), + process, + i, + args).IsEmpty()) { + return Nothing(); + } + return Just(true); +} + + // Call process.emitWarning(str), fmt is a snprintf() format string -void ProcessEmitWarning(Environment* env, const char* fmt, ...) { +Maybe ProcessEmitWarning(Environment* env, const char* fmt, ...) { char warning[1024]; va_list ap; @@ -2434,24 +2494,20 @@ void ProcessEmitWarning(Environment* env, const char* fmt, ...) { vsnprintf(warning, sizeof(warning), fmt, ap); va_end(ap); - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); - - Local process = env->process_object(); - MaybeLocal emit_warning = process->Get(env->context(), - FIXED_ONE_BYTE_STRING(env->isolate(), "emitWarning")); - Local arg = node::OneByteString(env->isolate(), warning); - - Local 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()->Call(process, 1, &arg); +Maybe ProcessEmitDeprecationWarning(Environment* env, + const char* warning, + const char* deprecation_code) { + return ProcessEmitWarningGeneric(env, + warning, + "DeprecationWarning", + deprecation_code); } + static bool PullFromCache(Environment* env, const FunctionCallbackInfo& args, Local module, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 6e9d2255cdd430..b08ed16d1b9613 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1062,10 +1062,13 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo& args) { root_cert_store, extra_root_certs_file.c_str()); if (err) { - ProcessEmitWarning(sc->env(), - "Ignoring extra certs from `%s`, load failed: %s\n", - extra_root_certs_file.c_str(), - ERR_error_string(err, nullptr)); + if (ProcessEmitWarning(sc->env(), + "Ignoring extra certs from `%s`, " + "load failed: %s\n", + extra_root_certs_file.c_str(), + ERR_error_string(err, nullptr)).IsNothing()) { + return; + } } } } @@ -3618,8 +3621,9 @@ 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", - cipher_type); + if (ProcessEmitWarning(env(), "Use Cipheriv for counter mode of %s", + cipher_type).IsNothing()) + return; } if (mode == EVP_CIPH_WRAP_MODE) diff --git a/src/node_internals.h b/src/node_internals.h index 5466736200d5b8..757d4785af2b16 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -287,7 +287,10 @@ class FatalTryCatch : public v8::TryCatch { Environment* env_; }; -void ProcessEmitWarning(Environment* env, const char* fmt, ...); +v8::Maybe ProcessEmitWarning(Environment* env, const char* fmt, ...); +v8::Maybe ProcessEmitDeprecationWarning(Environment* env, + const char* warning, + const char* deprecation_code); void FillStatsArray(double* fields, const uv_stat_t* s); diff --git a/test/parallel/test-process-emit-warning-from-native.js b/test/parallel/test-process-emit-warning-from-native.js new file mode 100644 index 00000000000000..1105ce2e2b17d8 --- /dev/null +++ b/test/parallel/test-process-emit-warning-from-native.js @@ -0,0 +1,44 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +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 +}); diff --git a/test/parallel/test-tls-env-bad-extra-ca.js b/test/parallel/test-tls-env-bad-extra-ca.js index f7e9341ad00757..7d9b92ad7b6f94 100644 --- a/test/parallel/test-tls-env-bad-extra-ca.js +++ b/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,7 +32,7 @@ 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/; + 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) { From 657555d3e73ee2a771cfb353d3b8d3e5709f2115 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Dec 2017 00:28:33 +0100 Subject: [PATCH 2/6] [squash] indentation --- src/node.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node.cc b/src/node.cc index de030d441ebc09..54664b73e45783 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2459,9 +2459,9 @@ static Maybe ProcessEmitWarningGeneric(Environment* env, } if (type != nullptr) { if (!String::NewFromOneByte(env->isolate(), - reinterpret_cast(type), - v8::NewStringType::kInternalized) - .ToLocal(&args[i++])) { + reinterpret_cast(type), + v8::NewStringType::kInternalized) + .ToLocal(&args[i++])) { return Nothing(); } if (code != nullptr && From bb5ba30120237f9d975b410afde1652d58cb12cf Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 2 Dec 2017 15:25:16 +0100 Subject: [PATCH 3/6] [squash] bnoordhuis & AndreasMadsen nits --- src/node.cc | 14 +++++++------- src/node_crypto.cc | 8 +++++--- src/node_internals.h | 4 ++-- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/node.cc b/src/node.cc index 54664b73e45783..fe564ae4898e5e 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2447,28 +2447,28 @@ static Maybe ProcessEmitWarningGeneric(Environment* env, if (!emit_warning->IsFunction()) return Just(false); - int i = 0; + int argc = 0; Local 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[i++])) { + v8::NewStringType::kNormal).ToLocal(&args[argc++])) { return Nothing(); } if (type != nullptr) { if (!String::NewFromOneByte(env->isolate(), reinterpret_cast(type), - v8::NewStringType::kInternalized) - .ToLocal(&args[i++])) { + v8::NewStringType::kNormal) + .ToLocal(&args[argc++])) { return Nothing(); } if (code != nullptr && !String::NewFromOneByte(env->isolate(), reinterpret_cast(code), - v8::NewStringType::kInternalized) - .ToLocal(&args[i++])) { + v8::NewStringType::kNormal) + .ToLocal(&args[argc++])) { return Nothing(); } } @@ -2477,7 +2477,7 @@ static Maybe ProcessEmitWarningGeneric(Environment* env, // process.emit('warning', ...), but does so on the nextTick. if (emit_warning.As()->Call(env->context(), process, - i, + argc, args).IsEmpty()) { return Nothing(); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b08ed16d1b9613..ff8b5e86e2bdb7 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3621,9 +3621,11 @@ 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)) { - if (ProcessEmitWarning(env(), "Use Cipheriv for counter mode of %s", - cipher_type).IsNothing()) - return; + // 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); } if (mode == EVP_CIPH_WRAP_MODE) diff --git a/src/node_internals.h b/src/node_internals.h index 757d4785af2b16..09bcbba6e0b39f 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -289,8 +289,8 @@ class FatalTryCatch : public v8::TryCatch { v8::Maybe ProcessEmitWarning(Environment* env, const char* fmt, ...); v8::Maybe ProcessEmitDeprecationWarning(Environment* env, - const char* warning, - const char* deprecation_code); + const char* warning, + const char* deprecation_code); void FillStatsArray(double* fields, const uv_stat_t* s); From 1c72f298c0485c3d515bb1d0f13d056ec0ecf53b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 9 Dec 2017 15:29:03 +0100 Subject: [PATCH 4/6] [squash] bnoordhuis comment --- src/node_crypto.cc | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index ff8b5e86e2bdb7..8d5efe4d6b426b 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1062,13 +1062,14 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo& args) { root_cert_store, extra_root_certs_file.c_str()); if (err) { - if (ProcessEmitWarning(sc->env(), - "Ignoring extra certs from `%s`, " - "load failed: %s\n", - extra_root_certs_file.c_str(), - ERR_error_string(err, nullptr)).IsNothing()) { - return; - } + // 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", + extra_root_certs_file.c_str(), + ERR_error_string(err, nullptr)); } } } From 35d3c42bf2f30490af24b8a841122d6e78e18cc2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Dec 2017 01:19:54 +0100 Subject: [PATCH 5/6] [squash] skip test in FIPS mode --- test/parallel/test-process-emit-warning-from-native.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/parallel/test-process-emit-warning-from-native.js b/test/parallel/test-process-emit-warning-from-native.js index 1105ce2e2b17d8..d50ea3962bdedc 100644 --- a/test/parallel/test-process-emit-warning-from-native.js +++ b/test/parallel/test-process-emit-warning-from-native.js @@ -4,6 +4,8 @@ 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'; From a9613ac7dc542922cde9886e9d9ddb03e1f7cf6a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Dec 2017 02:36:29 +0100 Subject: [PATCH 6/6] [squash] skip environment variable encoding test on windows --- test/parallel/test-tls-env-bad-extra-ca.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-tls-env-bad-extra-ca.js b/test/parallel/test-tls-env-bad-extra-ca.js index 7d9b92ad7b6f94..e141de34a9fe1b 100644 --- a/test/parallel/test-tls-env-bad-extra-ca.js +++ b/test/parallel/test-tls-env-bad-extra-ca.js @@ -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;