Skip to content

Commit 6ba00b8

Browse files
addaleaxMylesBorins
authored andcommitted
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]>
1 parent 5cd08d3 commit 6ba00b8

6 files changed

+140
-23
lines changed

src/env.h

+1
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ class ModuleWrap;
133133
V(dns_txt_string, "TXT") \
134134
V(domain_string, "domain") \
135135
V(emit_string, "emit") \
136+
V(emit_warning_string, "emitWarning") \
136137
V(exchange_string, "exchange") \
137138
V(enumerable_string, "enumerable") \
138139
V(idle_string, "idle") \

src/node.cc

+73-17
Original file line numberDiff line numberDiff line change
@@ -148,12 +148,15 @@ using v8::HandleScope;
148148
using v8::HeapStatistics;
149149
using v8::Integer;
150150
using v8::Isolate;
151+
using v8::Just;
151152
using v8::Local;
152153
using v8::Locker;
154+
using v8::Maybe;
153155
using v8::MaybeLocal;
154156
using v8::Message;
155157
using v8::Name;
156158
using v8::NamedPropertyHandlerConfiguration;
159+
using v8::Nothing;
157160
using v8::Null;
158161
using v8::Number;
159162
using v8::Object;
@@ -2605,8 +2608,11 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
26052608
}
26062609
if (mp->nm_version == -1) {
26072610
if (env->EmitNapiWarning()) {
2608-
ProcessEmitWarning(env, "N-API is an experimental feature and could "
2609-
"change at any time.");
2611+
if (ProcessEmitWarning(env, "N-API is an experimental feature and could "
2612+
"change at any time.").IsNothing()) {
2613+
dlib.Close();
2614+
return;
2615+
}
26102616
}
26112617
} else if (mp->nm_version != NODE_MODULE_VERSION) {
26122618
char errmsg[1024];
@@ -2753,33 +2759,83 @@ static void OnMessage(Local<Message> message, Local<Value> error) {
27532759
FatalException(Isolate::GetCurrent(), error, message);
27542760
}
27552761

2762+
static Maybe<bool> ProcessEmitWarningGeneric(Environment* env,
2763+
const char* warning,
2764+
const char* type = nullptr,
2765+
const char* code = nullptr) {
2766+
HandleScope handle_scope(env->isolate());
2767+
Context::Scope context_scope(env->context());
2768+
2769+
Local<Object> process = env->process_object();
2770+
Local<Value> emit_warning;
2771+
if (!process->Get(env->context(),
2772+
env->emit_warning_string()).ToLocal(&emit_warning)) {
2773+
return Nothing<bool>();
2774+
}
2775+
2776+
if (!emit_warning->IsFunction()) return Just(false);
2777+
2778+
int argc = 0;
2779+
Local<Value> args[3]; // warning, type, code
2780+
2781+
// The caller has to be able to handle a failure anyway, so we might as well
2782+
// do proper error checking for string creation.
2783+
if (!String::NewFromUtf8(env->isolate(),
2784+
warning,
2785+
v8::NewStringType::kNormal).ToLocal(&args[argc++])) {
2786+
return Nothing<bool>();
2787+
}
2788+
if (type != nullptr) {
2789+
if (!String::NewFromOneByte(env->isolate(),
2790+
reinterpret_cast<const uint8_t*>(type),
2791+
v8::NewStringType::kNormal)
2792+
.ToLocal(&args[argc++])) {
2793+
return Nothing<bool>();
2794+
}
2795+
if (code != nullptr &&
2796+
!String::NewFromOneByte(env->isolate(),
2797+
reinterpret_cast<const uint8_t*>(code),
2798+
v8::NewStringType::kNormal)
2799+
.ToLocal(&args[argc++])) {
2800+
return Nothing<bool>();
2801+
}
2802+
}
2803+
2804+
// MakeCallback() unneeded because emitWarning is internal code, it calls
2805+
// process.emit('warning', ...), but does so on the nextTick.
2806+
if (emit_warning.As<Function>()->Call(env->context(),
2807+
process,
2808+
argc,
2809+
args).IsEmpty()) {
2810+
return Nothing<bool>();
2811+
}
2812+
return Just(true);
2813+
}
2814+
2815+
27562816
// Call process.emitWarning(str), fmt is a snprintf() format string
2757-
void ProcessEmitWarning(Environment* env, const char* fmt, ...) {
2817+
Maybe<bool> ProcessEmitWarning(Environment* env, const char* fmt, ...) {
27582818
char warning[1024];
27592819
va_list ap;
27602820

27612821
va_start(ap, fmt);
27622822
vsnprintf(warning, sizeof(warning), fmt, ap);
27632823
va_end(ap);
27642824

2765-
HandleScope handle_scope(env->isolate());
2766-
Context::Scope context_scope(env->context());
2767-
2768-
Local<Object> process = env->process_object();
2769-
MaybeLocal<Value> emit_warning = process->Get(env->context(),
2770-
FIXED_ONE_BYTE_STRING(env->isolate(), "emitWarning"));
2771-
Local<Value> arg = node::OneByteString(env->isolate(), warning);
2772-
2773-
Local<Value> f;
2825+
return ProcessEmitWarningGeneric(env, warning);
2826+
}
27742827

2775-
if (!emit_warning.ToLocal(&f)) return;
2776-
if (!f->IsFunction()) return;
27772828

2778-
// MakeCallback() unneeded, because emitWarning is internal code, it calls
2779-
// process.emit('warning', ..), but does so on the nextTick.
2780-
f.As<v8::Function>()->Call(process, 1, &arg);
2829+
Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
2830+
const char* warning,
2831+
const char* deprecation_code) {
2832+
return ProcessEmitWarningGeneric(env,
2833+
warning,
2834+
"DeprecationWarning",
2835+
deprecation_code);
27812836
}
27822837

2838+
27832839
static bool PullFromCache(Environment* env,
27842840
const FunctionCallbackInfo<Value>& args,
27852841
Local<String> module,

src/node_crypto.cc

+9-2
Original file line numberDiff line numberDiff line change
@@ -1069,8 +1069,12 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
10691069
root_cert_store,
10701070
extra_root_certs_file.c_str());
10711071
if (err) {
1072+
// We do not call back into JS after this line anyway, so ignoring
1073+
// the return value of ProcessEmitWarning does not affect how a
1074+
// possible exception would be propagated.
10721075
ProcessEmitWarning(sc->env(),
1073-
"Ignoring extra certs from `%s`, load failed: %s\n",
1076+
"Ignoring extra certs from `%s`, "
1077+
"load failed: %s\n",
10741078
extra_root_certs_file.c_str(),
10751079
ERR_error_string(err, nullptr));
10761080
}
@@ -3625,7 +3629,10 @@ void CipherBase::Init(const char* cipher_type,
36253629
int mode = EVP_CIPHER_CTX_mode(ctx_);
36263630
if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE ||
36273631
mode == EVP_CIPH_CCM_MODE)) {
3628-
ProcessEmitWarning(env(), "Use Cipheriv for counter mode of %s",
3632+
// Ignore the return value (i.e. possible exception) because we are
3633+
// not calling back into JS anyway.
3634+
ProcessEmitWarning(env(),
3635+
"Use Cipheriv for counter mode of %s",
36293636
cipher_type);
36303637
}
36313638

src/node_internals.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,10 @@ class FatalTryCatch : public v8::TryCatch {
287287
Environment* env_;
288288
};
289289

290-
void ProcessEmitWarning(Environment* env, const char* fmt, ...);
290+
v8::Maybe<bool> ProcessEmitWarning(Environment* env, const char* fmt, ...);
291+
v8::Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
292+
const char* warning,
293+
const char* deprecation_code);
291294

292295
void FillStatsArray(double* fields, const uv_stat_t* s);
293296

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
if (common.hasFipsCrypto)
8+
common.skip('crypto.createCipher() is not supported in FIPS mode');
9+
10+
const crypto = require('crypto');
11+
const key = '0123456789';
12+
13+
{
14+
common.expectWarning('Warning',
15+
'Use Cipheriv for counter mode of aes-256-gcm');
16+
17+
// Emits regular warning expected by expectWarning()
18+
crypto.createCipher('aes-256-gcm', key);
19+
}
20+
21+
const realEmitWarning = process.emitWarning;
22+
23+
{
24+
// It's a good idea to make this overridable from userland.
25+
process.emitWarning = () => { throw new Error('foo'); };
26+
assert.throws(() => {
27+
crypto.createCipher('aes-256-gcm', key);
28+
}, /^Error: foo$/);
29+
}
30+
31+
{
32+
Object.defineProperty(process, 'emitWarning', {
33+
get() { throw new Error('bar'); },
34+
configurable: true
35+
});
36+
assert.throws(() => {
37+
crypto.createCipher('aes-256-gcm', key);
38+
}, /^Error: bar$/);
39+
}
40+
41+
// Reset back to default after the test.
42+
Object.defineProperty(process, 'emitWarning', {
43+
value: realEmitWarning,
44+
configurable: true,
45+
writable: true
46+
});

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

+7-3
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ if (process.env.CHILD) {
1818

1919
const env = Object.assign({}, process.env, {
2020
CHILD: 'yes',
21-
NODE_EXTRA_CA_CERTS: `${fixtures.fixturesDir}/no-such-file-exists`,
21+
NODE_EXTRA_CA_CERTS: `${fixtures.fixturesDir}/no-such-file-exists-🐢`,
2222
});
2323

2424
const opts = {
@@ -32,8 +32,12 @@ fork(__filename, opts)
3232
assert.strictEqual(status, 0, 'client did not succeed in connecting');
3333
}))
3434
.on('close', common.mustCall(function() {
35-
const re = /Warning: Ignoring extra certs from.*no-such-file-exists.* load failed:.*No such file or directory/;
36-
assert(re.test(stderr), stderr);
35+
// TODO(addaleax): Make `SafeGetenv` work like `process.env`
36+
// encoding-wise
37+
if (!common.isWindows) {
38+
const re = /Warning: Ignoring extra certs from.*no-such-file-exists-🐢.* load failed:.*No such file or directory/;
39+
assert(re.test(stderr), stderr);
40+
}
3741
}))
3842
.stderr.setEncoding('utf8').on('data', function(str) {
3943
stderr += str;

0 commit comments

Comments
 (0)