From 0c51d0a2b66c413f07a3ca3552b6ae5b33029917 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 17 May 2017 13:36:15 +0800 Subject: [PATCH 1/6] dns: fix crash while using dns.setServers after dns.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 --- src/cares_wrap.cc | 190 +++++++++++++++++- ...t-dns-setserver-in-callback-of-resolve4.js | 17 ++ 2 files changed, 198 insertions(+), 9 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 15c261b6f165f5..3cd1eecb2d9750 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -69,6 +69,8 @@ using v8::Value; namespace { +#define ARES_HOSTENT_COPY_ERROR -1000 + inline const char* ToErrorCodeString(int status) { switch (status) { #define V(code) case ARES_##code: return #code; @@ -96,8 +98,10 @@ inline const char* ToErrorCodeString(int status) { V(EREFUSED) V(ESERVFAIL) V(ETIMEOUT) + V(HOSTENT_COPY_ERROR) #undef V } + return "UNKNOWN_ARES_ERROR"; } @@ -296,6 +300,121 @@ Local HostentToNames(Environment* env, struct hostent* host) { return scope.Escape(names); } +/* copies a hostent structure*, returns 0 on success, -1 on error */ +/* this function refers to OpenSIPS */ +int cares_wrap_hostent_cpy(struct hostent *dst, struct hostent* src) { + unsigned int len, len2, i, r; + + /* start copying the host entry.. */ + /* copy h_name */ + len = strlen(src->h_name) + 1; + dst->h_name = reinterpret_cast(node::Malloc(sizeof(char) * len)); + if (dst->h_name) { + strncpy(dst->h_name, src->h_name, len); + } else { + return -1; + } + + /* copy h_aliases */ + len = 0; + if (src->h_aliases) { + for (; src->h_aliases[len]; len++) {} + } + + dst->h_aliases = reinterpret_cast( + node::Malloc(sizeof(char*) * (len + 1))); + + if (dst->h_aliases == 0) { + free(dst->h_name); + return -1; + } + + memset(reinterpret_cast(dst->h_aliases), 0, sizeof(char*) * (len + 1)); + for (i = 0; i < len; i++) { + len2 = strlen(src->h_aliases[i]) + 1; + dst->h_aliases[i] = reinterpret_cast( + node::Malloc(sizeof(char) * len2)); + + if (dst->h_aliases[i] == 0) { + free(dst->h_name); + for (r = 0; r < i; r++) free(dst->h_aliases[r]); + free(dst->h_aliases); + return -1; + } + + strncpy(dst->h_aliases[i], src->h_aliases[i], len2); + } + + /* copy h_addr_list */ + len = 0; + if (src->h_addr_list) { + for (; src->h_addr_list[len]; len++) {} + } + + dst->h_addr_list = reinterpret_cast( + node::Malloc(sizeof(char*) * (len + 1))); + + if (dst->h_addr_list == 0) { + free(dst->h_name); + for (r = 0; dst->h_aliases[r]; r++) free(dst->h_aliases[r]); + free(dst->h_aliases); + return -1; + } + + memset(reinterpret_cast( + dst->h_addr_list), 0, sizeof(char*) * (len + 1)); + + for (i=0; i < len; i++) { + dst->h_addr_list[i] = reinterpret_cast( + node::Malloc(sizeof(char) * src->h_length)); + if (dst->h_addr_list[i] == 0) { + free(dst->h_name); + for (r = 0; dst->h_aliases[r]; r++) free(dst->h_aliases[r]); + free(dst->h_aliases); + for (r = 0; r < i; r++) free(dst->h_addr_list[r]); + free(dst->h_addr_list); + return -1; + } + + memcpy(dst->h_addr_list[i], src->h_addr_list[i], src->h_length); + } + + /* copy h_addr_type & length */ + dst->h_addrtype = src->h_addrtype; + dst->h_length = src->h_length; + /*finished hostent copy */ + + return 0; +} + +/* this function refers to OpenSIPS */ +void cares_wrap_free_hostent(struct hostent *dst) { + int r; + if (dst->h_name) free(dst->h_name); + if (dst->h_aliases) { + for (r = 0; dst->h_aliases[r]; r++) { + free(dst->h_aliases[r]); + } + free(dst->h_aliases); + } + if (dst->h_addr_list) { + for (r = 0; dst->h_addr_list[r]; r++) { + free(dst->h_addr_list[r]); + } + free(dst->h_addr_list); + } +} + +class QueryWrap; +struct CaresAsyncData { + QueryWrap* wrap; + int status; + bool is_host; + void* buf; + int len; + + uv_async_t async_handle; +}; class QueryWrap : public AsyncWrap { public: @@ -328,30 +447,83 @@ 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); + struct CaresAsyncData* data = + static_cast(async->data); + delete data->wrap; + delete data; + } + + static void CaresAsyncCb(uv_async_t* handle) { + struct CaresAsyncData* 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 = reinterpret_cast(data->buf); + wrap->Parse(buf, data->len); + free(buf); } else { - wrap->Parse(answer_buf, answer_len); + hostent* host = static_cast(data->buf); + wrap->Parse(host); + cares_wrap_free_hostent(host); + free(host); } - delete wrap; + uv_close(reinterpret_cast(handle), CaresAsyncClose); + } + + static void Callback(void *arg, int status, int timeouts, + unsigned char* answer_buf, int answer_len) { + QueryWrap* wrap = static_cast(arg); + + unsigned char* buf_copy = (unsigned char*)node::Malloc( + sizeof(unsigned char) * answer_len); + memcpy(buf_copy, answer_buf, sizeof(unsigned char) * answer_len); + + struct CaresAsyncData* data = new struct CaresAsyncData(); + data->status = status; + data->wrap = wrap; + data->is_host = false; + 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); - if (status != ARES_SUCCESS) { - wrap->ParseError(status); + struct hostent* host_copy = (struct hostent*)node::Malloc(sizeof(hostent)); + int ret = cares_wrap_hostent_cpy(host_copy, host); + + struct CaresAsyncData* data = new struct CaresAsyncData(); + if (ret == 0) { + data->status = status; + data->buf = host_copy; } else { - wrap->Parse(host); + data->status = ARES_HOSTENT_COPY_ERROR; + data->buf = nullptr; + free(host_copy); } + data->wrap = wrap; + data->is_host = true; - delete wrap; + 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..c83bf39842969c --- /dev/null +++ b/test/internet/test-dns-setserver-in-callback-of-resolve4.js @@ -0,0 +1,17 @@ +'use strict'; + +const common = require('../common'); +const dns = require('dns'); + +dns.resolve4('google.com', common.mustCall(function(/* err, nameServers */) { + // do not care about `err` and `nameServers`, + // both failed and succeeded would be OK. + // + // just to test crash + + dns.setServers([ '8.8.8.8' ]); + + // the test case shouldn't crash here + // see https://github.com/nodejs/node/pull/13050 and + // https://github.com/nodejs/node/issues/894 +})); From c6ac52921d98482d4754eef787171f4f9644cdb1 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 17 May 2017 14:24:05 +0800 Subject: [PATCH 2/6] fix a bug may occure memory leaking (this commit will be rebased) --- src/cares_wrap.cc | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 3cd1eecb2d9750..5afb59e8fdbefc 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -482,9 +482,12 @@ class QueryWrap : public AsyncWrap { unsigned char* answer_buf, int answer_len) { QueryWrap* wrap = static_cast(arg); - unsigned char* buf_copy = (unsigned char*)node::Malloc( - sizeof(unsigned char) * answer_len); - memcpy(buf_copy, answer_buf, sizeof(unsigned char) * answer_len); + unsigned char* buf_copy = nullptr; + if (status == ARES_SUCCESS) { + buf_copy = (unsigned char*)node::Malloc( + sizeof(unsigned char) * answer_len); + memcpy(buf_copy, answer_buf, sizeof(unsigned char) * answer_len); + } struct CaresAsyncData* data = new struct CaresAsyncData(); data->status = status; @@ -504,11 +507,16 @@ class QueryWrap : public AsyncWrap { struct hostent* host) { QueryWrap* wrap = static_cast(arg); - struct hostent* host_copy = (struct hostent*)node::Malloc(sizeof(hostent)); - int ret = cares_wrap_hostent_cpy(host_copy, host); + struct hostent* host_copy = nullptr; + int copy_ret = 0; + + if (status == ARES_SUCCESS) { + host_copy = (struct hostent*)node::Malloc(sizeof(hostent)); + copy_ret = cares_wrap_hostent_cpy(host_copy, host); + } struct CaresAsyncData* data = new struct CaresAsyncData(); - if (ret == 0) { + if (copy_ret == 0) { data->status = status; data->buf = host_copy; } else { From fd610dba4aaf8e298c08172f0612b9f99d299cf4 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 17 May 2017 21:32:04 +0800 Subject: [PATCH 3/6] rewrite cares_wrap_hostent_cpy by myself --- src/cares_wrap.cc | 175 +++++++++++++++++++++++----------------------- 1 file changed, 89 insertions(+), 86 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 5afb59e8fdbefc..9bb0f1a615a6c0 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -300,109 +300,112 @@ Local HostentToNames(Environment* env, struct hostent* host) { return scope.Escape(names); } -/* copies a hostent structure*, returns 0 on success, -1 on error */ -/* this function refers to OpenSIPS */ -int cares_wrap_hostent_cpy(struct hostent *dst, struct hostent* src) { - unsigned int len, len2, i, r; - - /* start copying the host entry.. */ - /* copy h_name */ - len = strlen(src->h_name) + 1; - dst->h_name = reinterpret_cast(node::Malloc(sizeof(char) * len)); - if (dst->h_name) { - strncpy(dst->h_name, src->h_name, len); - } else { - return -1; - } +void safe_free_hostent(struct hostent* host) { + int idx; - /* copy h_aliases */ - len = 0; - if (src->h_aliases) { - for (; src->h_aliases[len]; len++) {} + if (host->h_addr_list) { + idx = 0; + while (host->h_addr_list[idx]) { + free(host->h_addr_list[idx++]); + } + free(host->h_addr_list); + host->h_addr_list = 0; } - dst->h_aliases = reinterpret_cast( - node::Malloc(sizeof(char*) * (len + 1))); + if (host->h_aliases) { + idx = 0; + while (host->h_aliases[idx]) { + free(host->h_aliases[idx++]); + } + free(host->h_aliases); + host->h_aliases = 0; + } - if (dst->h_aliases == 0) { - free(dst->h_name); - return -1; + if (host->h_name) { + free(host->h_name); } - memset(reinterpret_cast(dst->h_aliases), 0, sizeof(char*) * (len + 1)); - for (i = 0; i < len; i++) { - len2 = strlen(src->h_aliases[i]) + 1; - dst->h_aliases[i] = reinterpret_cast( - node::Malloc(sizeof(char) * len2)); + host->h_addrtype = host->h_length = 0; +} - if (dst->h_aliases[i] == 0) { - free(dst->h_name); - for (r = 0; r < i; r++) free(dst->h_aliases[r]); - free(dst->h_aliases); - return -1; +bool 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` */ + unsigned int name_size = (strlen(src->h_name) + 1) * sizeof(char); + dest->h_name = reinterpret_cast(node::Malloc(name_size)); + if (!dest->h_name) return false; + memcpy(dest->h_name, src->h_name, name_size); + + /* copy `h_aliases` */ + unsigned int alias_count; + unsigned int alias_size; + unsigned int cur_alias_length; + for (alias_count = 0; + src->h_aliases[alias_count] != nullptr; + alias_count++) { + } + alias_size = sizeof(char*) * (alias_count + 1); + + dest->h_aliases = reinterpret_cast(node::Malloc(alias_size)); + if (dest->h_aliases == nullptr) { + safe_free_hostent(dest); + return false; + } + + memset(reinterpret_cast(dest->h_aliases), 0, alias_size); + for (unsigned int i = 0; i < alias_count; i++) { + cur_alias_length = strlen(src->h_aliases[i]); + dest->h_aliases[i] = reinterpret_cast( + node::Malloc(sizeof(char) * (cur_alias_length + 1))); + + if (dest->h_aliases[i] == nullptr) { + safe_free_hostent(dest); + return false; } - strncpy(dst->h_aliases[i], src->h_aliases[i], len2); + memcpy(dest->h_aliases[i], + src->h_aliases[i], + sizeof(char) * (cur_alias_length + 1)); } - /* copy h_addr_list */ - len = 0; - if (src->h_addr_list) { - for (; src->h_addr_list[len]; len++) {} + /* copy `h_addr_list` */ + unsigned int list_count; + unsigned int list_size; + unsigned int node_size; + for (list_count = 0; + src->h_addr_list[list_count] != nullptr; + list_count++) { } + list_size = sizeof(char*) * (list_count + 1); - dst->h_addr_list = reinterpret_cast( - node::Malloc(sizeof(char*) * (len + 1))); - - if (dst->h_addr_list == 0) { - free(dst->h_name); - for (r = 0; dst->h_aliases[r]; r++) free(dst->h_aliases[r]); - free(dst->h_aliases); - return -1; + dest->h_addr_list = reinterpret_cast(node::Malloc(list_size)); + if (dest->h_addr_list == nullptr) { + safe_free_hostent(dest); + return false; } - memset(reinterpret_cast( - dst->h_addr_list), 0, sizeof(char*) * (len + 1)); - - for (i=0; i < len; i++) { - dst->h_addr_list[i] = reinterpret_cast( - node::Malloc(sizeof(char) * src->h_length)); - if (dst->h_addr_list[i] == 0) { - free(dst->h_name); - for (r = 0; dst->h_aliases[r]; r++) free(dst->h_aliases[r]); - free(dst->h_aliases); - for (r = 0; r < i; r++) free(dst->h_addr_list[r]); - free(dst->h_addr_list); - return -1; + node_size = sizeof(char) * src->h_length; + memset(reinterpret_cast(dest->h_addr_list), 0, list_size); + for (unsigned int i = 0; i < list_count; i++) { + dest->h_addr_list[i] = reinterpret_cast(node::Malloc(node_size)); + if (dest->h_addr_list[i] == nullptr) { + safe_free_hostent(dest); + return false; } - memcpy(dst->h_addr_list[i], src->h_addr_list[i], src->h_length); + memcpy(dest->h_addr_list[i], src->h_addr_list, node_size); } - /* copy h_addr_type & length */ - dst->h_addrtype = src->h_addrtype; - dst->h_length = src->h_length; - /*finished hostent copy */ - - return 0; -} + /* work after work */ + dest->h_length = src->h_length; + dest->h_addrtype = src->h_addrtype; -/* this function refers to OpenSIPS */ -void cares_wrap_free_hostent(struct hostent *dst) { - int r; - if (dst->h_name) free(dst->h_name); - if (dst->h_aliases) { - for (r = 0; dst->h_aliases[r]; r++) { - free(dst->h_aliases[r]); - } - free(dst->h_aliases); - } - if (dst->h_addr_list) { - for (r = 0; dst->h_addr_list[r]; r++) { - free(dst->h_addr_list[r]); - } - free(dst->h_addr_list); - } + return true; } class QueryWrap; @@ -471,7 +474,7 @@ class QueryWrap : public AsyncWrap { } else { hostent* host = static_cast(data->buf); wrap->Parse(host); - cares_wrap_free_hostent(host); + safe_free_hostent(host); free(host); } @@ -508,7 +511,7 @@ class QueryWrap : public AsyncWrap { QueryWrap* wrap = static_cast(arg); struct hostent* host_copy = nullptr; - int copy_ret = 0; + bool copy_ret = false; if (status == ARES_SUCCESS) { host_copy = (struct hostent*)node::Malloc(sizeof(hostent)); @@ -516,7 +519,7 @@ class QueryWrap : public AsyncWrap { } struct CaresAsyncData* data = new struct CaresAsyncData(); - if (copy_ret == 0) { + if (copy_ret) { data->status = status; data->buf = host_copy; } else { From 56aff34e358ea3d7a1a7015d5d289bdbc04bd8f8 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 17 May 2017 22:56:21 +0800 Subject: [PATCH 4/6] update after review (this commit will be rebased) --- src/cares_wrap.cc | 70 +++++-------------- ...t-dns-setserver-in-callback-of-resolve4.js | 14 ++-- 2 files changed, 23 insertions(+), 61 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 9bb0f1a615a6c0..557eff0d0cbeac 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -336,69 +336,38 @@ bool cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { dest->h_name = nullptr; /* copy `h_name` */ - unsigned int name_size = (strlen(src->h_name) + 1) * sizeof(char); - dest->h_name = reinterpret_cast(node::Malloc(name_size)); - if (!dest->h_name) return false; + 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` */ - unsigned int alias_count; - unsigned int alias_size; - unsigned int cur_alias_length; + size_t alias_count; + size_t cur_alias_length; for (alias_count = 0; src->h_aliases[alias_count] != nullptr; alias_count++) { } - alias_size = sizeof(char*) * (alias_count + 1); - dest->h_aliases = reinterpret_cast(node::Malloc(alias_size)); - if (dest->h_aliases == nullptr) { - safe_free_hostent(dest); - return false; - } - - memset(reinterpret_cast(dest->h_aliases), 0, alias_size); - for (unsigned int i = 0; i < alias_count; i++) { + dest->h_aliases = node::Malloc(alias_count + 1); + memset(dest->h_aliases, 0, sizeof(char*) * (alias_count + 1)); + 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(sizeof(char) * (cur_alias_length + 1))); - - if (dest->h_aliases[i] == nullptr) { - safe_free_hostent(dest); - return false; - } - - memcpy(dest->h_aliases[i], - src->h_aliases[i], - sizeof(char) * (cur_alias_length + 1)); + dest->h_aliases[i] = node::Malloc(cur_alias_length + 1); + memcpy(dest->h_aliases[i], src->h_aliases[i], cur_alias_length + 1); } /* copy `h_addr_list` */ unsigned int list_count; - unsigned int list_size; - unsigned int node_size; for (list_count = 0; src->h_addr_list[list_count] != nullptr; list_count++) { } - list_size = sizeof(char*) * (list_count + 1); - dest->h_addr_list = reinterpret_cast(node::Malloc(list_size)); - if (dest->h_addr_list == nullptr) { - safe_free_hostent(dest); - return false; - } - - node_size = sizeof(char) * src->h_length; - memset(reinterpret_cast(dest->h_addr_list), 0, list_size); + dest->h_addr_list = node::Malloc(list_count + 1); + memset(dest->h_addr_list, 0, sizeof(char*) * (list_count + 1)); for (unsigned int i = 0; i < list_count; i++) { - dest->h_addr_list[i] = reinterpret_cast(node::Malloc(node_size)); - if (dest->h_addr_list[i] == nullptr) { - safe_free_hostent(dest); - return false; - } - - memcpy(dest->h_addr_list[i], src->h_addr_list, node_size); + dest->h_addr_list[i] = node::Malloc(src->h_length); + memcpy(dest->h_addr_list[i], src->h_addr_list[i], src->h_length); } /* work after work */ @@ -452,15 +421,13 @@ class QueryWrap : public AsyncWrap { static void CaresAsyncClose(uv_handle_t* handle) { uv_async_t* async = reinterpret_cast(handle); - struct CaresAsyncData* data = - static_cast(async->data); + auto data = static_cast(async->data); delete data->wrap; delete data; } static void CaresAsyncCb(uv_async_t* handle) { - struct CaresAsyncData* data = - static_cast(handle->data); + auto data = static_cast(handle->data); QueryWrap* wrap = data->wrap; int status = data->status; @@ -487,9 +454,8 @@ class QueryWrap : public AsyncWrap { unsigned char* buf_copy = nullptr; if (status == ARES_SUCCESS) { - buf_copy = (unsigned char*)node::Malloc( - sizeof(unsigned char) * answer_len); - memcpy(buf_copy, answer_buf, sizeof(unsigned char) * answer_len); + buf_copy = node::Malloc(answer_len); + memcpy(buf_copy, answer_buf, answer_len); } struct CaresAsyncData* data = new struct CaresAsyncData(); @@ -514,7 +480,7 @@ class QueryWrap : public AsyncWrap { bool copy_ret = false; if (status == ARES_SUCCESS) { - host_copy = (struct hostent*)node::Malloc(sizeof(hostent)); + host_copy = node::Malloc(1); copy_ret = cares_wrap_hostent_cpy(host_copy, host); } diff --git a/test/internet/test-dns-setserver-in-callback-of-resolve4.js b/test/internet/test-dns-setserver-in-callback-of-resolve4.js index c83bf39842969c..c63e6072596bb5 100644 --- a/test/internet/test-dns-setserver-in-callback-of-resolve4.js +++ b/test/internet/test-dns-setserver-in-callback-of-resolve4.js @@ -1,17 +1,13 @@ 'use strict'; +// Refs: https://github.com/nodejs/node/pull/13050 +// We don't care about `err` in the callback function of `dns.resolve4` here. +// We just want to test whether `dns.setServers` here in the callback of +// `resolve4` will crash or not. If no crashing here, the test is succeeded. + const common = require('../common'); const dns = require('dns'); dns.resolve4('google.com', common.mustCall(function(/* err, nameServers */) { - // do not care about `err` and `nameServers`, - // both failed and succeeded would be OK. - // - // just to test crash - dns.setServers([ '8.8.8.8' ]); - - // the test case shouldn't crash here - // see https://github.com/nodejs/node/pull/13050 and - // https://github.com/nodejs/node/issues/894 })); From 00c2c110a1804ceed6c35a69f708e8c57e924a36 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Thu, 18 May 2017 11:05:25 +0800 Subject: [PATCH 5/6] update after review again (this commit will be rebased) --- src/cares_wrap.cc | 56 ++++++++----------- ...t-dns-setserver-in-callback-of-resolve4.js | 7 +-- 2 files changed, 25 insertions(+), 38 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 557eff0d0cbeac..c561dd8d1108e0 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -69,8 +69,6 @@ using v8::Value; namespace { -#define ARES_HOSTENT_COPY_ERROR -1000 - inline const char* ToErrorCodeString(int status) { switch (status) { #define V(code) case ARES_##code: return #code; @@ -98,7 +96,6 @@ inline const char* ToErrorCodeString(int status) { V(EREFUSED) V(ESERVFAIL) V(ETIMEOUT) - V(HOSTENT_COPY_ERROR) #undef V } @@ -303,7 +300,7 @@ Local HostentToNames(Environment* env, struct hostent* host) { void safe_free_hostent(struct hostent* host) { int idx; - if (host->h_addr_list) { + if (host->h_addr_list != nullptr) { idx = 0; while (host->h_addr_list[idx]) { free(host->h_addr_list[idx++]); @@ -312,7 +309,7 @@ void safe_free_hostent(struct hostent* host) { host->h_addr_list = 0; } - if (host->h_aliases) { + if (host->h_aliases != nullptr) { idx = 0; while (host->h_aliases[idx]) { free(host->h_aliases[idx++]); @@ -321,14 +318,14 @@ void safe_free_hostent(struct hostent* host) { host->h_aliases = 0; } - if (host->h_name) { + if (host->h_name != nullptr) { free(host->h_name); } host->h_addrtype = host->h_length = 0; } -bool cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { +void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { dest->h_addr_list = nullptr; dest->h_addrtype = 0; dest->h_aliases = nullptr; @@ -349,32 +346,28 @@ bool cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { } dest->h_aliases = node::Malloc(alias_count + 1); - memset(dest->h_aliases, 0, sizeof(char*) * (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); + dest->h_aliases[i] = node::Malloc(cur_alias_length + 1); memcpy(dest->h_aliases[i], src->h_aliases[i], cur_alias_length + 1); } /* copy `h_addr_list` */ - unsigned int list_count; + 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); - memset(dest->h_addr_list, 0, sizeof(char*) * (list_count + 1)); - for (unsigned int i = 0; i < list_count; i++) { - dest->h_addr_list[i] = node::Malloc(src->h_length); + 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); } /* work after work */ dest->h_length = src->h_length; dest->h_addrtype = src->h_addrtype; - - return true; } class QueryWrap; @@ -382,7 +375,10 @@ struct CaresAsyncData { QueryWrap* wrap; int status; bool is_host; - void* buf; + union { + hostent* host; + unsigned char* buf; + } data; int len; uv_async_t async_handle; @@ -435,11 +431,11 @@ class QueryWrap : public AsyncWrap { if (status != ARES_SUCCESS) { wrap->ParseError(status); } else if (!data->is_host) { - unsigned char* buf = reinterpret_cast(data->buf); + unsigned char* buf = data->data.buf; wrap->Parse(buf, data->len); free(buf); } else { - hostent* host = static_cast(data->buf); + hostent* host = data->data.host; wrap->Parse(host); safe_free_hostent(host); free(host); @@ -449,7 +445,7 @@ class QueryWrap : public AsyncWrap { } static void Callback(void *arg, int status, int timeouts, - unsigned char* answer_buf, int answer_len) { + unsigned char* answer_buf, int answer_len) { QueryWrap* wrap = static_cast(arg); unsigned char* buf_copy = nullptr; @@ -458,11 +454,11 @@ class QueryWrap : public AsyncWrap { memcpy(buf_copy, answer_buf, answer_len); } - struct CaresAsyncData* data = new struct CaresAsyncData(); + CaresAsyncData* data = new CaresAsyncData(); data->status = status; data->wrap = wrap; data->is_host = false; - data->buf = buf_copy; + data->data.buf = buf_copy; data->len = answer_len; uv_async_t* async_handle = &data->async_handle; @@ -473,26 +469,18 @@ class QueryWrap : public AsyncWrap { } static void Callback(void *arg, int status, int timeouts, - struct hostent* host) { + struct hostent* host) { QueryWrap* wrap = static_cast(arg); struct hostent* host_copy = nullptr; - bool copy_ret = false; - if (status == ARES_SUCCESS) { host_copy = node::Malloc(1); - copy_ret = cares_wrap_hostent_cpy(host_copy, host); + cares_wrap_hostent_cpy(host_copy, host); } - struct CaresAsyncData* data = new struct CaresAsyncData(); - if (copy_ret) { - data->status = status; - data->buf = host_copy; - } else { - data->status = ARES_HOSTENT_COPY_ERROR; - data->buf = nullptr; - free(host_copy); - } + CaresAsyncData* data = new CaresAsyncData(); + data->status = status; + data->data.host = host_copy; data->wrap = wrap; data->is_host = true; diff --git a/test/internet/test-dns-setserver-in-callback-of-resolve4.js b/test/internet/test-dns-setserver-in-callback-of-resolve4.js index c63e6072596bb5..9ee853b5e4080b 100644 --- a/test/internet/test-dns-setserver-in-callback-of-resolve4.js +++ b/test/internet/test-dns-setserver-in-callback-of-resolve4.js @@ -1,9 +1,8 @@ 'use strict'; -// Refs: https://github.com/nodejs/node/pull/13050 -// We don't care about `err` in the callback function of `dns.resolve4` here. -// We just want to test whether `dns.setServers` here in the callback of -// `resolve4` will crash or not. If no crashing here, the test is succeeded. +// 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'); From 7390ee0d05f3f20e6461e412fd21d8ff8c906969 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Thu, 18 May 2017 22:31:56 +0800 Subject: [PATCH 6/6] add the forgotten ending nullptr --- src/cares_wrap.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index c561dd8d1108e0..f0b81c8c97e43f 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -351,6 +351,7 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { 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; @@ -364,6 +365,7 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { 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;