Skip to content

Commit 026f760

Browse files
XadillaXMylesBorins
XadillaX
authored and
MylesBorins
committed
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. PR-URL: #14891 Fixes: #14734 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
1 parent 619cbc4 commit 026f760

File tree

4 files changed

+94
-21
lines changed

4 files changed

+94
-21
lines changed

‎lib/dns.js‎

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -380,28 +380,44 @@ function setServers(servers) {
380380
}
381381
}
382382

383-
const defaultResolver = new Resolver();
383+
let defaultResolver = new Resolver();
384+
385+
const resolverKeys = [
386+
'getServers',
387+
'resolve',
388+
'resolveAny',
389+
'resolve4',
390+
'resolve6',
391+
'resolveCname',
392+
'resolveMx',
393+
'resolveNs',
394+
'resolveTxt',
395+
'resolveSrv',
396+
'resolvePtr',
397+
'resolveNaptr',
398+
'resolveSoa',
399+
'reverse'
400+
];
401+
402+
function setExportsFunctions() {
403+
resolverKeys.forEach((key) => {
404+
module.exports[key] = defaultResolver[key].bind(defaultResolver);
405+
});
406+
}
407+
408+
function defaultResolverSetServers(servers) {
409+
const resolver = new Resolver();
410+
resolver.setServers(servers);
411+
defaultResolver = resolver;
412+
setExportsFunctions();
413+
}
384414

