From 46f783d74fd3c9a011b30870e11f2194e6d08af4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 27 Dec 2017 19:27:29 +0100 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/17883 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Tobias Nießen --- 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;