From 510e6260da54209c8808a509cab657376cc61ff6 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 17 May 2017 13:36:15 +0800 Subject: [PATCH 1/3] dns: fix crash using dns.setServers after resolve4 The callback function in cares_query is synchronous and called before closed. So dns.setServers in the synchronous callback before closed will occur crashing. Fixes: https://github.com/nodejs/node/issues/894 Refs: https://github.com/nodejs/node/blob/v6.9.4/deps/cares/src/ares_process.c#L1332-L1333 Refs: https://github.com/OpenSIPS/opensips/blob/2.3.0/proxy.c PR-URL: https://github.com/nodejs/node/pull/13050 Reviewed-By: Anna Henningsen --- src/cares_wrap.cc | 161 ++++++++++++++++-- ...t-dns-setserver-in-callback-of-resolve4.js | 12 ++ 2 files changed, 162 insertions(+), 11 deletions(-) create mode 100644 test/internet/test-dns-setserver-in-callback-of-resolve4.js diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 9c25b54bd764fe..090f229c37ed4f 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -76,6 +76,7 @@ inline const char* ToErrorCodeString(int status) { V(ETIMEOUT) #undef V } + return "UNKNOWN_ARES_ERROR"; } @@ -280,6 +281,94 @@ static Local HostentToNames(Environment* env, struct hostent* host) { return scope.Escape(names); } +void safe_free_hostent(struct hostent* host) { + int idx; + + if (host->h_addr_list != nullptr) { + idx = 0; + while (host->h_addr_list[idx]) { + free(host->h_addr_list[idx++]); + } + free(host->h_addr_list); + host->h_addr_list = 0; + } + + if (host->h_aliases != nullptr) { + idx = 0; + while (host->h_aliases[idx]) { + free(host->h_aliases[idx++]); + } + free(host->h_aliases); + host->h_aliases = 0; + } + + if (host->h_name != nullptr) { + free(host->h_name); + } + + host->h_addrtype = host->h_length = 0; +} + +void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { + dest->h_addr_list = nullptr; + dest->h_addrtype = 0; + dest->h_aliases = nullptr; + dest->h_length = 0; + dest->h_name = nullptr; + + /* copy `h_name` */ + size_t name_size = strlen(src->h_name) + 1; + dest->h_name = node::Malloc(name_size); + memcpy(dest->h_name, src->h_name, name_size); + + /* copy `h_aliases` */ + size_t alias_count; + size_t cur_alias_length; + for (alias_count = 0; + src->h_aliases[alias_count] != nullptr; + alias_count++) { + } + + dest->h_aliases = node::Malloc(alias_count + 1); + for (size_t i = 0; i < alias_count; i++) { + cur_alias_length = strlen(src->h_aliases[i]); + dest->h_aliases[i] = node::Malloc(cur_alias_length + 1); + memcpy(dest->h_aliases[i], src->h_aliases[i], cur_alias_length + 1); + } + dest->h_aliases[alias_count] = nullptr; + + /* copy `h_addr_list` */ + size_t list_count; + for (list_count = 0; + src->h_addr_list[list_count] != nullptr; + list_count++) { + } + + dest->h_addr_list = node::Malloc(list_count + 1); + for (size_t i = 0; i < list_count; i++) { + dest->h_addr_list[i] = node::Malloc(src->h_length); + memcpy(dest->h_addr_list[i], src->h_addr_list[i], src->h_length); + } + dest->h_addr_list[list_count] = nullptr; + + /* work after work */ + dest->h_length = src->h_length; + dest->h_addrtype = src->h_addrtype; +} + +class QueryWrap; +struct CaresAsyncData { + QueryWrap* wrap; + int status; + bool is_host; + union { + hostent* host; + unsigned char* buf; + } data; + int len; + + uv_async_t async_handle; +}; class QueryWrap : public AsyncWrap { public: @@ -311,30 +400,80 @@ class QueryWrap : public AsyncWrap { return static_cast(this); } - static void Callback(void *arg, int status, int timeouts, - unsigned char* answer_buf, int answer_len) { - QueryWrap* wrap = static_cast(arg); + static void CaresAsyncClose(uv_handle_t* handle) { + uv_async_t* async = reinterpret_cast(handle); + auto data = static_cast(async->data); + delete data->wrap; + delete data; + } + + static void CaresAsyncCb(uv_async_t* handle) { + auto data = static_cast(handle->data); + + QueryWrap* wrap = data->wrap; + int status = data->status; if (status != ARES_SUCCESS) { wrap->ParseError(status); + } else if (!data->is_host) { + unsigned char* buf = data->data.buf; + wrap->Parse(buf, data->len); + free(buf); } else { - wrap->Parse(answer_buf, answer_len); + hostent* host = data->data.host; + wrap->Parse(host); + safe_free_hostent(host); + free(host); } - delete wrap; + uv_close(reinterpret_cast(handle), CaresAsyncClose); } static void Callback(void *arg, int status, int timeouts, - struct hostent* host) { + unsigned char* answer_buf, int answer_len) { QueryWrap* wrap = static_cast(arg); - if (status != ARES_SUCCESS) { - wrap->ParseError(status); - } else { - wrap->Parse(host); + unsigned char* buf_copy = nullptr; + if (status == ARES_SUCCESS) { + buf_copy = node::Malloc(answer_len); + memcpy(buf_copy, answer_buf, answer_len); } - delete wrap; + CaresAsyncData* data = new CaresAsyncData(); + data->status = status; + data->wrap = wrap; + data->is_host = false; + data->data.buf = buf_copy; + data->len = answer_len; + + uv_async_t* async_handle = &data->async_handle; + uv_async_init(wrap->env()->event_loop(), async_handle, CaresAsyncCb); + + async_handle->data = data; + uv_async_send(async_handle); + } + + static void Callback(void *arg, int status, int timeouts, + struct hostent* host) { + QueryWrap* wrap = static_cast(arg); + + struct hostent* host_copy = nullptr; + if (status == ARES_SUCCESS) { + host_copy = node::Malloc(1); + cares_wrap_hostent_cpy(host_copy, host); + } + + CaresAsyncData* data = new CaresAsyncData(); + data->status = status; + data->data.host = host_copy; + data->wrap = wrap; + data->is_host = true; + + uv_async_t* async_handle = &data->async_handle; + uv_async_init(wrap->env()->event_loop(), async_handle, CaresAsyncCb); + + async_handle->data = data; + uv_async_send(async_handle); } void CallOnComplete(Local answer, diff --git a/test/internet/test-dns-setserver-in-callback-of-resolve4.js b/test/internet/test-dns-setserver-in-callback-of-resolve4.js new file mode 100644 index 00000000000000..9ee853b5e4080b --- /dev/null +++ b/test/internet/test-dns-setserver-in-callback-of-resolve4.js @@ -0,0 +1,12 @@ +'use strict'; + +// We don't care about `err` in the callback function of `dns.resolve4`. We just +// want to test whether `dns.setServers` that is run after `resolve4` will cause +// a crash or not. If it doesn't crash, the test succeeded. + +const common = require('../common'); +const dns = require('dns'); + +dns.resolve4('google.com', common.mustCall(function(/* err, nameServers */) { + dns.setServers([ '8.8.8.8' ]); +})); From 9d6951959ace3233a3f99351d3db71bb73fe5d17 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Sat, 17 Jun 2017 02:36:49 +0800 Subject: [PATCH 2/3] make it compilable --- src/cares_wrap.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 090f229c37ed4f..c6ecd4370cbd3e 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -318,7 +318,7 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { /* copy `h_name` */ size_t name_size = strlen(src->h_name) + 1; - dest->h_name = node::Malloc(name_size); + dest->h_name = reinterpret_cast(node::Malloc(name_size)); memcpy(dest->h_name, src->h_name, name_size); /* copy `h_aliases` */ @@ -329,10 +329,12 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { alias_count++) { } - dest->h_aliases = node::Malloc(alias_count + 1); + dest->h_aliases = reinterpret_cast(node::Malloc((alias_count + 1) * + sizeof(char*))); for (size_t i = 0; i < alias_count; i++) { cur_alias_length = strlen(src->h_aliases[i]); - dest->h_aliases[i] = node::Malloc(cur_alias_length + 1); + dest->h_aliases[i] = reinterpret_cast(node::Malloc(cur_alias_length + + 1)); memcpy(dest->h_aliases[i], src->h_aliases[i], cur_alias_length + 1); } dest->h_aliases[alias_count] = nullptr; @@ -344,9 +346,10 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { list_count++) { } - dest->h_addr_list = node::Malloc(list_count + 1); + dest->h_addr_list = reinterpret_cast(node::Malloc((list_count + 1) * + sizeof(char*))); for (size_t i = 0; i < list_count; i++) { - dest->h_addr_list[i] = node::Malloc(src->h_length); + dest->h_addr_list[i] = reinterpret_cast(node::Malloc(src->h_length)); memcpy(dest->h_addr_list[i], src->h_addr_list[i], src->h_length); } dest->h_addr_list[list_count] = nullptr; @@ -435,7 +438,7 @@ class QueryWrap : public AsyncWrap { unsigned char* buf_copy = nullptr; if (status == ARES_SUCCESS) { - buf_copy = node::Malloc(answer_len); + buf_copy = reinterpret_cast(node::Malloc(answer_len)); memcpy(buf_copy, answer_buf, answer_len); } @@ -459,7 +462,7 @@ class QueryWrap : public AsyncWrap { struct hostent* host_copy = nullptr; if (status == ARES_SUCCESS) { - host_copy = node::Malloc(1); + host_copy = reinterpret_cast(node::Malloc(sizeof(hostent))); cares_wrap_hostent_cpy(host_copy, host); } From 4244c215dabd5efa45a8082a5a8b8f785fa475b4 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Thu, 22 Jun 2017 16:31:53 +0800 Subject: [PATCH 3/3] update after @addaleax's reviewing --- src/cares_wrap.cc | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index c6ecd4370cbd3e..75cdc599a03a60 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -318,7 +318,7 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { /* copy `h_name` */ size_t name_size = strlen(src->h_name) + 1; - dest->h_name = reinterpret_cast(node::Malloc(name_size)); + dest->h_name = static_cast(node::Malloc(name_size)); memcpy(dest->h_name, src->h_name, name_size); /* copy `h_aliases` */ @@ -329,12 +329,11 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { alias_count++) { } - dest->h_aliases = reinterpret_cast(node::Malloc((alias_count + 1) * + dest->h_aliases = static_cast(node::Malloc((alias_count + 1) * sizeof(char*))); for (size_t i = 0; i < alias_count; i++) { cur_alias_length = strlen(src->h_aliases[i]); - dest->h_aliases[i] = reinterpret_cast(node::Malloc(cur_alias_length + - 1)); + dest->h_aliases[i] = static_cast(node::Malloc(cur_alias_length + 1)); memcpy(dest->h_aliases[i], src->h_aliases[i], cur_alias_length + 1); } dest->h_aliases[alias_count] = nullptr; @@ -346,10 +345,10 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { list_count++) { } - dest->h_addr_list = reinterpret_cast(node::Malloc((list_count + 1) * + dest->h_addr_list = static_cast(node::Malloc((list_count + 1) * sizeof(char*))); for (size_t i = 0; i < list_count; i++) { - dest->h_addr_list[i] = reinterpret_cast(node::Malloc(src->h_length)); + dest->h_addr_list[i] = static_cast(node::Malloc(src->h_length)); memcpy(dest->h_addr_list[i], src->h_addr_list[i], src->h_length); } dest->h_addr_list[list_count] = nullptr; @@ -438,7 +437,7 @@ class QueryWrap : public AsyncWrap { unsigned char* buf_copy = nullptr; if (status == ARES_SUCCESS) { - buf_copy = reinterpret_cast(node::Malloc(answer_len)); + buf_copy = static_cast(node::Malloc(answer_len)); memcpy(buf_copy, answer_buf, answer_len); } @@ -462,7 +461,7 @@ class QueryWrap : public AsyncWrap { struct hostent* host_copy = nullptr; if (status == ARES_SUCCESS) { - host_copy = reinterpret_cast(node::Malloc(sizeof(hostent))); + host_copy = static_cast(node::Malloc(sizeof(hostent))); cares_wrap_hostent_cpy(host_copy, host); }