From c3bb82a3b9cc929545a8c6221cfda90b55a135bf Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 15 Nov 2017 15:09:33 +0800 Subject: [PATCH 1/5] dns: fix crash while setting server during query Fix this issue follow these two points: 1. Keep track of how many queries are currently open. If `setServers()` is called while there are open queries, error out. 2. For `Resolver` instances, use option 1. For dns.setServers(), just create a fresh new default channel every time it is called, and then set its servers list. Fixes: https://github.com/nodejs/node/issues/14734 --- lib/dns.js | 50 +++++++++++++------ src/cares_wrap.cc | 31 ++++++++++-- ...t-dns-setserver-in-callback-of-resolve4.js | 11 +++- test/parallel/test-dns.js | 22 ++++++++ 4 files changed, 91 insertions(+), 23 deletions(-) diff --git a/lib/dns.js b/lib/dns.js index b8c98e712540c5..92b55f03595f45 100644 --- a/lib/dns.js +++ b/lib/dns.js @@ -380,28 +380,44 @@ function setServers(servers) { } } -const defaultResolver = new Resolver(); +let defaultResolver = new Resolver(); + +const resolverKeys = [ + 'getServers', + 'resolve', + 'resolveAny', + 'resolve4', + 'resolve6', + 'resolveCname', + 'resolveMx', + 'resolveNs', + 'resolveTxt', + 'resolveSrv', + 'resolvePtr', + 'resolveNaptr', + 'resolveSoa', + 'reverse' +]; + +function setExportsFunctions() { + resolverKeys.forEach((key) => { + module.exports[key] = defaultResolver[key].bind(defaultResolver); + }); +} + +function defaultResolverSetServers(servers) { + const resolver = new Resolver(); + resolver.setServers(servers); + defaultResolver = resolver; + setExportsFunctions(); +} module.exports = { lookup, lookupService, Resolver, - getServers: defaultResolver.getServers.bind(defaultResolver), - setServers: defaultResolver.setServers.bind(defaultResolver), - resolve: defaultResolver.resolve.bind(defaultResolver), - resolveAny: defaultResolver.resolveAny.bind(defaultResolver), - resolve4: defaultResolver.resolve4.bind(defaultResolver), - resolve6: defaultResolver.resolve6.bind(defaultResolver), - resolveCname: defaultResolver.resolveCname.bind(defaultResolver), - resolveMx: defaultResolver.resolveMx.bind(defaultResolver), - resolveNs: defaultResolver.resolveNs.bind(defaultResolver), - resolveTxt: defaultResolver.resolveTxt.bind(defaultResolver), - resolveSrv: defaultResolver.resolveSrv.bind(defaultResolver), - resolvePtr: defaultResolver.resolvePtr.bind(defaultResolver), - resolveNaptr: defaultResolver.resolveNaptr.bind(defaultResolver), - resolveSoa: defaultResolver.resolveSoa.bind(defaultResolver), - reverse: defaultResolver.reverse.bind(defaultResolver), + setServers: defaultResolverSetServers, // uv_getaddrinfo flags ADDRCONFIG: cares.AI_ADDRCONFIG, @@ -433,3 +449,5 @@ module.exports = { ADDRGETNETWORKPARAMS: 'EADDRGETNETWORKPARAMS', CANCELLED: 'ECANCELLED' }; + +setExportsFunctions(); diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 1382d60ee351f5..9bf67c8895b4ab 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -83,6 +83,7 @@ inline uint32_t cares_get_32bit(const unsigned char* p) { const int ns_t_cname_or_a = -1; +#define DNS_ESETSRVPENDING -1000 inline const char* ToErrorCodeString(int status) { switch (status) { #define V(code) case ARES_##code: return #code; @@ -150,6 +151,8 @@ class ChannelWrap : public AsyncWrap { void EnsureServers(); void CleanupTimer(); + void ModifyActivityQueryCount(int count); + 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_; } @@ -158,6 +161,7 @@ class ChannelWrap : public AsyncWrap { inline void set_is_servers_default(bool is_default) { is_servers_default_ = is_default; } + inline int active_query_count() { return active_query_count_; } inline node_ares_task_list* task_list() { return &task_list_; } size_t self_size() const override { return sizeof(this); } @@ -170,6 +174,7 @@ class ChannelWrap : public AsyncWrap { bool query_last_ok_; bool is_servers_default_; bool library_inited_; + int active_query_count_; node_ares_task_list task_list_; }; @@ -180,7 +185,8 @@ ChannelWrap::ChannelWrap(Environment* env, channel_(nullptr), query_last_ok_(true), is_servers_default_(true), - library_inited_(false) { + library_inited_(false), + active_query_count_(0) { MakeWeak(this); Setup(); @@ -545,6 +551,11 @@ void ChannelWrap::CleanupTimer() { timer_handle_ = nullptr; } +void ChannelWrap::ModifyActivityQueryCount(int count) { + active_query_count_ += count; + if (active_query_count_ < 0) active_query_count_ = 0; +} + /** * This function is to check whether current servers are fallback servers @@ -688,6 +699,7 @@ class QueryWrap : public AsyncWrap { CaresAsyncCb)); wrap->channel_->set_query_last_ok(status != ARES_ECONNREFUSED); + wrap->channel_->ModifyActivityQueryCount(-1); async_handle->data = data; uv_async_send(async_handle); } @@ -1808,9 +1820,12 @@ static void Query(const FunctionCallbackInfo& args) { Wrap* wrap = new Wrap(channel, req_wrap_obj); node::Utf8Value name(env->isolate(), string); + channel->ModifyActivityQueryCount(1); int err = wrap->Send(*name); - if (err) + if (err) { + channel->ModifyActivityQueryCount(-1); delete wrap; + } args.GetReturnValue().Set(err); } @@ -2087,6 +2102,10 @@ void SetServers(const FunctionCallbackInfo& args) { ChannelWrap* channel; ASSIGN_OR_RETURN_UNWRAP(&channel, args.Holder()); + if (channel->active_query_count()) { + return args.GetReturnValue().Set(DNS_ESETSRVPENDING); + } + CHECK(args[0]->IsArray()); Local arr = Local::Cast(args[0]); @@ -2167,11 +2186,13 @@ void Cancel(const FunctionCallbackInfo& args) { ares_cancel(channel->cares_channel()); } - +const char EMSG_ESETSRVPENDING[] = "There are pending queries."; void StrError(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - const char* errmsg = ares_strerror(args[0]->Int32Value(env->context()) - .FromJust()); + int code = args[0]->Int32Value(env->context()).FromJust(); + const char* errmsg = (code == DNS_ESETSRVPENDING) ? + EMSG_ESETSRVPENDING : + ares_strerror(code); args.GetReturnValue().Set(OneByteString(env->isolate(), errmsg)); } 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 2ef7e91f701efd..4505dcd8305b29 100644 --- a/test/internet/test-dns-setserver-in-callback-of-resolve4.js +++ b/test/internet/test-dns-setserver-in-callback-of-resolve4.js @@ -9,7 +9,14 @@ const { addresses } = require('../common/internet'); const dns = require('dns'); dns.resolve4( - addresses.INET4_HOST, + addresses.INET_HOST, common.mustCall(function(/* err, nameServers */) { - dns.setServers([ addresses.DNS4_SERVER ]); + dns.setServers([ '8.8.8.8' ]); + })); + +// Test https://github.com/nodejs/node/issues/14734 +dns.resolve4( + addresses.INET_HOST, + common.mustCall(function() { + // DO NOTHING })); diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js index 08a26ab2184dd2..7a25153171ce9a 100644 --- a/test/parallel/test-dns.js +++ b/test/parallel/test-dns.js @@ -244,3 +244,25 @@ assert.throws(() => { code: 'ERR_INVALID_CALLBACK', type: TypeError })); + +{ + // Fix https://github.com/nodejs/node/issues/14734 + const resolver = new dns.Resolver(); + resolver.resolve('localhost', function(err/*, ret*/) { + // nothing + }); + + assert.throws(() => { + resolver.setServers(goog); + }, common.expectsError({ + code: 'ERR_DNS_SET_SERVERS_FAILED', + message: /^c-ares failed to set servers: "There are pending queries\." \[.+\]$/g + })); + + dns.resolve('localhost', function(err/*, ret*/) { + // nothing + }); + + // should not throw + dns.setServers(goog); +} From 3af0573eba1b7b1468cc195f8ecf643c293d32b0 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 15 Nov 2017 15:12:42 +0800 Subject: [PATCH 2/5] fix a typo --- test/internet/test-dns-setserver-in-callback-of-resolve4.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 4505dcd8305b29..39d1b0b2a99dbb 100644 --- a/test/internet/test-dns-setserver-in-callback-of-resolve4.js +++ b/test/internet/test-dns-setserver-in-callback-of-resolve4.js @@ -9,14 +9,14 @@ const { addresses } = require('../common/internet'); const dns = require('dns'); dns.resolve4( - addresses.INET_HOST, + addresses.INET4_HOST, common.mustCall(function(/* err, nameServers */) { - dns.setServers([ '8.8.8.8' ]); + dns.setServers([ addresses.DNS4_SERVER ]); })); // Test https://github.com/nodejs/node/issues/14734 dns.resolve4( - addresses.INET_HOST, + addresses.INET4_HOST, common.mustCall(function() { // DO NOTHING })); From f7db3d66eb38aacdee4a67fa0350ded1c60e5230 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Tue, 5 Dec 2017 10:13:07 +0800 Subject: [PATCH 3/5] update based on @addaleax' advise --- .../test-dns-setserver-when-querying.js | 33 +++++++++++++++++++ test/parallel/test-dns.js | 22 ------------- 2 files changed, 33 insertions(+), 22 deletions(-) create mode 100644 test/parallel/test-dns-setserver-when-querying.js diff --git a/test/parallel/test-dns-setserver-when-querying.js b/test/parallel/test-dns-setserver-when-querying.js new file mode 100644 index 00000000000000..2b7bb15cb14430 --- /dev/null +++ b/test/parallel/test-dns-setserver-when-querying.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +const dns = require('dns'); + +const goog = [ + '8.8.8.8', + '8.8.4.4', +]; + +{ + // Fix https://github.com/nodejs/node/issues/14734 + const resolver = new dns.Resolver(); + resolver.resolve('localhost', function(err/*, ret*/) { + // nothing + }); + + assert.throws(() => { + resolver.setServers(goog); + }, common.expectsError({ + code: 'ERR_DNS_SET_SERVERS_FAILED', + message: /^c-ares failed to set servers: "There are pending queries\." \[.+\]$/g + })); + + dns.resolve('localhost', function(err/*, ret*/) { + // nothing + }); + + // should not throw + dns.setServers(goog); +} diff --git a/test/parallel/test-dns.js b/test/parallel/test-dns.js index 7a25153171ce9a..08a26ab2184dd2 100644 --- a/test/parallel/test-dns.js +++ b/test/parallel/test-dns.js @@ -244,25 +244,3 @@ assert.throws(() => { code: 'ERR_INVALID_CALLBACK', type: TypeError })); - -{ - // Fix https://github.com/nodejs/node/issues/14734 - const resolver = new dns.Resolver(); - resolver.resolve('localhost', function(err/*, ret*/) { - // nothing - }); - - assert.throws(() => { - resolver.setServers(goog); - }, common.expectsError({ - code: 'ERR_DNS_SET_SERVERS_FAILED', - message: /^c-ares failed to set servers: "There are pending queries\." \[.+\]$/g - })); - - dns.resolve('localhost', function(err/*, ret*/) { - // nothing - }); - - // should not throw - dns.setServers(goog); -} From caab532b6efc9b8aa6c8dd931e95d3db61d7fa9b Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 6 Dec 2017 09:40:27 +0800 Subject: [PATCH 4/5] update after @BridgeAR's reviewing --- ...t-dns-setserver-in-callback-of-resolve4.js | 6 +--- .../test-dns-setserver-when-querying.js | 35 +++++++++---------- 2 files changed, 18 insertions(+), 23 deletions(-) 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 39d1b0b2a99dbb..58b3327efe9475 100644 --- a/test/internet/test-dns-setserver-in-callback-of-resolve4.js +++ b/test/internet/test-dns-setserver-in-callback-of-resolve4.js @@ -15,8 +15,4 @@ dns.resolve4( })); // Test https://github.com/nodejs/node/issues/14734 -dns.resolve4( - addresses.INET4_HOST, - common.mustCall(function() { - // DO NOTHING - })); +dns.resolve4(addresses.INET4_HOST, common.mustCall()); diff --git a/test/parallel/test-dns-setserver-when-querying.js b/test/parallel/test-dns-setserver-when-querying.js index 2b7bb15cb14430..1dda1986b98f05 100644 --- a/test/parallel/test-dns-setserver-when-querying.js +++ b/test/parallel/test-dns-setserver-when-querying.js @@ -12,22 +12,21 @@ const goog = [ { // Fix https://github.com/nodejs/node/issues/14734 - const resolver = new dns.Resolver(); - resolver.resolve('localhost', function(err/*, ret*/) { - // nothing - }); - - assert.throws(() => { - resolver.setServers(goog); - }, common.expectsError({ - code: 'ERR_DNS_SET_SERVERS_FAILED', - message: /^c-ares failed to set servers: "There are pending queries\." \[.+\]$/g - })); - - dns.resolve('localhost', function(err/*, ret*/) { - // nothing - }); - - // should not throw - dns.setServers(goog); + + { + const resolver = new dns.Resolver(); + resolver.resolve('localhost', common.mustCall()); + + common.expectsError(resolver.setServers.bind(resolver, goog), { + code: 'ERR_DNS_SET_SERVERS_FAILED', + message: /^c-ares failed to set servers: "There are pending queries\." \[.+\]$/g + }); + } + + { + dns.resolve('localhost', common.mustCall()); + + // should not throw + dns.setServers(goog); + } } From ac04c852a0b2fa2ec63e2578f02590b7c20fe0ee Mon Sep 17 00:00:00 2001 From: XadillaX Date: Wed, 6 Dec 2017 09:55:25 +0800 Subject: [PATCH 5/5] fix a lint nit --- test/parallel/test-dns-setserver-when-querying.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/parallel/test-dns-setserver-when-querying.js b/test/parallel/test-dns-setserver-when-querying.js index 1dda1986b98f05..205a5b6c8585d0 100644 --- a/test/parallel/test-dns-setserver-when-querying.js +++ b/test/parallel/test-dns-setserver-when-querying.js @@ -1,7 +1,6 @@ 'use strict'; const common = require('../common'); -const assert = require('assert'); const dns = require('dns');