385415
module.exports = {
386416
lookup,
387417
lookupService,
388418

389419
Resolver,
390-
getServers: defaultResolver.getServers.bind(defaultResolver),
391-
setServers: defaultResolver.setServers.bind(defaultResolver),
392-
resolve: defaultResolver.resolve.bind(defaultResolver),
393-
resolveAny: defaultResolver.resolveAny.bind(defaultResolver),
394-
resolve4: defaultResolver.resolve4.bind(defaultResolver),
395-
resolve6: defaultResolver.resolve6.bind(defaultResolver),
396-
resolveCname: defaultResolver.resolveCname.bind(defaultResolver),
397-
resolveMx: defaultResolver.resolveMx.bind(defaultResolver),
398-
resolveNs: defaultResolver.resolveNs.bind(defaultResolver),
399-
resolveTxt: defaultResolver.resolveTxt.bind(defaultResolver),
400-
resolveSrv: defaultResolver.resolveSrv.bind(defaultResolver),
401-
resolvePtr: defaultResolver.resolvePtr.bind(defaultResolver),
402-
resolveNaptr: defaultResolver.resolveNaptr.bind(defaultResolver),
403-
resolveSoa: defaultResolver.resolveSoa.bind(defaultResolver),
404-
reverse: defaultResolver.reverse.bind(defaultResolver),
420+
setServers: defaultResolverSetServers,
405421

406422
// uv_getaddrinfo flags
407423
ADDRCONFIG: cares.AI_ADDRCONFIG,
@@ -433,3 +449,5 @@ module.exports = {
433449
ADDRGETNETWORKPARAMS: 'EADDRGETNETWORKPARAMS',
434450
CANCELLED: 'ECANCELLED'
435451
};
452+
453+
setExportsFunctions();

‎src/cares_wrap.cc‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ inline uint32_t cares_get_32bit(const unsigned char* p) {
8383

8484
const int ns_t_cname_or_a = -1;
8585

86+
#define DNS_ESETSRVPENDING -1000
8687
inline const char* ToErrorCodeString(int status) {
8788
switch (status) {
8889
#define V(code) case ARES_##code: return #code;
@@ -150,6 +151,8 @@ class ChannelWrap : public AsyncWrap {
150151
void EnsureServers();
151152
void CleanupTimer();
152153

154+
void ModifyActivityQueryCount(int count);
155+
153156
inline uv_timer_t* timer_handle() { return timer_handle_; }
154157
inline ares_channel cares_channel() { return channel_; }
155158
inline bool query_last_ok() const { return query_last_ok_; }
@@ -158,6 +161,7 @@ class ChannelWrap : public AsyncWrap {
158161
inline void set_is_servers_default(bool is_default) {
159162
is_servers_default_ = is_default;
160163
}
164+
inline int active_query_count() { return active_query_count_; }
161165
inline node_ares_task_list* task_list() { return &task_list_; }
162166

163167
size_t self_size() const override { return sizeof(*this); }
@@ -170,6 +174,7 @@ class ChannelWrap : public AsyncWrap {
170174
bool query_last_ok_;
171175
bool is_servers_default_;
172176
bool library_inited_;
177+
int active_query_count_;
173178
node_ares_task_list task_list_;
174179
};
175180

@@ -180,7 +185,8 @@ ChannelWrap::ChannelWrap(Environment* env,
180185
channel_(nullptr),
181186
query_last_ok_(true),
182187
is_servers_default_(true),
183-
library_inited_(false) {
188+
library_inited_(false),
189+
active_query_count_(0) {
184190
MakeWeak<ChannelWrap>(this);
185191

186192
Setup();
@@ -545,6 +551,11 @@ void ChannelWrap::CleanupTimer() {
545551
timer_handle_ = nullptr;
546552
}
547553

554+
void ChannelWrap::ModifyActivityQueryCount(int count) {
555+
active_query_count_ += count;
556+
if (active_query_count_ < 0) active_query_count_ = 0;
557+
}
558+
548559

549560
/**
550561
* This function is to check whether current servers are fallback servers
@@ -682,6 +693,7 @@ class QueryWrap : public AsyncWrap {
682693
CaresAsyncCb));
683694

684695
wrap->channel_->set_query_last_ok(status != ARES_ECONNREFUSED);
696+
wrap->channel_->ModifyActivityQueryCount(-1);
685697
async_handle->data = data;
686698
uv_async_send(async_handle);
687699
}
@@ -1802,9 +1814,12 @@ static void Query(const FunctionCallbackInfo<Value>& args) {
18021814
Wrap* wrap = new Wrap(channel, req_wrap_obj);
18031815

18041816
node::Utf8Value name(env->isolate(), string);
1817+
channel->ModifyActivityQueryCount(1);
18051818
int err = wrap->Send(*name);
1806-
if (err)
1819+
if (err) {
1820+
channel->ModifyActivityQueryCount(-1);
18071821
delete wrap;
1822+
}
18081823

18091824
args.GetReturnValue().Set(err);
18101825
}
@@ -2081,6 +2096,10 @@ void SetServers(const FunctionCallbackInfo<Value>& args) {
20812096
ChannelWrap* channel;
20822097
ASSIGN_OR_RETURN_UNWRAP(&channel, args.Holder());
20832098

2099+
if (channel->active_query_count()) {
2100+
return args.GetReturnValue().Set(DNS_ESETSRVPENDING);
2101+
}
2102+
20842103
CHECK(args[0]->IsArray());
20852104

20862105
Local<Array> arr = Local<Array>::Cast(args[0]);
@@ -2161,11 +2180,13 @@ void Cancel(const FunctionCallbackInfo<Value>& args) {
21612180
ares_cancel(channel->cares_channel());
21622181
}
21632182

2164-
2183+
const char EMSG_ESETSRVPENDING[] = "There are pending queries.";
21652184
void StrError(const FunctionCallbackInfo<Value>& args) {
21662185
Environment* env = Environment::GetCurrent(args);
2167-
const char* errmsg = ares_strerror(args[0]->Int32Value(env->context())
2168-
.FromJust());
2186+
int code = args[0]->Int32Value(env->context()).FromJust();
2187+
const char* errmsg = (code == DNS_ESETSRVPENDING) ?
2188+
EMSG_ESETSRVPENDING :
2189+
ares_strerror(code);
21692190
args.GetReturnValue().Set(OneByteString(env->isolate(), errmsg));
21702191
}
21712192

‎test/internet/test-dns-setserver-in-callback-of-resolve4.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,3 +13,6 @@ dns.resolve4(
1313
common.mustCall(function(/* err, nameServers */) {
1414
dns.setServers([ addresses.DNS4_SERVER ]);
1515
}));
16+
17+
// Test https://github.com/nodejs/node/issues/14734
18+
dns.resolve4(addresses.INET4_HOST, common.mustCall());

‎test/parallel/test-dns-setserver-when-querying.js‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
const dns = require('dns');
6+
7+
const goog = [
8+
'8.8.8.8',
9+
'8.8.4.4',
10+
];
11+
12+
{
13+
// Fix https://github.com/nodejs/node/issues/14734
14+
15+
{
16+
const resolver = new dns.Resolver();
17+
resolver.resolve('localhost', common.mustCall());
18+
19+
common.expectsError(resolver.setServers.bind(resolver, goog), {
20+
code: 'ERR_DNS_SET_SERVERS_FAILED',
21+
message: /^c-ares failed to set servers: "There are pending queries\." \[.+\]$/g
22+
});
23+
}
24+
25+
{
26+
dns.resolve('localhost', common.mustCall());
27+
28+
// should not throw
29+
dns.setServers(goog);
30+
}
31+
}

0 commit comments

Comments
 (0)