Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: handle TryCatch with empty message #20708

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1414,9 +1414,11 @@ static void ReportException(Environment* env,
Local<Value> er,
Local<Message> message) {
CHECK(!er.IsEmpty());
CHECK(!message.IsEmpty());
HandleScope scope(env->isolate());

if (message.IsEmpty())
message = Exception::CreateMessage(env->isolate(), er);

AppendExceptionLine(env, er, message, FATAL_ERROR);

Local<Value> trace_value;
Expand Down
57 changes: 57 additions & 0 deletions test/parallel/test-tls-handshake-exception.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
'use strict';

// Verify that exceptions from a callback don't result in
// failed CHECKs when trying to print the exception message.

// This test is convoluted because it needs to trigger a callback
// into JS land at just the right time when an exception is pending,
// and does so by exploiting a weakness in the streams infrastructure.
// I won't shed any tears if this test ever becomes invalidated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry if I'm being dense, but can you explain what you mean by this comment? I'm not sure what you mean exactly by "becomes invalidated". Does that mean "becomes unnecessary" in this context? Or maybe it means "stops working"? Or something else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary, yes, as in "no longer tests this particular condition because node's internals changed."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, literally invalidated like you wrote initially, just I was thinking about it wrong. I get it now. Thanks.


const common = require('../common');

if (!common.hasCrypto)
common.skip('missing crypto');

if (process.argv[2] === 'child') {
const fixtures = require('../common/fixtures');
const https = require('https');
const net = require('net');
const tls = require('tls');
const { Duplex } = require('stream');
const { mustCall } = common;

const cert = fixtures.readSync('test_cert.pem');
const key = fixtures.readSync('test_key.pem');

net.createServer(mustCall(onplaintext)).listen(0, mustCall(onlisten));

function onlisten() {
const { port } = this.address();
https.get({ port, rejectUnauthorized: false });
}

function onplaintext(c) {
const d = new class extends Duplex {
_read(n) {
const data = c.read(n);
if (data) d.push(data);
}
_write(...xs) {
c.write(...xs);
}
}();
c.on('data', d.push.bind(d));

const options = { key, cert };
const fail = () => { throw new Error('eyecatcher'); };
tls.createServer(options, mustCall(fail)).emit('connection', d);
}
} else {
const assert = require('assert');
const { spawnSync } = require('child_process');
const result = spawnSync(process.execPath, [__filename, 'child']);
const stderr = result.stderr.toString();
const ok = stderr.includes('Error: eyecatcher');
assert(ok, stderr);
}