From b1bd9e3dd210d6b20d94c50758e12e93d9dbb3db Mon Sep 17 00:00:00 2001
From: Sam Roberts <vieuxtech@gmail.com>
Date: Thu, 23 May 2019 10:56:41 -0700
Subject: [PATCH] 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: https://github.com/nodejs/node/issues/27636

PR-URL: https://github.com/nodejs/node/pull/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>
---
 src/tls_wrap.cc                            | 12 +++++++++++-
 test/parallel/test-tls-enable-trace-cli.js | 14 ++++++++------
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index e9fe9693586ccf..17329c4b5ce56a 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -938,7 +938,17 @@ void TLSWrap::EnableTrace(
 #if HAVE_SSL_TRACE
   if (wrap->ssl_) {
     wrap->bio_trace_.reset(BIO_new_fp(stderr,  BIO_NOCLOSE | BIO_FP_TEXT));
-    SSL_set_msg_callback(wrap->ssl_.get(), SSL_trace);
+    SSL_set_msg_callback(wrap->ssl_.get(), [](int write_p, int version, int
+          content_type, const void* buf, size_t len, SSL* ssl, void* arg)
+        -> void {
+        // BIO_write(), etc., called by SSL_trace, may error. The error should
+        // be ignored, trace is a "best effort", and its usually because stderr
+        // is a non-blocking pipe, and its buffer has overflowed. Leaving errors
+        // on the stack that can get picked up by later SSL_ calls causes
+        // unwanted failures in SSL_ calls, so keep the error stack unchanged.
+        crypto::MarkPopErrorOnReturn mark_pop_error_on_return;
+        SSL_trace(write_p,  version, content_type, buf, len, ssl, arg);
+    });
     SSL_set_msg_callback_arg(wrap->ssl_.get(), wrap->bio_trace_.get());
   }
 #endif
diff --git a/test/parallel/test-tls-enable-trace-cli.js b/test/parallel/test-tls-enable-trace-cli.js
index d41334a0f51d63..4d3065e757fce6 100644
--- a/test/parallel/test-tls-enable-trace-cli.js
+++ b/test/parallel/test-tls-enable-trace-cli.js
@@ -36,8 +36,8 @@ child.on('close', common.mustCall((code, signal) => {
   assert.strictEqual(signal, null);
   assert.strictEqual(stdout.trim(), '');
   assert(/Warning: Enabling --trace-tls can expose sensitive/.test(stderr));
+  assert(/Sent Record/.test(stderr));
   assert(/Received Record/.test(stderr));
-  assert(/ClientHello/.test(stderr));
 }));
 
 function test() {
@@ -55,12 +55,14 @@ function test() {
       key: keys.agent6.key
     },
   }, common.mustCall((err, pair, cleanup) => {
-    if (err) {
-      console.error(err);
-      console.error(err.opensslErrorStack);
-      console.error(err.reason);
-      assert(err);
+    if (pair.server.err) {
+      console.trace('server', pair.server.err);
     }
+    if (pair.client.err) {
+      console.trace('client', pair.client.err);
+    }
+    assert.ifError(pair.server.err);
+    assert.ifError(pair.client.err);
 
     return cleanup();
   }));