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: #14734
PR-URL: #14891
Backport-PR-URL: #17778
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>

Author: XadillaX

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();

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<ChannelWrap>(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<Value>& 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<Value>& 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<Array> arr = Local<Array>::Cast(args[0]);
@@ -2165,11 +2184,13 @@ void Cancel(const FunctionCallbackInfo<Value>& args) {
   ares_cancel(channel->cares_channel());
 }
 
-
+const char EMSG_ESETSRVPENDING[] = "There are pending queries.";
 void StrError(const FunctionCallbackInfo<Value>& 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));
 }
 

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());

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);
+  }
+}