dns: fix crash while setting server during query #14891

XadillaX · 2017-08-17T09:28:45Z

Fix this issue follow these two points:

Keep track of how many queries are currently open. If setServers()
is called while there are open queries, error out.
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

Checklist

make -j4 test passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

dns, cares

XadillaX · 2017-08-17T09:36:01Z

Occasional crash in this PR:

/Users/xadillax/Workspace/cpp/master-node/out/Release/node[20134]: ../src/cares_wrap.cc:351:void node::cares_wrap::(anonymous namespace)::ares_sockstate_cb(void *, ares_socket_t, int, int): Assertion `task && "When an ares socket is closed we should have a handle for it"' failed.
 1: node::Abort() [/Users/xadillax/Workspace/cpp/master-node/./node]
 2: node::MakeCallback(v8::Isolate*, v8::Local<v8::Object>, char const*, int, v8::Local<v8::Value>*, node::async_context) [/Users/xadillax/Workspace/cpp/master-node/./node]
 3: node::cares_wrap::(anonymous namespace)::ares_sockstate_cb(void*, int, int, int) [/Users/xadillax/Workspace/cpp/master-node/./node]
 4: ares__close_sockets [/Users/xadillax/Workspace/cpp/master-node/./node]
 5: ares__destroy_servers_state [/Users/xadillax/Workspace/cpp/master-node/./node]
 6: ares_destroy [/Users/xadillax/Workspace/cpp/master-node/./node]
 7: node::cares_wrap::(anonymous namespace)::ExecuteDestroyChannel(uv_work_s*) [/Users/xadillax/Workspace/cpp/master-node/./node]
 8: worker [/Users/xadillax/Workspace/cpp/master-node/./node]
 9: _pthread_body [/usr/lib/system/libsystem_pthread.dylib]
10: _pthread_body [/usr/lib/system/libsystem_pthread.dylib]
11: thread_start [/usr/lib/system/libsystem_pthread.dylib]
[1]    20134 abort      ./node test/internet/test-dns-setserver-in-callback-of-resolve4.js

@addaleax Do you have any idea with this crash?

tniessen · 2017-08-17T16:15:50Z

I have got some questions before doing a full review, I hope you don't mind:

Is it a reasonable assumption that there are no outstanding queries (referring to this check)? To me, this sounds like something that could be fixed upstream, but I could be wrong. However, if it is a reasonable assumption, we could consider to throw an error instead.
What does this imply for Resolvers? We effectively hot-swap the channel of a resolver? Are we sure this won't cause problems?
If we do, we need to patch Cancel() to cancel requests in all channels which were created for that Resolver, right? Assuming they have not been destroyed yet.

addaleax · 2017-08-17T17:06:54Z

Heads up: This is going to conflict with #14826

addaleax · 2017-08-17T17:08:12Z

I think an easier fix would be to wait with actually carrying out setServers() until all current queries have been handled (and queueing up queries until that point)?

tniessen · 2017-08-17T18:25:17Z

I think an easier fix would be to wait with actually carrying out setServers() until all current queries have been handled (and queueing up queries until that point)?

Easier, yes, and probably less error-prone, but that can cause unexpected delays, especially when using the default resolver. I am still in favor of permitting this behavior upstream if there is no good reason to disallow it. And if there is a good reason to disallow it, we should probably do the same instead of working around the actual problem like this.

XadillaX · 2017-08-18T03:06:24Z

@tniessen

Is it a reasonable assumption that there are no outstanding queries (referring to this check)? To me, this sounds like something that could be fixed upstream, but I could be wrong. However, if it is a reasonable assumption, we could consider to throw an error instead.

C-ARES is not designed for libuv. If we use it in multi-thread, some locks are needed.

Imagine this situation in Node.js:

dns.resolve4('google.com', common.mustCall(function(/* err, nameServers */) {
  dns.setServers([ '8.8.8.8' ]);
}));

dns.resolve4('google.com', common.mustCall(function() {
  // do nothing...
}));

If the first resolve4() done, and second one is querying in another thread and not finished yet. Now the main event loop do the dns.setServer() - the assert will be trigged and Node.js crashes.

XadillaX · 2017-08-18T03:08:16Z

I think an easier fix would be to wait with actually carrying out setServers() until all current queries have been handled (and queueing up queries until that point)?

Doesn't it block the event loop?

tniessen · 2017-08-18T08:58:58Z

Is it a reasonable assumption that there are no outstanding queries (referring to this check)? To me, this sounds like something that could be fixed upstream, but I could be wrong. However, if it is a reasonable assumption, we could consider to throw an error instead.

C-ARES is not designed for libuv. If we use it in multi-thread, some locks are needed.

If the first resolve4() done, and second one is querying in another thread and not finished yet.

Sorry, I don't really get your point here. How is this related to threads? (c-ares does not use threads internally as far as I know, but I might be wrong.)

XadillaX · 2017-08-18T09:02:54Z

@tniessen But Node.js do dns.resolveA() in another thread in libuv (via AsyncWrap) or it will be blocked. That's the problem. C-ARES thinks you only have one thread and if you're setting a DNS server that you must finished all your queries.

BridgeAR · 2017-09-02T06:50:33Z

Where do we stand here?

XadillaX · 2017-09-03T07:47:01Z

Where do we stand here?

@BridgeAR Sorry but I don't quite understand what you mean.

BridgeAR · 2017-09-03T20:07:02Z

@XadillaX you posted that this PR this has an issue, there was no proper review yet and the last update was more then two weeks before my post. Therefore my question is how this is further progressing. Did you look into the issue again?

XadillaX · 2017-09-05T03:37:15Z

@BridgeAR I've updated the code based on #14826.

/cc @addaleax

tniessen · 2017-09-08T12:28:23Z

By the way, I just noticed the documentation explicitely prohibits this behavior:

The dns.setServers() method must not be called while a DNS query is in progress.

addaleax · 2017-09-08T13:05:41Z

@XadillaX I am not sure – can you explain what the job of the added threading in this PR is? Previously, everything was single-threaded with no synchronization needed…

XadillaX · 2017-09-10T05:48:17Z

@addaleax You see this example:

dns.resolve4('google.com', common.mustCall(function(/* err, nameServers */) {
    dns.setServer([ '8.8.8.8' ]);
});

dns.resolve4('google.com', common.mustCall(function() {
  // DO NOTHING
}));

Sometime this example will crash because when we are doing the second dns.resolve4, it may execute setServer in the main eventloop. And dns.setServer will assert there's no active query in C-ARES. But the result is, the second dns.resolve4 actives the query channel and occurs the crash.

addaleax · 2017-09-10T12:56:19Z

@XadillaX Yes, I understand the problem. What I don’t understand so much is why using multi-threading is helpful to solve it.

XadillaX · 2017-09-10T13:02:12Z

@addaleax

add read write lock to setServer and resolve.
when resolve (query), create a queue work and do some lock operation.
when setServer, replace the original channel to a new one and wait for unlocking the write lock. after unlocking, delete the original channel.

If I do not use multithread, the main eventloop will be blocked by lock.

addaleax · 2017-09-10T13:18:29Z

@XadillaX I see! There is at least one problem, though: If you look up the docs for pthread_rwlock_unlock(), which is what libuv uses for its own rwlocks, you’ll find that “results are undefined if the read-write lock rwlock is not held by the calling thread”.

Also, I think this adds a lot of complexity for behaviour that is arguably not a bug, because this is explicitly forbidden by our documentation.

I could think of these alternative approaches:

Keep track of how many queries are currently open. If setServers() is called while there are open queries, error out.
Keep track of how many queries are currently open. If setServers() is called while there are open queries, store the servers structure that we create, and add a hook that calls ares_set_servers_ports() once that count drops to zero.
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.

What do you think?

XadillaX · 2017-09-11T02:48:31Z

@addaleax

This is awesome but not for the default Resolver. Maybe we can provide another API activityCount.
If we call resolve before another one is finished, and call a new resolve before the one we mentioned before is finished, the loop won't break and the new servers won't be set forever.
It's suitable for option 1, but I wonder the performance.

addaleax · 2017-09-11T09:08:01Z

@XadillaX Yes, I don’t like 2 either. For 1 and 3, I think either is fine, and I wouldn’t worry about performance too much as I can’t imagine anybody is calling .setServers() more than once a minute or so in practice (and if they are, they probably shouldn’t).

XadillaX · 2017-09-11T09:55:12Z

@addaleax Sorry I read it wrong. I read it as creating a new channel everytime resolve and setServer.

Now I agree with 1 and 3. And I'll continue working for it.

BridgeAR · 2017-09-19T21:17:48Z

Ping @XadillaX you still want to follow up on this and rework the PR, right?

BridgeAR · 2017-09-27T22:19:32Z

Ping @XadillaX

addaleax · 2017-11-20T11:50:12Z

@XadillaX Yes – I think why this works a lot of the time is that setServers([]) usually, depending on the contents of unitialized memory, results in queries failing with ENOMEM due to the miscalculation that gives malloc(-8).

I think for now you should be fine moving the added tests from this PR into a new file, don’t worry about setServers([]).

XadillaX · 2017-12-05T02:15:41Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/11879/

XadillaX · 2017-12-05T03:06:20Z

/cc @addaleax I've updated the test case.

BridgeAR · 2017-12-05T03:24:15Z

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

+  addresses.INET4_HOST,
+  common.mustCall(function() {
+    // DO NOTHING
+  }));


Tiny nit

A must call does not require a noop function.

BridgeAR · 2017-12-05T03:29:02Z

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

+  }, common.expectsError({
+    code: 'ERR_DNS_SET_SERVERS_FAILED',
+    message: /^c-ares failed to set servers: "There are pending queries\." \[.+\]$/g
+  }));


Tiny nit

You could use common.expectsError(throwingFn, errorObj) instead of assert.throws(throwingFn, common.expectsError(errorObj)).

BridgeAR · 2017-12-05T03:48:02Z

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

+
+  dns.resolve('localhost', function(err/*, ret*/) {
+    // nothing
+  });


Maybe use common.mustCall for the noop functions in this test? Or are they actually not called at all? In that case a mustNotCall could be used.

XadillaX · 2017-12-06T01:57:10Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/11905/

addaleax · 2017-12-06T03:22:53Z

Landed in b3678df … finally. ;) Thanks for sticking with this PR and pushing it through!

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>

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 <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Aug 17, 2017

XadillaX added the lts-watch-v6.x label Aug 17, 2017

jasnell requested review from addaleax and bnoordhuis August 17, 2017 14:20

XadillaX force-pushed the fix-crash-14734 branch from 351c3d7 to 6e0e0ff Compare September 5, 2017 03:36

joyeecheung mentioned this pull request Nov 21, 2017

doc: require CI status indicator in PRs #17151

Closed

1 task

BridgeAR added the wip Issues and PRs that are still a work in progress. label Nov 22, 2017

addaleax mentioned this pull request Nov 22, 2017

dns: add setDefaultResolver and getDefaultResolver #14620

Closed

4 tasks

update based on @addaleax' advise

f7db3d6

BridgeAR removed the wip Issues and PRs that are still a work in progress. label Dec 6, 2017

BridgeAR reviewed Dec 6, 2017

View reviewed changes

XadillaX added 2 commits December 6, 2017 09:40

update after @BridgeAR's reviewing

caab532

fix a lint nit

ac04c85

addaleax approved these changes Dec 6, 2017

View reviewed changes

addaleax closed this Dec 6, 2017

XadillaX mentioned this pull request Dec 8, 2017

v8.9.3 proposal #17532

Merged

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added lts-watch-v8.x baking-for-lts PRs that need to wait before landing in a LTS release. labels Dec 20, 2017

XadillaX mentioned this pull request Dec 20, 2017

[v8.x backport] dns: fix crash while setting server during query #17778

Closed

3 tasks

gibfahn added backported-to-v8.x and removed lts-watch-v8.x labels Feb 19, 2018

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

addaleax mentioned this pull request Jan 23, 2019

doc: clarify what dns.setResolvers() affects #25570

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: fix crash while setting server during query #14891

dns: fix crash while setting server during query #14891

XadillaX commented Aug 17, 2017 •

edited

Loading

XadillaX commented Aug 17, 2017 •

edited

Loading

tniessen commented Aug 17, 2017

addaleax commented Aug 17, 2017

addaleax commented Aug 17, 2017 •

edited

Loading

tniessen commented Aug 17, 2017

XadillaX commented Aug 18, 2017 •

edited

Loading

XadillaX commented Aug 18, 2017

tniessen commented Aug 18, 2017

XadillaX commented Aug 18, 2017 •

edited

Loading

BridgeAR commented Sep 2, 2017

XadillaX commented Sep 3, 2017 •

edited

Loading

BridgeAR commented Sep 3, 2017

XadillaX commented Sep 5, 2017 •

edited

Loading

tniessen commented Sep 8, 2017

addaleax commented Sep 8, 2017

XadillaX commented Sep 10, 2017

addaleax commented Sep 10, 2017

XadillaX commented Sep 10, 2017 •

edited

Loading

addaleax commented Sep 10, 2017

XadillaX commented Sep 11, 2017 •

edited

Loading

addaleax commented Sep 11, 2017

XadillaX commented Sep 11, 2017 •

edited

Loading

BridgeAR commented Sep 19, 2017

BridgeAR commented Sep 27, 2017

addaleax commented Nov 20, 2017

XadillaX commented Dec 5, 2017

XadillaX commented Dec 5, 2017

BridgeAR Dec 5, 2017

BridgeAR Dec 5, 2017

BridgeAR Dec 5, 2017

XadillaX commented Dec 6, 2017

addaleax commented Dec 6, 2017

dns: fix crash while setting server during query #14891

dns: fix crash while setting server during query #14891

Conversation

XadillaX commented Aug 17, 2017 • edited Loading

Checklist

Affected core subsystem(s)

XadillaX commented Aug 17, 2017 • edited Loading

tniessen commented Aug 17, 2017

addaleax commented Aug 17, 2017

addaleax commented Aug 17, 2017 • edited Loading

tniessen commented Aug 17, 2017

XadillaX commented Aug 18, 2017 • edited Loading

XadillaX commented Aug 18, 2017

tniessen commented Aug 18, 2017

XadillaX commented Aug 18, 2017 • edited Loading

BridgeAR commented Sep 2, 2017

XadillaX commented Sep 3, 2017 • edited Loading

BridgeAR commented Sep 3, 2017

XadillaX commented Sep 5, 2017 • edited Loading

tniessen commented Sep 8, 2017

addaleax commented Sep 8, 2017

XadillaX commented Sep 10, 2017

addaleax commented Sep 10, 2017

XadillaX commented Sep 10, 2017 • edited Loading

addaleax commented Sep 10, 2017

XadillaX commented Sep 11, 2017 • edited Loading

addaleax commented Sep 11, 2017

XadillaX commented Sep 11, 2017 • edited Loading

BridgeAR commented Sep 19, 2017

BridgeAR commented Sep 27, 2017

addaleax commented Nov 20, 2017

XadillaX commented Dec 5, 2017

XadillaX commented Dec 5, 2017

BridgeAR Dec 5, 2017

Choose a reason for hiding this comment

BridgeAR Dec 5, 2017

Choose a reason for hiding this comment

BridgeAR Dec 5, 2017

Choose a reason for hiding this comment

XadillaX commented Dec 6, 2017

addaleax commented Dec 6, 2017

XadillaX commented Aug 17, 2017 •

edited

Loading

XadillaX commented Aug 17, 2017 •

edited

Loading

addaleax commented Aug 17, 2017 •

edited

Loading

XadillaX commented Aug 18, 2017 •

edited

Loading

XadillaX commented Aug 18, 2017 •

edited

Loading

XadillaX commented Sep 3, 2017 •

edited

Loading

XadillaX commented Sep 5, 2017 •

edited

Loading

XadillaX commented Sep 10, 2017 •

edited

Loading

XadillaX commented Sep 11, 2017 •

edited

Loading

XadillaX commented Sep 11, 2017 •

edited

Loading