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: #17883 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Author: addaleax

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(){
 }
 
 
-voidTLSWrap::MakePending(){
- write_callback_scheduled_ = true;
-}
-
-
 boolTLSWrap::InvokeQueued(int status, constchar* error_str){
 if (!write_callback_scheduled_)
 returnfalse;
@@ -183,10 +175,6 @@ void TLSWrap::InitSSL(){
 // Unexpected
 ABORT();
  }
-
-// Initialize ring for queud clear data
- clear_in_ = newcrypto::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)
 returnfalse;
 
+ 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);
 returntrue;
  }
@@ -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()]);
  }
 
 returnfalse;
@@ -615,14 +612,6 @@ int TLSWrap::DoWrite(WriteWrap* w,
 return0;
  }
 
-// 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);
-return0;
- }
-
 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;
  }

src/tls_wrap.h

@@ -101,7 +101,6 @@ class TLSWrap : public AsyncWrap,
 voidEncOutAfterWrite(WriteWrap* req_wrap, int status);
 boolClearIn();
 voidClearOut();
-voidMakePending();
 boolInvokeQueued(int status, constchar* error_str = nullptr);
 
 inlinevoidCycle(){
@@ -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;

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);

src/util.h

@@ -181,7 +181,6 @@ class ListHead{
 
 inlineListHead() = default;
 inline~ListHead();
-inlinevoidMoveBack(ListHead* that);
 inlinevoidPushBack(T* element);
 inlinevoidPushFront(T* element);
 inlineboolIsEmpty() const;