Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dns.setServers() crash #894

Closed
BLeQuerrec opened this issue Feb 19, 2015 · 27 comments
Closed

dns.setServers() crash #894

BLeQuerrec opened this issue Feb 19, 2015 · 27 comments
Assignees

Comments

@BLeQuerrec
Copy link

@BLeQuerrec BLeQuerrec commented Feb 19, 2015

Hi,

In a Vagrant Ubuntu environment, io.js crashes with this code:

$ cat dns.js
var dns = require('dns');

dns.resolveSoa('example.org', function(err, soa){
    console.log(soa.nsname);
    dns.resolve4(soa.nsname, function (err, nameServers) {
        console.log(dns.getServers());
        dns.setServers(nameServers);
        console.log(dns.getServers());
    });
});

Expected result:

$ iojs dns.js
sns.dns.icann.org
[ '10.0.2.3' ]
[ '199.4.28.26' ]

Actual result:

$ iojs dns.js
sns.dns.icann.org
[ '10.0.2.3' ]
iojs: ../deps/cares/src/ares_destroy.c:102: ares__destroy_servers_state: Assertion `ares__is_list_empty(&server->queries_to_server)' failed.
Aborted (core dumped)

Other informations:

$ cat /etc/os-release
NAME="Ubuntu"
VERSION="14.04.1 LTS, Trusty Tahr"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 14.04.1 LTS"
VERSION_ID="14.04"
HOME_URL="http://www.ubuntu.com/"
SUPPORT_URL="http://help.ubuntu.com/"
BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/"
$ iojs -v
v1.2.0

DNS test (in test/simple/test-dns.js) ends successfully. This bug can be fixed by removing line 102 in deps/cares/src/ares_destroy.c.
Node.js bug: nodejs/node-v0.x-archive#9243

@tellnes
Copy link
Member

@tellnes tellnes commented Feb 19, 2015

I'm seeing similar results on os x 10.9.5

$ iojs dns.js
sns.dns.icann.org
[ '10.0.0.1' ]
Assertion failed: (ares__is_list_empty(&server->queries_to_server)), function ares__destroy_servers_state, file ../deps/cares/src/ares_destroy.c, line 102.
Abort trap: 6

Loading

@rvagg
Copy link
Member

@rvagg rvagg commented Feb 19, 2015

@iojs/tc can someone qualified jump on this? otherwise it'll have to go in to "known issues" in 1.3.0.

Loading

@indutny
Copy link
Member

@indutny indutny commented Feb 19, 2015

Will do.

Loading

@indutny indutny self-assigned this Feb 19, 2015
indutny added a commit to indutny/io.js that referenced this issue Feb 20, 2015
`ares_set_servers()` should terminate all active queries to previous
servers before destroying them.

When invoking query callback remove it from the queue, to ensure that it
won't be called two times.

Fix: nodejs#894
@indutny
Copy link
Member

@indutny indutny commented Feb 20, 2015

I have two solutions to the problem:

  • #899
  • throw in JS if there is any active queries
  • wait until query completion before changing the servers

What do you think will be most preferable? cc @bnoordhuis

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented Feb 20, 2015

wait until query completion before changing the servers

Would that block new queries from using the updated servers until all outstanding requests finish? If so, any way to have setServers apply for all following queries?

Loading

@tellnes
Copy link
Member

@tellnes tellnes commented Feb 20, 2015

The best solution would be that queries created before calling setServers is using the old servers while queries created after calling setServers is using the new servers.

If I understand your solutions correctly, that would mean wait until query completion and queue up new queries.

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented Feb 20, 2015

Meant what @tellnes said. If that's possible, there's no need to throw.

Loading

@indutny
Copy link
Member

@indutny indutny commented Feb 20, 2015

Good suggestion. I'll see what I can do.

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented Mar 10, 2015

I'm unsure how far you got here @indutny, but if it proves easier to add support for a server option on resolve like I proposed in #1071, we could possibly deprecate setServers and getServers in 2.0 or 3.0.

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented Mar 11, 2015

Once this is fixed to work like #894 (comment) describes, I can work on adding the resolve option for a truly async API. Probably not worth to deprecate after all.

Loading

rvagg added a commit to rvagg/io.js that referenced this issue Mar 12, 2015
Use an EE with some state data to gate async resolution operations in
c-ares so that setServers() can run independently.

Likely a temporary fix for nodejs#894
with a better solution being to fix c-ares to do this without barfing.
@arenddeboer
Copy link

@arenddeboer arenddeboer commented Dec 26, 2015

Any progress on this ?

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented Feb 8, 2016

@emotional-engineering c-ares is now on GitHub. Would you mind posting your patch as a pull request there? @indutny mentioned they should be more responsive now.

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented Feb 12, 2016

Loading

@jasnell
Copy link
Member

@jasnell jasnell commented Apr 22, 2016

Closing this as it's a duplicate of #1071 (or, 1071 is a duplicate of this... either way, we don't need two issues tracking the same problem). It's still an open issue, however.

Loading

@jasnell jasnell closed this Apr 22, 2016
@silverwind
Copy link
Contributor

@silverwind silverwind commented Apr 22, 2016

I have to object. This is the original issue tracking the crash. The other one is more of an API topic.

@indutny you're still assigned here?

Loading

@silverwind silverwind reopened this Apr 22, 2016
@jasnell
Copy link
Member

@jasnell jasnell commented Apr 22, 2016

@silverwind .. ok, no problem :-) just trying to do a bit of housecleaning.

Loading

@raimondi1337
Copy link

@raimondi1337 raimondi1337 commented Dec 15, 2016

Ran into this issue today on node v6.9.1

I'm running this loop thousands of times one after another and it works:

    dns.setServers([server['ip']]);
    dns.lookup('something.com', fn()(...));

but this breaks after two executions resulting in the error in the original post:

    dns.setServers([server['ip']]);
    dns.resolve('something.com', fn()(...));

has this been fixed for lookup but not resolve or something like that?

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented Dec 15, 2016

@raimondi1337 lookup uses the system resolver, so does not go through c-ares. setServers has no effect on lookup.

Loading

@addaleax addaleax closed this in 5a948f6 May 18, 2017
anchnk pushed a commit to anchnk/node that referenced this issue May 19, 2017
The callback function in cares_query is synchronous and called before
closed. So dns.setServers in the synchronous callback before closed will
occur crashing.

Fixes: nodejs#894
Refs: https://github.com/nodejs/node/blob/v6.9.4/deps/cares/src/ares_process.c#L1332-L1333
Refs: https://github.com/OpenSIPS/opensips/blob/2.3.0/proxy.c
PR-URL: nodejs#13050
Reviewed-By: Anna Henningsen <anna@addaleax.net>
XadillaX added a commit to XadillaX/node that referenced this issue Jun 16, 2017
The callback function in cares_query is synchronous and called before
closed. So dns.setServers in the synchronous callback before closed will
occur crashing.

Fixes: nodejs#894
Refs: https://github.com/nodejs/node/blob/v6.9.4/deps/cares/src/ares_process.c#L1332-L1333
Refs: https://github.com/OpenSIPS/opensips/blob/2.3.0/proxy.c
PR-URL: nodejs#13050
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit that referenced this issue Jul 10, 2017
The callback function in cares_query is synchronous and called before
closed. So dns.setServers in the synchronous callback before closed will
occur crashing.

Fixes: #894
Refs: https://github.com/nodejs/node/blob/v6.9.4/deps/cares/src/ares_process.c#L1332-L1333
Refs: https://github.com/OpenSIPS/opensips/blob/2.3.0/proxy.c
Backport-PR-URL: #13724
PR-URL: #13050
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit that referenced this issue Jul 11, 2017
The callback function in cares_query is synchronous and called before
closed. So dns.setServers in the synchronous callback before closed will
occur crashing.

Fixes: #894
Refs: https://github.com/nodejs/node/blob/v6.9.4/deps/cares/src/ares_process.c#L1332-L1333
Refs: https://github.com/OpenSIPS/opensips/blob/2.3.0/proxy.c
Backport-PR-URL: #13724
PR-URL: #13050
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@dzek69
Copy link

@dzek69 dzek69 commented Jun 16, 2018

If noone noticed - this is fixed with 9.3.0 version and fix seems to be backported to 8.10.0 too.

But I monkey patched dns module to fix that - if anyone needs to keep unpatched Node version and doesn't want the crashes - please use my fix.
More information here: https://github.com/dzek69/node-dns-bugfix

Loading

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Oct 22, 2018
Original commit message:

    Prevent changing name servers while queries are outstanding

    Changing name servers doesn't work, per c-ares/c-ares#41.
    Better to return an error code than to crash.

Refs: c-ares/c-ares#41
Refs: c-ares/c-ares#191
Refs: nodejs#894
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet