Skip to content

Commit f3cd537

Browse files
addaleaxBridgeAR
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 ef49f55 commit f3cd537

File tree

6 files changed

+140
-23
lines changed

6 files changed

+140
-23
lines changed

src/env.h

Lines changed: 1 addition & 0 deletions
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

Lines changed: 73 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -147,12 +147,15 @@ using v8::HandleScope;
147147
using v8::HeapStatistics;
148148
using v8::Integer;
149149
using v8::Isolate;
150+
using v8::Just;
150151
using v8::Local;
151152
using v8::Locker;
153+
using v8::Maybe;
152154
using v8::MaybeLocal;
153155
using v8::Message;
154156
using v8::Name;
155157
using v8::NamedPropertyHandlerConfiguration;
158+
using v8::Nothing;
156159
using v8::Null;
157160
using v8::Number;
158161
using v8::Object;
@@ -2277,8 +2280,11 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
22772280
}
22782281
if (mp->nm_version == -1) {
22792282
if (env->EmitNapiWarning()) {
2280-
ProcessEmitWarning(env, "N-API is an experimental feature and could "
2281-
"change at any time.");
2283+
if (ProcessEmitWarning(env, "N-API is an experimental feature and could "
2284+
"change at any time.").IsNothing()) {
2285+
dlib.Close();
2286+
return;
2287+
}
22822288
}
22832289
} else if (mp->nm_version != NODE_MODULE_VERSION) {
22842290
char errmsg[1024];
@@ -2425,33 +2431,83 @@ static void OnMessage(Local<Message> message, Local<Value> error) {
24252431
FatalException(Isolate::GetCurrent(), error, message);
24262432
}
24272433

2434+
static Maybe<bool> ProcessEmitWarningGeneric(Environment* env,
2435+
const char* warning,
2436+
const char* type = nullptr,
2437+
const char* code = nullptr) {
2438+
HandleScope handle_scope(env->isolate());
2439+
Context::Scope context_scope(env->context());
2440+
2441+
Local<Object> process = env->process_object();
2442+
Local<Value> emit_warning;
2443+
if (!process->Get(env->context(),
2444+
env->emit_warning_string()).ToLocal(&emit_warning)) {
2445+
return Nothing<bool>();
2446+
}
2447+
2448+
if (!emit_warning->IsFunction()) return Just(false);
2449+
2450+
int argc = 0;
2451+
Local<Value> args[3]; // warning, type, code
2452+
2453+
// The caller has to be able to handle a failure anyway, so we might as well
2454+
// do proper error checking for string creation.
2455+
if (!String::NewFromUtf8(env->isolate(),
2456+
warning,
2457+
v8::NewStringType::kNormal).ToLocal(&args[argc++])) {
2458+
return Nothing<bool>();
2459+
}
2460+
if (type != nullptr) {
2461+
if (!String::NewFromOneByte(env->isolate(),
2462+
reinterpret_cast<const uint8_t*>(type),
2463+
v8::NewStringType::kNormal)
2464+
.ToLocal(&args[argc++])) {
2465+
return Nothing<bool>();
2466+
}
2467+
if (code != nullptr &&
2468+
!String::NewFromOneByte(env->isolate(),
2469+
reinterpret_cast<const uint8_t*>(code),
2470+
v8::NewStringType::kNormal)
2471+
.ToLocal(&args[argc++])) {
2472+
return Nothing<bool>();
2473+
}
2474+
}
2475+
2476+
// MakeCallback() unneeded because emitWarning is internal code, it calls
2477+
// process.emit('warning', ...), but does so on the nextTick.
2478+
if (emit_warning.As<Function>()->Call(env->context(),
2479+
process,
2480+
argc,
2481+
args).IsEmpty()) {
2482+
return Nothing<bool>();
2483+
}
2484+
return Just(true);
2485+
}
2486+
2487+
24282488
// Call process.emitWarning(str), fmt is a snprintf() format string
2429-
void ProcessEmitWarning(Environment* env, const char* fmt, ...) {
2489+
Maybe<bool> ProcessEmitWarning(Environment* env, const char* fmt, ...) {
24302490
char warning[1024];
24312491
va_list ap;
24322492

24332493
va_start(ap, fmt);
24342494
vsnprintf(warning, sizeof(warning), fmt, ap);
24352495
va_end(ap);
24362496

2437-
HandleScope handle_scope(env->isolate());
2438-
Context::Scope context_scope(env->context());
2439-
2440-
Local<Object> process = env->process_object();
2441-
MaybeLocal<Value> emit_warning = process->Get(env->context(),
2442-
FIXED_ONE_BYTE_STRING(env->isolate(), "emitWarning"));
2443-
Local<Value> arg = node::OneByteString(env->isolate(), warning);
2444-
2445-
Local<Value> f;
2497+
return ProcessEmitWarningGeneric(env, warning);
2498+
}
24462499

2447-
if (!emit_warning.ToLocal(&f)) return;
2448-
if (!f->IsFunction()) return;
24492500

2450-
// MakeCallback() unneeded, because emitWarning is internal code, it calls
2451-
// process.emit('warning', ..), but does so on the nextTick.
2452-
f.As<v8::Function>()->Call(process, 1, &arg);
2501+
Maybe<bool> ProcessEmitDeprecationWarning(Environment* env,
2502+
const char* warning,
2503+
const char* deprecation_code) {
2504+
return ProcessEmitWarningGeneric(env,
2505+
warning,
2506+
"DeprecationWarning",
2507+
deprecation_code);
24532508
}
24542509

2510+
24552511
static bool PullFromCache(Environment* env,
24562512
const FunctionCallbackInfo<Value>& args,
24572513
Local<String> module,

src/node_crypto.cc

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1062,8 +1062,12 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
10621062
root_cert_store,
10631063
extra_root_certs_file.c_str());
10641064
if (err) {
1065+
// We do not call back into JS after this line anyway, so ignoring
1066+
// the return value of ProcessEmitWarning does not affect how a
1067+
// possible exception would be propagated.
10651068
ProcessEmitWarning(sc->env(),
1066-
"Ignoring extra certs from `%s`, load failed: %s\n",
1069+
"Ignoring extra certs from `%s`, "
1070+
"load failed: %s\n",
10671071
extra_root_certs_file.c_str(),
10681072
ERR_error_string(err, nullptr));
10691073
}
@@ -3618,7 +3622,10 @@ void CipherBase::Init(const char* cipher_type,
36183622
int mode = EVP_CIPHER_CTX_mode(ctx_);
36193623
if (encrypt && (mode == EVP_CIPH_CTR_MODE || mode == EVP_CIPH_GCM_MODE ||
36203624
mode == EVP_CIPH_CCM_MODE)) {
3621-
ProcessEmitWarning(env(), "Use Cipheriv for counter mode of %s",
3625+
// Ignore the return value (i.e. possible exception) because we are
3626+
// not calling back into JS anyway.
3627+
ProcessEmitWarning(env(),
3628+
"Use Cipheriv for counter mode of %s",
36223629
cipher_type);
36233630
}
36243631

src/node_internals.h

Lines changed: 4 additions & 1 deletion
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

Lines changed: 46 additions & 0 deletions
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

Lines changed: 7 additions & 3 deletions
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)