From d5c3cfe066f751d4e6418090448c1adffa1ef9ef Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 27 Dec 2017 19:12:13 +0100 Subject: [PATCH 1/2] tls: refactor write queues away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TLS implementation previously had two separate queues for WriteWrap instances, one for currently active and one for finishing writes (i.e. where the encrypted output is being written to the underlying socket). However, the streams implementation in Node doesn’t allow for more than one write to be active at a time; effectively, the only possible states were that: - No write was active. - The first write queue had one item, the second one was empty. - Only the second write queue had one item, the first one was empty. To reduce overhead and implementation complexity, remove these queues, and instead store a single `WriteWrap` pointer and keep track of whether its write callback should be called on the next invocation of `InvokeQueued()` or not (which is what being in the second queue previously effected). --- src/tls_wrap.cc | 21 ++++++++++----------- src/tls_wrap.h | 18 ++---------------- 2 files changed, 12 insertions(+), 27 deletions(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index b2ef5184e077b8..6752d16b20659b 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -120,20 +120,18 @@ TLSWrap::~TLSWrap() { void TLSWrap::MakePending() { - write_item_queue_.MoveBack(&pending_write_items_); + write_callback_scheduled_ = true; } bool TLSWrap::InvokeQueued(int status, const char* error_str) { - if (pending_write_items_.IsEmpty()) + if (!write_callback_scheduled_) return false; - // Process old queue - WriteItemList queue; - pending_write_items_.MoveBack(&queue); - while (WriteItem* wi = queue.PopFront()) { - wi->w_->Done(status, error_str); - delete wi; + if (current_write_ != nullptr) { + WriteWrap* w = current_write_; + current_write_ = nullptr; + w->Done(status, error_str); } return true; @@ -303,7 +301,7 @@ void TLSWrap::EncOut() { return; // Split-off queue - if (established_ && !write_item_queue_.IsEmpty()) + if (established_ && current_write_ != nullptr) MakePending(); if (ssl_ == nullptr) @@ -606,8 +604,9 @@ int TLSWrap::DoWrite(WriteWrap* w, } } - // Queue callback to execute it on next tick - write_item_queue_.PushBack(new WriteItem(w)); + // Store the current write wrap + CHECK_EQ(current_write_, nullptr); + current_write_ = w; w->Dispatched(); // Write queued data diff --git a/src/tls_wrap.h b/src/tls_wrap.h index ed047797c2e969..ffa4fb5b4d5c31 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -90,19 +90,6 @@ class TLSWrap : public AsyncWrap, // Maximum number of buffers passed to uv_write() static const int kSimultaneousBufferCount = 10; - // Write callback queue's item - class WriteItem { - public: - explicit WriteItem(WriteWrap* w) : w_(w) { - } - ~WriteItem() { - w_ = nullptr; - } - - WriteWrap* w_; - ListNode member_; - }; - TLSWrap(Environment* env, Kind kind, StreamBase* stream, @@ -173,9 +160,8 @@ class TLSWrap : public AsyncWrap, BIO* enc_out_; crypto::NodeBIO* clear_in_; size_t write_size_; - typedef ListHead WriteItemList; - WriteItemList write_item_queue_; - WriteItemList pending_write_items_; + WriteWrap* current_write_ = nullptr; + bool write_callback_scheduled_ = false; bool started_; bool established_; bool shutdown_; From 4cbd571a0d280ffda98befd09798abdb9b821b1d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 27 Dec 2017 19:27:29 +0100 Subject: [PATCH 2/2] tls: remove cleartext input data queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/tls_wrap.cc | 62 +++++++++++++++++++------------------------------ src/tls_wrap.h | 3 +-- src/util-inl.h | 13 ----------- src/util.h | 1 - 4 files changed, 25 insertions(+), 54 deletions(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 6752d16b20659b..78cc20810532c7 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 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(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 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& 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::DestroySSL(); - - delete wrap->clear_in_; - wrap->clear_in_ = nullptr; } @@ -927,7 +913,7 @@ void TLSWrap::GetWriteQueueSize(const FunctionCallbackInfo& 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 index ffa4fb5b4d5c31..ae83c82c3226fd 100644 --- 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 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 index c3855eba098c67..558a0ab2b42611 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -99,19 +99,6 @@ ListHead::~ListHead() { head_.next_->Remove(); } -template (T::*M)> -void ListHead::MoveBack(ListHead* that) { - if (IsEmpty()) - return; - ListNode* 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 (T::*M)> void ListHead::PushBack(T* element) { ListNode* that = &(element->*M); diff --git a/src/util.h b/src/util.h index eb060a57b8801b..47bdf27c307109 100644 --- 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;