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.
nodejs · addaleax · Dec 27, 2017 · Dec 27, 2017 · Jan 7, 2018 · 4cbd571a0d280ffda98befd09798abdb9b821b1d
commit 4cbd571a0d280ffda98befd09798abdb9b821b1d
diff --git 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;
   }

diff --git a/src/tls_wrap.h b/src/tls_wrap.h
@@ -101,7 +101,6 @@ class TLSWrap : public AsyncWrap,
   void EncOutAfterWrite(WriteWrap* req_wrap, int status);
   bool ClearIn();
   void ClearOut();
-  void MakePending();
   bool InvokeQueued(int status, const char* error_str = nullptr);
 
   inline void Cycle() {
@@ -158,7 +157,7 @@ class TLSWrap : public AsyncWrap,
   StreamBase* stream_;
   BIO* enc_in_;
   BIO* enc_out_;
-  crypto::NodeBIO* clear_in_;
+  std::vector<uv_buf_t> pending_cleartext_input_;
   size_t write_size_;
   WriteWrap* current_write_ = nullptr;
   bool write_callback_scheduled_ = false;

diff --git a/src/util-inl.h b/src/util-inl.h
@@ -99,19 +99,6 @@ ListHead<T, M>::~ListHead() {
     head_.next_->Remove();
 }
 
-template <typename T, ListNode<T> (T::*M)>
-void ListHead<T, M>::MoveBack(ListHead* that) {
-  if (IsEmpty())
-    return;
-  ListNode<T>* to = &that->head_;
-  head_.next_->prev_ = to->prev_;
-  to->prev_->next_ = head_.next_;
-  head_.prev_->next_ = to;
-  to->prev_ = head_.prev_;
-  head_.prev_ = &head_;
-  head_.next_ = &head_;
-}
-
 template <typename T, ListNode<T> (T::*M)>
 void ListHead<T, M>::PushBack(T* element) {
   ListNode<T>* that = &(element->*M);

diff --git a/src/util.h b/src/util.h
@@ -181,7 +181,6 @@ class ListHead {
 
   inline ListHead() = default;
   inline ~ListHead();
-  inline void MoveBack(ListHead* that);
   inline void PushBack(T* element);
   inline void PushFront(T* element);
   inline bool IsEmpty() const;