tls: remove cleartext input data queue

The TLS implementation previously kept a separate buffer for
incoming pieces of data, into which buffers were copied
before they were up for writing.

This removes this buffer, and replaces it with a simple list
of `uv_buf_t`s:

- The previous implementation copied all incoming data into
  that buffer, both allocating new storage and wasting time
  with copy operations. Node’s streams/net implementation
  already has to make sure that the allocated memory stays
  fresh until the write is finished, since that is what
  libuv streams rely on anyway.
- The fact that a separate kind of buffer, `crypto::NodeBIO`
  was used, was confusing: These `BIO` instances are
  only used to communicate with openssl’s streams system
  otherwise, whereas this one was purely for internal
  memory management.
- The name `clear_in_` was not very helpful.

PR-URL: https://github.com/nodejs/node/pull/17883
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

1 files changed, 24 insertions, 38 deletions

diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index 6752d16b20..78cc208105 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -62,7 +62,6 @@ TLSWrap::TLSWrap(Environment* env,
       stream_(stream),
       enc_in_(nullptr),
       enc_out_(nullptr),
-      clear_in_(nullptr),
       write_size_(0),
       started_(false),
       established_(false),
@@ -95,8 +94,6 @@ TLSWrap::TLSWrap(Environment* env,
 TLSWrap::~TLSWrap() {
   enc_in_ = nullptr;
   enc_out_ = nullptr;
-  delete clear_in_;
-  clear_in_ = nullptr;
 
   sc_ = nullptr;
 
@@ -119,11 +116,6 @@ TLSWrap::~TLSWrap() {
 }
 
 
-void TLSWrap::MakePending() {
-  write_callback_scheduled_ = true;
-}
-
-
 bool TLSWrap::InvokeQueued(int status, const char* error_str) {
   if (!write_callback_scheduled_)
     return false;
@@ -183,10 +175,6 @@ void TLSWrap::InitSSL() {
     // Unexpected
     ABORT();
   }
-
-  // Initialize ring for queud clear data
-  clear_in_ = new crypto::NodeBIO();
-  clear_in_->AssignEnvironment(env());
 }
 
 
@@ -302,14 +290,14 @@ void TLSWrap::EncOut() {
 
   // Split-off queue
   if (established_ && current_write_ != nullptr)
-    MakePending();
+    write_callback_scheduled_ = true;
 
   if (ssl_ == nullptr)
     return;
 
   // No data to write
   if (BIO_pending(enc_out_) == 0) {
-    if (clear_in_->Length() == 0)
+    if (pending_cleartext_input_.empty())
       InvokeQueued(0);
     return;
   }
@@ -496,21 +484,24 @@ bool TLSWrap::ClearIn() {
   if (ssl_ == nullptr)
     return false;
 
+  std::vector<uv_buf_t> buffers;
+  buffers.swap(pending_cleartext_input_);
+
   crypto::MarkPopErrorOnReturn mark_pop_error_on_return;
 
+  size_t i;
   int written = 0;
-  while (clear_in_->Length() > 0) {
-    size_t avail = 0;
-    char* data = clear_in_->Peek(&avail);
+  for (i = 0; i < buffers.size(); ++i) {
+    size_t avail = buffers[i].len;
+    char* data = buffers[i].base;
     written = SSL_write(ssl_, data, avail);
     CHECK(written == -1 || written == static_cast<int>(avail));
     if (written == -1)
       break;
-    clear_in_->Read(nullptr, avail);
   }
 
   // All written
-  if (clear_in_->Length() == 0) {
+  if (i == buffers.size()) {
     CHECK_GE(written, 0);
     return true;
   }
@@ -520,9 +511,15 @@ bool TLSWrap::ClearIn() {
   std::string error_str;
   Local<Value> arg = GetSSLError(written, &err, &error_str);
   if (!arg.IsEmpty()) {
-    MakePending();
+    write_callback_scheduled_ = true;
     InvokeQueued(UV_EPROTO, error_str.c_str());
-    clear_in_->Reset();
+  } else {
+    // Push back the not-yet-written pending buffers into their queue.
+    // This can be skipped in the error case because no further writes
+    // would succeed anyway.
+    pending_cleartext_input_.insert(pending_cleartext_input_.end(),
+                                    &buffers[i],
+                                    &buffers[buffers.size()]);
   }
 
   return false;
@@ -615,14 +612,6 @@ int TLSWrap::DoWrite(WriteWrap* w,
     return 0;
   }
 
-  // Process enqueued data first
-  if (!ClearIn()) {
-    // If there're still data to process - enqueue current one
-    for (i = 0; i < count; i++)
-      clear_in_->Write(bufs[i].base, bufs[i].len);
-    return 0;
-  }
-
   if (ssl_ == nullptr) {
     ClearError();
     error_ = "Write after DestroySSL";
@@ -645,9 +634,9 @@ int TLSWrap::DoWrite(WriteWrap* w,
     if (!arg.IsEmpty())
       return UV_EPROTO;
 
-    // No errors, queue rest
-    for (; i < count; i++)
-      clear_in_->Write(bufs[i].base, bufs[i].len);
+    pending_cleartext_input_.insert(pending_cleartext_input_.end(),
+                                    &bufs[i],
+                                    &bufs[count]);
   }
 
   // Try writing data immediately
@@ -817,17 +806,14 @@ void TLSWrap::DestroySSL(const FunctionCallbackInfo<Value>& args) {
   TLSWrap* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
 
-  // Move all writes to pending
-  wrap->MakePending();
+  // If there is a write happening, mark it as finished.
+  wrap->write_callback_scheduled_ = true;
 
   // And destroy
   wrap->InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction");
 
   // Destroy the SSL structure and friends
   wrap->SSLWrap<TLSWrap>::DestroySSL();
-
-  delete wrap->clear_in_;
-  wrap->clear_in_ = nullptr;
 }
 
 
@@ -927,7 +913,7 @@ void TLSWrap::GetWriteQueueSize(const FunctionCallbackInfo<Value>& info) {
   TLSWrap* wrap;
   ASSIGN_OR_RETURN_UNWRAP(&wrap, info.This());
 
-  if (wrap->clear_in_ == nullptr) {
+  if (wrap->ssl_ == nullptr) {
     info.GetReturnValue().Set(0);
     return;
   }