From 0dee2aded483c151db855604e65d270197f0b9e1 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 7 Mar 2015 11:41:22 -0500 Subject: [PATCH 1/4] tls: do not leak WriteWrap objects Kill WriteWrap instances that are allocated in `tls_wrap.cc` internally. Fix: https://github.com/iojs/io.js/issues/1075 --- src/tls_wrap.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index a42e5faa70d8ae..e3dc1e18853d5e 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -305,6 +305,7 @@ void TLSWrap::EncOut() { for (size_t i = 0; i < count; i++) buf[i] = uv_buf_init(data[i], size[i]); int r = stream_->DoWrite(write_req, buf, count, nullptr); + write_req->Dispatched(); // Ignore errors, this should be already handled in js if (!r) @@ -314,6 +315,8 @@ void TLSWrap::EncOut() { void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) { TLSWrap* wrap = req_wrap->wrap()->Cast(); + req_wrap->~WriteWrap(); + delete[] reinterpret_cast(req_wrap); // Handle error if (status) { From 441eb0663300b557876c9e8d478c7cc82446bec3 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 7 Mar 2015 16:42:02 -0500 Subject: [PATCH 2/4] stream_base: WriteWrap::New/::Dispose --- src/stream_base-inl.h | 24 ++++++++++++++++++++++ src/stream_base.cc | 46 +++++++++++++++++-------------------------- src/stream_base.h | 25 ++++++++++++++--------- src/tls_wrap.cc | 12 +++++------ 4 files changed, 63 insertions(+), 44 deletions(-) diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 46d9f78905f2eb..b3673d4932ad6c 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -15,6 +15,7 @@ using v8::FunctionTemplate; using v8::Handle; using v8::HandleScope; using v8::Local; +using v8::Object; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; using v8::String; @@ -82,6 +83,29 @@ void StreamBase::JSMethod(const FunctionCallbackInfo& args) { args.GetReturnValue().Set((wrap->*Method)(args)); } + +WriteWrap* WriteWrap::New(Environment* env, + Local obj, + StreamBase* wrap, + DoneCb cb, + int extra) { + int storage_size = ROUND_UP(sizeof(WriteWrap), 16) + extra; + char* storage = new char[storage_size]; + + return new(storage) WriteWrap(env, obj, wrap, cb); +} + + +void WriteWrap::Dispose() { + this->~WriteWrap(); + delete[] reinterpret_cast(this); +} + + +char* WriteWrap::Extra(int offset) { + return reinterpret_cast(this) + ROUND_UP(sizeof(*this), 16) + offset; +} + } // namespace node #endif // SRC_STREAM_BASE_INL_H_ diff --git a/src/stream_base.cc b/src/stream_base.cc index bd963bc3433b93..d846b2a177ff84 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -1,4 +1,5 @@ #include "stream_base.h" +#include "stream_base-inl.h" #include "stream_wrap.h" #include "node.h" @@ -133,13 +134,14 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { if (ARRAY_SIZE(bufs_) < count) bufs = new uv_buf_t[count]; - storage_size += sizeof(WriteWrap); - char* storage = new char[storage_size]; - WriteWrap* req_wrap = - new(storage) WriteWrap(env, req_wrap_obj, this, AfterWrite); + WriteWrap* req_wrap = WriteWrap::New(env, + req_wrap_obj, + this, + AfterWrite, + storage_size); uint32_t bytes = 0; - size_t offset = sizeof(WriteWrap); + size_t offset = 0; for (size_t i = 0; i < count; i++) { Handle chunk = chunks->Get(i * 2); @@ -154,7 +156,7 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { // Write string offset = ROUND_UP(offset, 16); CHECK_LT(offset, storage_size); - char* str_storage = storage + offset; + char* str_storage = req_wrap->Extra(offset); size_t str_size = storage_size - offset; Handle string = chunk->ToString(env->isolate()); @@ -187,10 +189,8 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { ClearError(); } - if (err) { - req_wrap->~WriteWrap(); - delete[] storage; - } + if (err) + req_wrap->Dispose(); return err; } @@ -207,7 +207,6 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& args) { const char* data = Buffer::Data(args[1]); size_t length = Buffer::Length(args[1]); - char* storage; WriteWrap* req_wrap; uv_buf_t buf; buf.base = const_cast(data); @@ -224,17 +223,14 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo& args) { CHECK_EQ(count, 1); // Allocate, or write rest - storage = new char[sizeof(WriteWrap)]; - req_wrap = new(storage) WriteWrap(env, req_wrap_obj, this, AfterWrite); + 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) { - req_wrap->~WriteWrap(); - delete[] storage; - } + if (err) + req_wrap->Dispose(); done: const char* msg = Error(); @@ -275,7 +271,6 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { return UV_ENOBUFS; // Try writing immediately if write size isn't too big - char* storage; WriteWrap* req_wrap; char* data; char stack_storage[16384]; // 16kb @@ -308,11 +303,9 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { CHECK_EQ(count, 1); } - storage = new char[sizeof(WriteWrap) + storage_size + 15]; - req_wrap = new(storage) WriteWrap(env, req_wrap_obj, this, AfterWrite); + req_wrap = WriteWrap::New(env, req_wrap_obj, this, AfterWrite, storage_size); - data = reinterpret_cast(ROUND_UP( - reinterpret_cast(storage) + sizeof(WriteWrap), 16)); + data = req_wrap->Extra(); if (try_write) { // Copy partial data @@ -355,10 +348,8 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { req_wrap->Dispatched(); req_wrap->object()->Set(env->async(), True(env->isolate())); - if (err) { - req_wrap->~WriteWrap(); - delete[] storage; - } + if (err) + req_wrap->Dispose(); done: const char* msg = Error(); @@ -404,8 +395,7 @@ void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) { if (req_wrap->object()->Has(env->oncomplete_string())) req_wrap->MakeCallback(env->oncomplete_string(), ARRAY_SIZE(argv), argv); - req_wrap->~WriteWrap(); - delete[] reinterpret_cast(req_wrap); + req_wrap->Dispose(); } diff --git a/src/stream_base.h b/src/stream_base.h index 5718f07ae14d29..91205de85a7427 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -56,6 +56,21 @@ class ShutdownWrap : public ReqWrap, class WriteWrap: public ReqWrap, public StreamReq { public: + static inline WriteWrap* New(Environment* env, + v8::Local obj, + StreamBase* wrap, + DoneCb cb, + int extra = 0); + inline void Dispose(); + inline char* Extra(int offset = 0); + + inline StreamBase* wrap() const { return wrap_; } + + static void NewWriteWrap(const v8::FunctionCallbackInfo& args) { + CHECK(args.IsConstructCall()); + } + + protected: WriteWrap(Environment* env, v8::Local obj, StreamBase* wrap, @@ -66,24 +81,16 @@ class WriteWrap: public ReqWrap, Wrap(obj, this); } + void* operator new(size_t size) = delete; void* operator new(size_t size, char* storage) { return storage; } // This is just to keep the compiler happy. It should never be called, since // we don't use exceptions in node. void operator delete(void* ptr, char* storage) { UNREACHABLE(); } - inline StreamBase* wrap() const { - return wrap_; - } - - static void NewWriteWrap(const v8::FunctionCallbackInfo& args) { - CHECK(args.IsConstructCall()); - } - private: // People should not be using the non-placement new and delete operator on a // WriteWrap. Ensure this never happens. - void* operator new(size_t size) { UNREACHABLE(); } void operator delete(void* ptr) { UNREACHABLE(); } StreamBase* const wrap_; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index e3dc1e18853d5e..c5d5d554d86d8b 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -295,11 +295,10 @@ void TLSWrap::EncOut() { Local req_wrap_obj = env()->write_wrap_constructor_function()->NewInstance(); - char* storage = new char[sizeof(WriteWrap)]; - WriteWrap* write_req = new(storage) WriteWrap(env(), - req_wrap_obj, - this, - EncOutCb); + WriteWrap* write_req = WriteWrap::New(env(), + req_wrap_obj, + this, + EncOutCb); uv_buf_t buf[ARRAY_SIZE(data)]; for (size_t i = 0; i < count; i++) @@ -315,8 +314,7 @@ void TLSWrap::EncOut() { void TLSWrap::EncOutCb(WriteWrap* req_wrap, int status) { TLSWrap* wrap = req_wrap->wrap()->Cast(); - req_wrap->~WriteWrap(); - delete[] reinterpret_cast(req_wrap); + req_wrap->Dispose(); // Handle error if (status) { From 92b1de86ec67f8f6124207f1d1f976e8dfafe392 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 7 Mar 2015 17:12:10 -0500 Subject: [PATCH 3/4] stream_base: remove left-overs of +15 thing --- src/stream_base.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/stream_base.cc b/src/stream_base.cc index d846b2a177ff84..4ff284b5c252a3 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -109,6 +109,8 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { // Determine storage size first size_t storage_size = 0; for (size_t i = 0; i < count; i++) { + storage_size = ROUND_UP(storage_size, 16); + Handle chunk = chunks->Get(i * 2); if (Buffer::HasInstance(chunk)) @@ -125,7 +127,7 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { else chunk_size = StringBytes::StorageSize(env->isolate(), string, encoding); - storage_size += chunk_size + 15; + storage_size += chunk_size; } if (storage_size > INT_MAX) @@ -277,7 +279,7 @@ int StreamBase::WriteString(const FunctionCallbackInfo& args) { size_t data_size; uv_buf_t buf; - bool try_write = storage_size + 15 <= sizeof(stack_storage) && + bool try_write = storage_size <= sizeof(stack_storage) && (!IsIPCPipe() || send_handle_obj.IsEmpty()); if (try_write) { data_size = StringBytes::Write(env->isolate(), From b26d411996364a32e6f9773077663c0ec9b0eacc Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 7 Mar 2015 17:17:18 -0500 Subject: [PATCH 4/4] stream_base: WriteWrap::kAlignSize --- src/stream_base-inl.h | 6 ++++-- src/stream_base.cc | 4 ++-- src/stream_base.h | 2 ++ 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index b3673d4932ad6c..38ee3b2a5d24bd 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -89,7 +89,7 @@ WriteWrap* WriteWrap::New(Environment* env, StreamBase* wrap, DoneCb cb, int extra) { - int storage_size = ROUND_UP(sizeof(WriteWrap), 16) + extra; + int storage_size = ROUND_UP(sizeof(WriteWrap), kAlignSize) + extra; char* storage = new char[storage_size]; return new(storage) WriteWrap(env, obj, wrap, cb); @@ -103,7 +103,9 @@ void WriteWrap::Dispose() { char* WriteWrap::Extra(int offset) { - return reinterpret_cast(this) + ROUND_UP(sizeof(*this), 16) + offset; + return reinterpret_cast(this) + + ROUND_UP(sizeof(*this), kAlignSize) + + offset; } } // namespace node diff --git a/src/stream_base.cc b/src/stream_base.cc index 4ff284b5c252a3..3a9f30f279bd2c 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -109,7 +109,7 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { // Determine storage size first size_t storage_size = 0; for (size_t i = 0; i < count; i++) { - storage_size = ROUND_UP(storage_size, 16); + storage_size = ROUND_UP(storage_size, WriteWrap::kAlignSize); Handle chunk = chunks->Get(i * 2); @@ -156,7 +156,7 @@ int StreamBase::Writev(const FunctionCallbackInfo& args) { } // Write string - offset = ROUND_UP(offset, 16); + offset = ROUND_UP(offset, WriteWrap::kAlignSize); CHECK_LT(offset, storage_size); char* str_storage = req_wrap->Extra(offset); size_t str_size = storage_size - offset; diff --git a/src/stream_base.h b/src/stream_base.h index 91205de85a7427..34ac5fe65df934 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -70,6 +70,8 @@ class WriteWrap: public ReqWrap, CHECK(args.IsConstructCall()); } + static const int kAlignSize = 16; + protected: WriteWrap(Environment* env, v8::Local obj,