Skip to content

Commit 5eb0765

Browse files
bnoordhuisMylesBorins
authored andcommitted
src: handle TryCatch with empty message
This bug needs a test case with a high goldilocks factor to trigger but the synopsis is that `v8::TryCatch::Message()` returns an empty handle when the TryCatch is declared at a time when an exception is already pending. We now recompute the message inside `node::ReportException()` and all is well again. PR-URL: #20708 Fixes: #8854 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
1 parent a4cbe30 commit 5eb0765

File tree

2 files changed

+60
-1
lines changed

2 files changed

+60
-1
lines changed

src/node.cc

+3-1
Original file line numberDiff line numberDiff line change
@@ -1413,9 +1413,11 @@ static void ReportException(Environment* env,
14131413
Local<Value> er,
14141414
Local<Message> message) {
14151415
CHECK(!er.IsEmpty());
1416-
CHECK(!message.IsEmpty());
14171416
HandleScope scope(env->isolate());
14181417

1418+
if (message.IsEmpty())
1419+
message = Exception::CreateMessage(env->isolate(), er);
1420+
14191421
AppendExceptionLine(env, er, message, FATAL_ERROR);
14201422

14211423
Local<Value> trace_value;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
'use strict';
2+
3+
// Verify that exceptions from a callback don't result in
4+
// failed CHECKs when trying to print the exception message.
5+
6+
// This test is convoluted because it needs to trigger a callback
7+
// into JS land at just the right time when an exception is pending,
8+
// and does so by exploiting a weakness in the streams infrastructure.
9+
// I won't shed any tears if this test ever becomes invalidated.
10+
11+
const common = require('../common');
12+
13+
if (!common.hasCrypto)
14+
common.skip('missing crypto');
15+
16+
if (process.argv[2] === 'child') {
17+
const fixtures = require('../common/fixtures');
18+
const https = require('https');
19+
const net = require('net');
20+
const tls = require('tls');
21+
const { Duplex } = require('stream');
22+
const { mustCall } = common;
23+
24+
const cert = fixtures.readSync('test_cert.pem');
25+
const key = fixtures.readSync('test_key.pem');
26+
27+
net.createServer(mustCall(onplaintext)).listen(0, mustCall(onlisten));
28+
29+
function onlisten() {
30+
const { port } = this.address();
31+
https.get({ port, rejectUnauthorized: false });
32+
}
33+
34+
function onplaintext(c) {
35+
const d = new class extends Duplex {
36+
_read(n) {
37+
const data = c.read(n);
38+
if (data) d.push(data);
39+
}
40+
_write(...xs) {
41+
c.write(...xs);
42+
}
43+
}();
44+
c.on('data', d.push.bind(d));
45+
46+
const options = { key, cert };
47+
const fail = () => { throw new Error('eyecatcher'); };
48+
tls.createServer(options, mustCall(fail)).emit('connection', d);
49+
}
50+
} else {
51+
const assert = require('assert');
52+
const { spawnSync } = require('child_process');
53+
const result = spawnSync(process.execPath, [__filename, 'child']);
54+
const stderr = result.stderr.toString();
55+
const ok = stderr.includes('Error: eyecatcher');
56+
assert(ok, stderr);
57+
}

0 commit comments

Comments
 (0)