From 3baafef544de624eecb88e992e518b7b82f2c5fa Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 4 Aug 2017 20:55:15 +0200 Subject: [PATCH 1/5] src: properly manage timer in cares ChannelWrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a bug introduced in 727b2911eca9f00cb7fa6a5f4ee8a73c7e9c94f0 where code managing the `uv_timer_t` for a `ChannelWrap` instance was left unchanged, when it should have changed the lifetime of the handle to being tied to the `ChannelWrap` instance’s lifetime. Fixes: https://github.com/nodejs/node/issues/14599 Ref: https://github.com/nodejs/node/pull/14518 --- src/cares_wrap.cc | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 327d947f3aa1cf..ecbe1e98ee1690 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -136,8 +136,9 @@ class ChannelWrap : public AsyncWrap { void Setup(); void EnsureServers(); + void CleanupTimer(); - inline uv_timer_t* timer_handle() { return &timer_handle_; } + inline uv_timer_t* timer_handle() { return timer_handle_; } inline ares_channel cares_channel() { return channel_; } inline bool query_last_ok() const { return query_last_ok_; } inline void set_query_last_ok(bool ok) { query_last_ok_ = ok; } @@ -152,7 +153,7 @@ class ChannelWrap : public AsyncWrap { static void AresTimeout(uv_timer_t* handle); private: - uv_timer_t timer_handle_; + uv_timer_t* timer_handle_; ares_channel channel_; bool query_last_ok_; bool is_servers_default_; @@ -163,6 +164,7 @@ class ChannelWrap : public AsyncWrap { ChannelWrap::ChannelWrap(Environment* env, Local object) : AsyncWrap(env, object, PROVIDER_DNSCHANNEL), + timer_handle_(nullptr), channel_(nullptr), query_last_ok_(true), is_servers_default_(true), @@ -236,7 +238,8 @@ RB_GENERATE_STATIC(node_ares_task_list, node_ares_task, node, cmp_ares_tasks) /* This is called once per second by loop->timer. It is used to constantly */ /* call back into c-ares for possibly processing timeouts. */ void ChannelWrap::AresTimeout(uv_timer_t* handle) { - ChannelWrap* channel = ContainerOf(&ChannelWrap::timer_handle_, handle); + ChannelWrap* channel = static_cast(handle->data); + CHECK_EQ(channel->timer_handle(), handle); CHECK_EQ(false, RB_EMPTY(channel->task_list())); ares_process_fd(channel->cares_channel(), ARES_SOCKET_BAD, ARES_SOCKET_BAD); } @@ -505,25 +508,29 @@ void ChannelWrap::Setup() { /* Initialize the timeout timer. The timer won't be started until the */ /* first socket is opened. */ - uv_timer_init(env()->event_loop(), &timer_handle_); - env()->RegisterHandleCleanup( - reinterpret_cast(&timer_handle_), - [](Environment* env, uv_handle_t* handle, void* arg) { - uv_close(handle, [](uv_handle_t* handle) { - ChannelWrap* channel = ContainerOf( - &ChannelWrap::timer_handle_, - reinterpret_cast(handle)); - channel->env()->FinishHandleCleanup(handle); - }); - }, - nullptr); + CleanupTimer(); + timer_handle_ = new uv_timer_t(); + timer_handle_->data = static_cast(this); + uv_timer_init(env()->event_loop(), timer_handle_); } ChannelWrap::~ChannelWrap() { if (library_inited_) ares_library_cleanup(); + ares_destroy(channel_); + CleanupTimer(); +} + +void ChannelWrap::CleanupTimer() { + if (!timer_handle_) return; + + uv_close(reinterpret_cast(timer_handle_), + [](uv_handle_t* handle) { + delete reinterpret_cast(handle); + }); + timer_handle_ = nullptr; } From e764842840c8708c7fb338cb5755be64fdbe43b2 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 5 Aug 2017 13:54:56 -0400 Subject: [PATCH 2/5] addressing indutny comments --- src/cares_wrap.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index ecbe1e98ee1690..12b3ad2f6ab890 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -523,11 +523,13 @@ ChannelWrap::~ChannelWrap() { CleanupTimer(); } + void ChannelWrap::CleanupTimer() { if (!timer_handle_) return; uv_close(reinterpret_cast(timer_handle_), [](uv_handle_t* handle) { + handle->data = NULL; delete reinterpret_cast(handle); }); timer_handle_ = nullptr; From 1fdf0ac9cdbb3beaee32234e76ce0ed390001cb8 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 5 Aug 2017 13:59:20 -0400 Subject: [PATCH 3/5] nullptr --- src/cares_wrap.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 12b3ad2f6ab890..046d6fea56ebbc 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -529,7 +529,7 @@ void ChannelWrap::CleanupTimer() { uv_close(reinterpret_cast(timer_handle_), [](uv_handle_t* handle) { - handle->data = NULL; + handle->data = nullptr; delete reinterpret_cast(handle); }); timer_handle_ = nullptr; From 75885f37f2c32d8d5a38fa479e9fcbbaa33b1204 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 Aug 2017 12:42:23 +0200 Subject: [PATCH 4/5] [squash] undo pointless change --- src/cares_wrap.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 046d6fea56ebbc..2cd8a5853335c5 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -529,7 +529,6 @@ void ChannelWrap::CleanupTimer() { uv_close(reinterpret_cast(timer_handle_), [](uv_handle_t* handle) { - handle->data = nullptr; delete reinterpret_cast(handle); }); timer_handle_ = nullptr; From 8a19e57d577c1b9124ad6c3fde041e9316f94e6e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 7 Aug 2017 12:43:46 +0200 Subject: [PATCH 5/5] [squash] style nit --- src/cares_wrap.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 2cd8a5853335c5..53a005444cc498 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -525,7 +525,7 @@ ChannelWrap::~ChannelWrap() { void ChannelWrap::CleanupTimer() { - if (!timer_handle_) return; + if (timer_handle_ == nullptr) return; uv_close(reinterpret_cast(timer_handle_), [](uv_handle_t* handle) {