Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 6548b91

Browse filesBrowse files
sam-githubtargos
authored andcommitted
tls: trace errors can show up as SSL errors
Calls to TLS_trace might leave errors on the SSL error stack, which then get reported as SSL errors instead of being ignored. Wrap TLS_trace to keep the error stack unchanged. Fixes: #27636 PR-URL: #27841 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
1 parent 25eb05a commit 6548b91
Copy full SHA for 6548b91

File tree

Expand file treeCollapse file tree

2 files changed

+19
-7
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+19
-7
lines changed
Open diff view settings
Collapse file

‎src/tls_wrap.cc‎

Copy file name to clipboardExpand all lines: src/tls_wrap.cc
+11-1Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -938,7 +938,17 @@ void TLSWrap::EnableTrace(
938938
#if HAVE_SSL_TRACE
939939
if (wrap->ssl_) {
940940
wrap->bio_trace_.reset(BIO_new_fp(stderr, BIO_NOCLOSE | BIO_FP_TEXT));
941-
SSL_set_msg_callback(wrap->ssl_.get(), SSL_trace);
941+
SSL_set_msg_callback(wrap->ssl_.get(), [](int write_p, int version, int
942+
content_type, const void* buf, size_t len, SSL* ssl, void* arg)
943+
-> void {
944+
// BIO_write(), etc., called by SSL_trace, may error. The error should
945+
// be ignored, trace is a "best effort", and its usually because stderr
946+
// is a non-blocking pipe, and its buffer has overflowed. Leaving errors
947+
// on the stack that can get picked up by later SSL_ calls causes
948+
// unwanted failures in SSL_ calls, so keep the error stack unchanged.
949+
crypto::MarkPopErrorOnReturn mark_pop_error_on_return;
950+
SSL_trace(write_p, version, content_type, buf, len, ssl, arg);
951+
});
942952
SSL_set_msg_callback_arg(wrap->ssl_.get(), wrap->bio_trace_.get());
943953
}
944954
#endif
Collapse file

‎test/parallel/test-tls-enable-trace-cli.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-tls-enable-trace-cli.js
+8-6Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ child.on('close', common.mustCall((code, signal) => {
3636
assert.strictEqual(signal, null);
3737
assert.strictEqual(stdout.trim(), '');
3838
assert(/Warning: Enabling --trace-tls can expose sensitive/.test(stderr));
39+
assert(/Sent Record/.test(stderr));
3940
assert(/Received Record/.test(stderr));
40-
assert(/ClientHello/.test(stderr));
4141
}));
4242

4343
function test() {
@@ -55,12 +55,14 @@ function test() {
5555
key: keys.agent6.key
5656
},
5757
}, common.mustCall((err, pair, cleanup) => {
58-
if (err) {
59-
console.error(err);
60-
console.error(err.opensslErrorStack);
61-
console.error(err.reason);
62-
assert(err);
58+
if (pair.server.err) {
59+
console.trace('server', pair.server.err);
6360
}
61+
if (pair.client.err) {
62+
console.trace('client', pair.client.err);
63+
}
64+
assert.ifError(pair.server.err);
65+
assert.ifError(pair.client.err);
6466

6567
return cleanup();
6668
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.