From 35887306f10a8272b1115e1b14e20ff0f985fe01 Mon Sep 17 00:00:00 2001 From: XadillaX Date: Thu, 25 Jan 2018 21:00:25 +0800 Subject: [PATCH] dns: fix crash while setting server during query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 PR-URL: https://github.com/nodejs/node/pull/14891 Backport-PR-URL: https://github.com/nodejs/node/pull/17778 Reviewed-By: Anna Henningsen Reviewed-By: Tobias Nießen --- lib/dns.js | 50 +++++++++++++------ src/cares_wrap.cc | 31 ++++++++++-- ...t-dns-setserver-in-callback-of-resolve4.js | 3 ++ .../test-dns-setserver-when-querying.js | 30 +++++++++++ 4 files changed, 93 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-dns-setserver-when-querying.js diff --git a/lib/dns.js b/lib/dns.js index 09076a6b4bbd37..4c7fbe147497f4 100644 --- a/lib/dns.js +++ b/lib/dns.js @@ -374,28 +374,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, @@ -427,3 +443,5 @@ module.exports = { ADDRGETNETWORKPARAMS: 'EADDRGETNETWORKPARAMS', CANCELLED: 'ECANCELLED' }; + +setExportsFunctions(); diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 24f2f395c47a1e..ee9950597965e7 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -81,6 +81,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; @@ -148,6 +149,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_; } @@ -156,6 +159,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); } @@ -168,6 +172,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_; }; @@ -178,7 +183,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(); @@ -543,6 +549,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 @@ -686,6 +697,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); } @@ -1806,9 +1818,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); } @@ -2085,6 +2100,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]); @@ -2165,11 +2184,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 9ee853b5e4080b..222ac4dcc8ee31 100644 --- a/test/internet/test-dns-setserver-in-callback-of-resolve4.js +++ b/test/internet/test-dns-setserver-in-callback-of-resolve4.js @@ -10,3 +10,6 @@ const dns = require('dns'); dns.resolve4('google.com', common.mustCall(function(/* err, nameServers */) { dns.setServers([ '8.8.8.8' ]); })); + +// Test https://github.com/nodejs/node/issues/14734 +dns.resolve4('google.com', common.mustCall()); 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..142f03f40b61cb --- /dev/null +++ b/test/parallel/test-dns-setserver-when-querying.js @@ -0,0 +1,30 @@ +'use strict'; + +const common = require('../common'); + +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', common.mustCall()); + + common.expectsError(resolver.setServers.bind(resolver, goog), { + message: /^c-ares failed to set servers: "There are pending queries\." \[.+\]$/g + }); + } + + { + dns.resolve('localhost', common.mustCall()); + + // should not throw + dns.setServers(goog); + } +}