Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 3725d4c

Browse filesBrowse files
addaleaxMylesBorins
addaleax
authored and
MylesBorins
committed
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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
1 parent 25ce458 commit 3725d4c
Copy full SHA for 3725d4c

File tree

Expand file treeCollapse file tree

4 files changed

+25
-54
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+25
-54
lines changed
Open diff view settings
Collapse file

‎src/tls_wrap.cc‎

Copy file name to clipboardExpand all lines: src/tls_wrap.cc
+24-38Lines changed: 24 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ TLSWrap::TLSWrap(Environment* env,
6262
stream_(stream),
6363
enc_in_(nullptr),
6464
enc_out_(nullptr),
65-
clear_in_(nullptr),
6665
write_size_(0),
6766
started_(false),
6867
established_(false),
@@ -95,8 +94,6 @@ TLSWrap::TLSWrap(Environment* env,
9594
TLSWrap::~TLSWrap() {
9695
enc_in_ = nullptr;
9796
enc_out_ = nullptr;
98-
delete clear_in_;
99-
clear_in_ = nullptr;
10097

10198
sc_ = nullptr;
10299

@@ -119,11 +116,6 @@ TLSWrap::~TLSWrap() {
119116
}
120117

121118

122-
void TLSWrap::MakePending() {
123-
write_callback_scheduled_ = true;
124-
}
125-
126-
127119
bool TLSWrap::InvokeQueued(int status, const char* error_str) {
128120
if (!write_callback_scheduled_)
129121
return false;
@@ -183,10 +175,6 @@ void TLSWrap::InitSSL() {
183175
// Unexpected
184176
ABORT();
185177
}
186-
187-
// Initialize ring for queud clear data
188-
clear_in_ = new crypto::NodeBIO();
189-
clear_in_->AssignEnvironment(env());
190178
}
191179

192180

@@ -302,14 +290,14 @@ void TLSWrap::EncOut() {
302290

303291
// Split-off queue
304292
if (established_ && current_write_ != nullptr)
305-
MakePending();
293+
write_callback_scheduled_ = true;
306294

307295
if (ssl_ == nullptr)
308296
return;
309297

310298
// No data to write
311299
if (BIO_pending(enc_out_) == 0) {
312-
if (clear_in_->Length() == 0)
300+
if (pending_cleartext_input_.empty())
313301
InvokeQueued(0);
314302
return;
315303
}
@@ -496,21 +484,24 @@ bool TLSWrap::ClearIn() {
496484
if (ssl_ == nullptr)
497485
return false;
498486

487+
std::vector<uv_buf_t> buffers;
488+
buffers.swap(pending_cleartext_input_);
489+
499490
crypto::MarkPopErrorOnReturn mark_pop_error_on_return;
500491

492+
size_t i;
501493
int written = 0;
502-
while (clear_in_->Length() > 0) {
503-
size_t avail = 0;
504-
char* data = clear_in_->Peek(&avail);
494+
for (i = 0; i < buffers.size(); ++i) {
495+
size_t avail = buffers[i].len;
496+
char* data = buffers[i].base;
505497
written = SSL_write(ssl_, data, avail);
506498
CHECK(written == -1 || written == static_cast<int>(avail));
507499
if (written == -1)
508500
break;
509-
clear_in_->Read(nullptr, avail);
510501
}
511502

512503
// All written
513-
if (clear_in_->Length() == 0) {
504+
if (i == buffers.size()) {
514505
CHECK_GE(written, 0);
515506
return true;
516507
}
@@ -520,9 +511,15 @@ bool TLSWrap::ClearIn() {
520511
std::string error_str;
521512
Local<Value> arg = GetSSLError(written, &err, &error_str);
522513
if (!arg.IsEmpty()) {
523-
MakePending();
514+
write_callback_scheduled_ = true;
524515
InvokeQueued(UV_EPROTO, error_str.c_str());
525-
clear_in_->Reset();
516+
} else {
517+
// Push back the not-yet-written pending buffers into their queue.
518+
// This can be skipped in the error case because no further writes
519+
// would succeed anyway.
520+
pending_cleartext_input_.insert(pending_cleartext_input_.end(),
521+
&buffers[i],
522+
&buffers[buffers.size()]);
526523
}
527524

528525
return false;
@@ -615,14 +612,6 @@ int TLSWrap::DoWrite(WriteWrap* w,
615612
return 0;
616613
}
617614

618-
// Process enqueued data first
619-
if (!ClearIn()) {
620-
// If there're still data to process - enqueue current one
621-
for (i = 0; i < count; i++)
622-
clear_in_->Write(bufs[i].base, bufs[i].len);
623-
return 0;
624-
}
625-
626615
if (ssl_ == nullptr) {
627616
ClearError();
628617
error_ = "Write after DestroySSL";
@@ -645,9 +634,9 @@ int TLSWrap::DoWrite(WriteWrap* w,
645634
if (!arg.IsEmpty())
646635
return UV_EPROTO;
647636

648-
// No errors, queue rest
649-
for (; i < count; i++)
650-
clear_in_->Write(bufs[i].base, bufs[i].len);
637+
pending_cleartext_input_.insert(pending_cleartext_input_.end(),
638+
&bufs[i],
639+
&bufs[count]);
651640
}
652641

653642
// Try writing data immediately
@@ -817,17 +806,14 @@ void TLSWrap::DestroySSL(const FunctionCallbackInfo<Value>& args) {
817806
TLSWrap* wrap;
818807
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
819808

820-
// Move all writes to pending
821-
wrap->MakePending();
809+
// If there is a write happening, mark it as finished.
810+
wrap->write_callback_scheduled_ = true;
822811

823812
// And destroy
824813
wrap->InvokeQueued(UV_ECANCELED, "Canceled because of SSL destruction");
825814

826815
// Destroy the SSL structure and friends
827816
wrap->SSLWrap<TLSWrap>::DestroySSL();
828-
829-
delete wrap->clear_in_;
830-
wrap->clear_in_ = nullptr;
831817
}
832818

833819

@@ -927,7 +913,7 @@ void TLSWrap::GetWriteQueueSize(const FunctionCallbackInfo<Value>& info) {
927913
TLSWrap* wrap;
928914
ASSIGN_OR_RETURN_UNWRAP(&wrap, info.This());
929915

930-
if (wrap->clear_in_ == nullptr) {
916+
if (wrap->ssl_ == nullptr) {
931917
info.GetReturnValue().Set(0);
932918
return;
933919
}
Collapse file

‎src/tls_wrap.h‎

Copy file name to clipboardExpand all lines: src/tls_wrap.h
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,6 @@ class TLSWrap : public AsyncWrap,
101101
void EncOutAfterWrite(WriteWrap* req_wrap, int status);
102102
bool ClearIn();
103103
void ClearOut();
104-
void MakePending();
105104
bool InvokeQueued(int status, const char* error_str = nullptr);
106105

107106
inline void Cycle() {
@@ -158,7 +157,7 @@ class TLSWrap : public AsyncWrap,
158157
StreamBase* stream_;
159158
BIO* enc_in_;
160159
BIO* enc_out_;
161-
crypto::NodeBIO* clear_in_;
160+
std::vector<uv_buf_t> pending_cleartext_input_;
162161
size_t write_size_;
163162
WriteWrap* current_write_ = nullptr;
164163
bool write_callback_scheduled_ = false;
Collapse file

‎src/util-inl.h‎

Copy file name to clipboardExpand all lines: src/util-inl.h
-13Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -99,19 +99,6 @@ ListHead<T, M>::~ListHead() {
9999
head_.next_->Remove();
100100
}
101101

102-
template <typename T, ListNode<T> (T::*M)>
103-
void ListHead<T, M>::MoveBack(ListHead* that) {
104-
if (IsEmpty())
105-
return;
106-
ListNode<T>* to = &that->head_;
107-
head_.next_->prev_ = to->prev_;
108-
to->prev_->next_ = head_.next_;
109-
head_.prev_->next_ = to;
110-
to->prev_ = head_.prev_;
111-
head_.prev_ = &head_;
112-
head_.next_ = &head_;
113-
}
114-
115102
template <typename T, ListNode<T> (T::*M)>
116103
void ListHead<T, M>::PushBack(T* element) {
117104
ListNode<T>* that = &(element->*M);
Collapse file

‎src/util.h‎

Copy file name to clipboardExpand all lines: src/util.h
-1Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,6 @@ class ListHead {
181181

182182
inline ListHead() = default;
183183
inline ~ListHead();
184-
inline void MoveBack(ListHead* that);
185184
inline void PushBack(T* element);
186185
inline void PushFront(T* element);
187186
inline bool IsEmpty() const;

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.