tls: use after free in tls_wrap

[kfarnung]

The root cause is that req_wrap is created in StreamBase::Write
and passed to TLSWrap::DoWrite. In the TLS case the object gets
disposed and replaced with a new instance, but the caller's pointer is
never updated. When the StreamBase::Write method returns, it returns
a pointer to the freed object to the caller. In some cases when the
object memory has already been reused an assert is hit in
WriteWrap::SetAllocatedStorage because the pointer is non-null.

Refs: #18676

This was introduced in @addaleax recent PR (#18676) and hasn't propagated to any release branches yet, so I'm going ahead and opening a PR here.

In node-chakracore this was causing a pretty consistent crash only on macOS.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

tls

[kfarnung]

CI: https://ci.nodejs.org/job/node-test-pull-request/13268/
Linux rerun: https://ci.nodejs.org/job/node-test-commit-linux/16538/

[addaleax]

Hi, thanks for catching this! This would work, but the original approach there was kind of hacky anyway, and if it doesn’t work I’d probably not try to accomodate it by changing the API so that it does…

One alternative would be to not re-use the WriteWrap object, and instead potentially create a new one that’s suited for the underlying stream (which makes more sense semantically anyway):

Diff in the fold

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index f2a84b83f32d..8bd15fca01f4 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -303,10 +303,12 @@ void TLSWrap::EncOut() {
 
 
 void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) {
-  // Report back to the previous listener as well. This is only needed for the
-  // "empty" writes that are passed through directly to the underlying stream.
-  if (req_wrap != nullptr)
-    previous_listener_->OnStreamAfterWrite(req_wrap, status);
+  if (current_empty_write_ != nullptr) {
+    WriteWrap* finishing = current_empty_write_;
+    current_empty_write_ = nullptr;
+    finishing->Done(status);
+    return;
+  }
 
   if (ssl_ == nullptr)
     status = UV_ECANCELED;
@@ -572,18 +574,17 @@ int TLSWrap::DoWrite(WriteWrap* w,
     // However, if there is any data that should be written to the socket,
     // the callback should not be invoked immediately
     if (BIO_pending(enc_out_) == 0) {
-      // We destroy the current WriteWrap* object and create a new one that
-      // matches the underlying stream, rather than the TLSWrap itself.
-
-      // Note: We cannot simply use w->object() because of the "optimized"
-      // way in which we read persistent handles; the JS object itself might be
-      // destroyed by w->Dispose(), and the Local<Object> we have is not a
-      // "real" handle in the sense the V8 is aware of its existence.
-      Local<Object> req_wrap_obj =
-          w->GetAsyncWrap()->persistent().Get(env()->isolate());
-      w->Dispose();
-      w = underlying_stream()->CreateWriteWrap(req_wrap_obj);
-      return stream_->DoWrite(w, bufs, count, send_handle);
+      CHECK_EQ(current_empty_write_, nullptr);
+      current_empty_write_ = w;
+      StreamWriteResult res =
+          underlying_stream()->Write(bufs, count, send_handle);
+      if (!res.async) {
+        env()->SetImmediate([](Environment* env, void* data) {
+          TLSWrap* self = static_cast<TLSWrap*>(data);
+          self->OnStreamAfterWrite(self->current_empty_write_, 0);
+        }, this, object());
+      }
+      return 0;
     }
   }
 
diff --git a/src/tls_wrap.h b/src/tls_wrap.h
index afd19c027e70..245a6d518ac5 100644
--- a/src/tls_wrap.h
+++ b/src/tls_wrap.h
@@ -152,6 +152,7 @@ class TLSWrap : public AsyncWrap,
   std::vector<uv_buf_t> pending_cleartext_input_;
   size_t write_size_;
   WriteWrap* current_write_ = nullptr;
+  WriteWrap* current_empty_write_ = nullptr;
   bool write_callback_scheduled_ = false;
   bool started_;
   bool established_;

What do you think?

(Edit: The nicenesses of this approach are that Dispose() and DoWrite() aren’t supposed to be called by stream users anyway, whereas Write() is intended only for stream users. The downside is the added field on the TLSWrap structure)

[kfarnung]

Thanks Anna, that fix looks a lot cleaner. I'm still reviewing the content, but in the meantime I've applied it to node-chakracore and kicked off a CI run to verify (https://ci.nodejs.org/job/chakracore-test/247/).

[addaleax]

Seems a little pointless to say it, but this LGTM 😉

[kfarnung]

Applied changes to this PR and started a CI: https://ci.nodejs.org/job/node-test-pull-request/13281/

The original changes are still preserved in a commit that can be squashed out.

[kfarnung]

Looks like we have clean CIs. Thanks for the suggestion @addaleax, I wasn't feeling great about my hack, but I figured it was a good starting point. This looks a lot cleaner.

[kfarnung]

/cc @apapirovski @bnoordhuis @jasnell

Could I get a couple more eyes on this change? I'd like to land it in the next day or so.

[kfarnung]

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/13294/

[devsnek]

i just came across this crash today (on mac) on 9.5.0 release (and master i built this morning). is it possibly related/fixed by this?

node[26422]: ../src/tls_wrap.cc:621:virtual int node::TLSWrap::DoWrite(node::WriteWrap *, uv_buf_t *, size_t, uv_stream_t *): Assertion `(current_write_) == (nullptr)' failed.
 1: node::Abort() [/usr/local/bin/node]
 2: node::InternalCallbackScope::~InternalCallbackScope() [/usr/local/bin/node]
 3: node::TLSWrap::DoWrite(node::WriteWrap*, uv_buf_t*, unsigned long, uv_stream_s*) [/usr/local/bin/node]
 4: node::http2::Http2Session::SendPendingData() [/usr/local/bin/node]
 5: node::http2::Http2Session::MaybeScheduleWrite()::$_1::__invoke(node::Environment*, void*) [/usr/local/bin/node]
 6: node::Environment::RunAndClearNativeImmediates() [/usr/local/bin/node]
 7: node::Environment::CheckImmediate(uv_check_s*) [/usr/local/bin/node]
 8: uv__run_check [/usr/local/bin/node]
 9: uv_run [/usr/local/bin/node]
10: node::Start(v8::Isolate*, node::IsolateData*, int, char const* const*, int, char const* const*) [/usr/local/bin/node]
11: node::Start(uv_loop_s*, int, char const* const*, int, char const* const*) [/usr/local/bin/node]
12: node::Start(int, char**) [/usr/local/bin/node]
13: start [/usr/local/bin/node]
fish: 'node' terminated by signal SIGABRT (Abort)

[addaleax]

@devsnek Maybe, but not necessarily. Do you have a reproduction, core dump or something of that sort?

[devsnek]

@addaleax it happened while i was working on code for node-fetch, and i'm not familiar enough with the source to really know whats up with it. if it helps i added a wrap with tls.connect which should be automatically choosing between http/1.1 and h2 and i haven't updated the rest of the code base for handling an http2 response stream yet so it would be treating it as regular stream and something might be being called with the wrong arity or types (thats my guess)

[addaleax]

something might be being called with the wrong arity?

I might be wrong but it doesn’t look that way to me.

The stack trace suggests that HTTP/2 tries to schedule a write on a TLS socket while another write is currently under way, which shouldn’t be happening, and I haven’t seen that pop up anywhere.

It’s most likely a bug that’s caused by me, so I’d be more than interested to figure out how that is happening.

[addaleax]

@devsnek Also, do you know whether this just started happening with 9.5.0? Or is it older than that?

[devsnek]

@addaleax i can go back a few versions

[devsnek]

9.4.0/9.5.0/9.6.0/master -> SIGABRT

9.3.0 ->

{ FetchError: request to https://httpbin.org/get failed, reason: socket hang up
    at ClientRequest.<anonymous> (/Users/gus/Desktop/misc/node-fetch/lib/index.js:1447:15)
    at ClientRequest.emit (events.js:159:13)
    at TLSSocket.socketCloseListener (_http_client.js:362:9)
    at TLSSocket.emit (events.js:164:20)
    at _handle.close (net.js:568:12)
    at TCP.done [as _onclose] (_tls_wrap.js:379:7)
  message: 'request to https://httpbin.org/get failed, reason: socket hang up',
  type: 'system',
  errno: 'ECONNRESET',
  code: 'ECONNRESET' }

request to httpbin.org would also mean its using https.request not http2.connect.request

[addaleax]

To give an update here, we pretty much figured out what’s going on in IRC; it’s not related to this PR.

I’d land this PR by tomorrow evening if nobody objects to that, it fixes a real bug and it would be nice to have that in master.

[kfarnung]

Thanks @addaleax! I'll plan to land this tomorrow afternoon unless there are any objections.

[kfarnung]

One last CI before landing: https://ci.nodejs.org/job/node-test-pull-request/13353/

[kfarnung]

Landed in 743f890