From 5138d0753d80a2a121c5fd7c2fbcc318d3712f36 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Wed, 29 Apr 2015 13:50:36 +0200 Subject: [PATCH 1/2] 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 7fcdfd9a94d9..09c4f58b9623 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 3a9f30f279bd..b2518404a8fe 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 be5757d4a60b..540639d45805 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 c774a8490b54..79bd1fb0d847 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 af885a4c93949389acf0cefe17bf8caf26d2fce6 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Wed, 29 Apr 2015 20:23:48 +0200 Subject: [PATCH 2/2] 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 79bd1fb0d847..8c57b0a46538 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 9f095355bb58..65e270c6f15f 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,