Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 026f760

Browse filesBrowse 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 <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de>
1 parent 619cbc4 commit 026f760
Copy full SHA for 026f760

File tree

Expand file treeCollapse file tree

4 files changed

+94
-21
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+94
-21
lines changed
Open diff view settings
Collapse file

‎lib/dns.js‎

Copy file name to clipboardExpand all lines: lib/dns.js
+34-16Lines 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();
Collapse file

‎src/cares_wrap.cc‎

Copy file name to clipboardExpand all lines: src/cares_wrap.cc
+26-5Lines 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

Collapse file

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

Copy file name to clipboardExpand all lines: test/internet/test-dns-setserver-in-callback-of-resolve4.js
+3Lines 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());
Collapse file

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

Copy file name to clipboard
+31Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.