From 599218778dc4ca8581c4b7e7dae7db8ceff7e0f8 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Wed, 29 Apr 2015 13:50:36 +0200 Subject: [PATCH 1/3] stream_base: dispatch reqs in the stream impl Dispatch requests in the implementation of the stream, not in the code creating these requests. The requests might be piled up and invoked internally in the implementation, so it should know better when it is the time to dispatch them. In fact, TLS was doing exactly this thing which led us to... Fix: https://github.com/iojs/io.js/issues/1512 --- src/js_stream.cc | 2 ++ src/stream_base.cc | 4 ---- src/stream_wrap.cc | 6 +++++- src/tls_wrap.cc | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/js_stream.cc b/src/js_stream.cc index 7fcdfd9a94d9fd..09c4f58b96230e 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -71,6 +71,7 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) { req_wrap->object() }; + req_wrap->Dispatched(); Local res = MakeCallback(env()->onshutdown_string(), ARRAY_SIZE(argv), argv); @@ -95,6 +96,7 @@ int JSStream::DoWrite(WriteWrap* w, bufs_arr }; + w->Dispatched(); Local res = MakeCallback(env()->onwrite_string(), ARRAY_SIZE(argv), argv); diff --git a/src/stream_base.cc b/src/stream_base.cc index 3a9f30f279bd2c..b2518404a8fe62 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -60,7 +60,6 @@ int StreamBase::Shutdown(const FunctionCallbackInfo& args) { AfterShutdown); int err = DoShutdown(req_wrap); - req_wrap->Dispatched(); if (err) delete req_wrap; return err; @@ -181,7 +180,6 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { if (bufs != bufs_) delete[] bufs; - req_wrap->Dispatched(); req_wrap->object()->Set(env->async(), True(env->isolate())); req_wrap->object()->Set(env->bytes_string(), Number::New(env->isolate(), bytes)); @@ -228,7 +226,6 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& args) { req_wrap = WriteWrap::New(env, req_wrap_obj, this, AfterWrite); err = DoWrite(req_wrap, bufs, count, nullptr); - req_wrap->Dispatched(); req_wrap_obj->Set(env->async(), True(env->isolate())); if (err) @@ -347,7 +344,6 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { reinterpret_cast(send_handle)); } - req_wrap->Dispatched(); req_wrap->object()->Set(env->async(), True(env->isolate())); if (err) diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index be5757d4a60bd1..540639d458050c 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -279,7 +279,10 @@ void StreamWrap::SetBlocking(const FunctionCallbackInfo& args) { int StreamWrap::DoShutdown(ShutdownWrap* req_wrap) { - return uv_shutdown(&req_wrap->req_, stream(), AfterShutdown); + int err; + err = uv_shutdown(&req_wrap->req_, stream(), AfterShutdown); + req_wrap->Dispatched(); + return err; } @@ -353,6 +356,7 @@ int StreamWrap::DoWrite(WriteWrap* w, } } + w->Dispatched(); UpdateWriteQueueSize(); return r; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index c774a8490b54f2..79bd1fb0d847bf 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -306,7 +306,6 @@ void TLSWrap::EncOut() { for (size_t i = 0; i < count; i++) buf[i] = uv_buf_init(data[i], size[i]); int err = stream_->DoWrite(write_req, buf, count, nullptr); - write_req->Dispatched(); // Ignore errors, this should be already handled in js if (err) { @@ -558,6 +557,7 @@ int TLSWrap::DoWrite(WriteWrap* w, // Queue callback to execute it on next tick write_item_queue_.PushBack(new WriteItem(w)); + w->Dispatched(); // Write queued data if (empty) { From df28a738443da3ebde03e24e4c1af68cdf9b21c3 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Wed, 29 Apr 2015 20:23:48 +0200 Subject: [PATCH 2/3] tls: ensure no synchronous callbacks Make sure that no WriteItem's callback will be invoked synchronously. Doing so may lead to the use of uninitialized `req` object, or even worse use-after-free in the caller code. Fix: https://github.com/iojs/io.js/issues/1512 --- src/tls_wrap.cc | 30 ++++++++++++++++++++++++++++-- src/tls_wrap.h | 32 +++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 79bd1fb0d847bf..8c57b0a46538ac 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -109,7 +109,22 @@ bool TLSWrap::InvokeQueued(int status) { WriteItemList queue; pending_write_items_.MoveBack(&queue); while (WriteItem* wi = queue.PopFront()) { - wi->w_->Done(status); + if (wi->async_) { + wi->w_->Done(status); + } else { + CheckWriteItem* check = new CheckWriteItem(wi->w_, status); + int err = uv_check_init(env()->event_loop(), &check->check_); + check->check_.data = check; + if (err == 0) + err = uv_check_start(&check->check_, CheckWriteItem::CheckCb); + + // No luck today, do it on next InvokeQueued + if (err) { + delete check; + pending_write_items_.PushBack(wi); + continue; + } + } delete wi; } @@ -117,6 +132,15 @@ bool TLSWrap::InvokeQueued(int status) { } +void TLSWrap::CheckWriteItem::CheckCb(uv_check_t* check) { + CheckWriteItem* c = reinterpret_cast(check->data); + + c->w_->Done(c->status_); + uv_close(reinterpret_cast(check), nullptr); + delete c; +} + + void TLSWrap::NewSessionDoneCb() { Cycle(); } @@ -556,7 +580,9 @@ int TLSWrap::DoWrite(WriteWrap* w, } // Queue callback to execute it on next tick - write_item_queue_.PushBack(new WriteItem(w)); + WriteItem* item = new WriteItem(w); + WriteItem::SyncScope item_async(item); + write_item_queue_.PushBack(item); w->Dispatched(); // Write queued data diff --git a/src/tls_wrap.h b/src/tls_wrap.h index 9f095355bb58bd..65e270c6f15f89 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -65,16 +65,46 @@ class TLSWrap : public crypto::SSLWrap, // Write callback queue's item class WriteItem { public: - explicit WriteItem(WriteWrap* w) : w_(w) { + class SyncScope { + public: + explicit SyncScope(WriteItem* item) : item_(item) { + item_->async_ = false; + } + ~SyncScope() { + item_->async_ = true; + } + + private: + WriteItem* item_; + }; + + explicit WriteItem(WriteWrap* w) : w_(w), async_(false) { } ~WriteItem() { w_ = nullptr; } WriteWrap* w_; + bool async_; ListNode member_; }; + class CheckWriteItem { + public: + CheckWriteItem(WriteWrap* w, int status) : w_(w), status_(status) { + } + + ~CheckWriteItem() { + w_ = nullptr; + } + + static void CheckCb(uv_check_t* check); + + WriteWrap* w_; + int status_; + uv_check_t check_; + }; + TLSWrap(Environment* env, Kind kind, StreamBase* stream, From 2c5df36e61e1696fecb94f24882edd699c5b4617 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Fri, 1 May 2015 00:09:38 +0200 Subject: [PATCH 3/3] tls: check error fix --- src/tls_wrap.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 8c57b0a46538ac..b4d5a70d412573 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -119,7 +119,7 @@ bool TLSWrap::InvokeQueued(int status) { err = uv_check_start(&check->check_, CheckWriteItem::CheckCb); // No luck today, do it on next InvokeQueued - if (err) { + if (err != 0) { delete check; pending_write_items_.PushBack(wi); continue;