dns: fix crash while using dns.setServers after dns.resolve4

[XadillaX]

The callback function in cares_query is synchronous and called before closed. So dns.setServers in the synchronous callback before closed will occur crashing.

This PR makes the real callback of dns.resolve4 or some other functions in dns asynchronous to resolve this problem.

Solution

I use uv_async_t to send the callback task to next loop. And the task data is created from CaresAsyncData.

Because Callback has two functions with parameters hostent* or unsigned char*, int, CaresAsyncData has a flag is_host to distinguish them.

In the Callback function, whether hostent* or unsigned char* will be destroyed after this function is done. So I make a copy for those buffers for asynchronous use. In the asynchronous function and asynchronous done callback, I delete some related pointers.

Related Issue: #894 #1071

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

dns

[XadillaX]

The related cares code is under ares_process.c.

query->callback(query->arg, status, query->timeouts, abuf, alen);
ares__free_query(query);

If real callback is synchronous and we do dns.setServers in the callback, the program will crash because ares_set_servers will destroy current servers first.

[Trott]

Awesome!

Someone else will have to comment on the C++ code. /cc @trevnorris @indutny @addaleax @danbev

But I can comment on the test. The test itself looks good. Some small comments:

• I'm pretty sure the test file needs to go in test/internet because it attempts to do a DNS resolution.
• In the comment, it's probably a good idea to use the full URL for the GitHub issues rather than the #1234 shortcut. We have a lot of old tests where it is ambiguous if #1234 means issue 1234 in this repo or if it is referring to github.com/nodejs/node-v0.x-archive/. Using the full URL will (hopefully) eliminate that ambiguity.

/cc @nodejs/testing

[XadillaX]

@Trott

• I wrote test refer to test-dns-regress-6244, it resolve4 with 127.0.0.1 and actually has no internet request. And then I setServers with 127.0.0.1 too. And I think we can discuss more about it.
• I've updated the comment.

Thanks for your suggestions.

[Trott]

I wrote test refer to test-dns-regress-6244, it resolve4 with 127.0.0.1 and actually has no internet request.

Are you sure about that? If I change your test code to this:

'use strict';

const common = require('../common');
const dns = require('dns');

dns.resolve4('nodejs.org', common.mustCall(function(err, nameServers) {
  if (err)
    throw err;
}));

...running it with a network connection results in no error. If I run it without a network connection enabled, I get ECONNREFUSED. I haven't fired up Wireshark or anything, but that sure seems to suggest it's sending a DNS request...

[XadillaX]

I see. Actually I don't care about whether it will throw an error or not. My test destination is to test whether the case will crash or not.

Anyway, I moved the test case under internet now.

Before this fixture PR, that test case will fail because of crash.

[silverwind]

Shouldn't this target master instead of v6.x? Because as far as I know, the bug is still present there.

[XadillaX]

@silverwind hmmmm, I think if this PR is merged, we can port it to master.

But if reviewers consider that this PR must target to master, then I will change the target.

[silverwind]

We generally land on master first and then backport it after a few weeks/months, so I'd appreciate if you target master now.

[XadillaX]

@silverwind I've updated the PR to make master as base. And who help to update the label 6.x?

[silverwind]

I can confirm it fixes the test case in #894.

[pmq20]

Excellent work! Thanks for the patch. To my understanding this PR simply postponed the execution of the callback of dns.resolve4 to the next tick, yet achieved this in a lot of code. I would prefer a simpler approach (or even only patch the JS level if possible), e.g. wrapping the callback function into a process.nextTick(). It would be easier to land if the diff was smaller. Do you think it's possible? @XadillaX

[XadillaX]

My consideration is that:

If we solve this bug in JavaScript level, the bug will still exist if developers use DNS binding directly. @pmq20

Or if someone add some new features for dns in dns.js, he / she may forget this bug and then uses sync code.

[addaleax]

This is code that was licensed under the GPL, we can’t use it – sorry.

[XadillaX]

Okay, I'll rewrite this function then.

[addaleax]

(See also https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#developers-certificate-of-origin)

[XadillaX]

Thx. I've rewritten those two functions by myself now.

[addaleax]

Landed in 5a948f6, thanks for the PR!

[refack]

@XadillaX congrats 🎉 is this your first code contribution?

[silverwind]

It is. Pretty awesome first contribution!

[XadillaX]

Yes, it is my first contribution. Thank you all for helping with my first contribution!

[sam-github]

2 new defect(s) introduced to Node.js found with Coverity Scan.


New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)


** CID 169571:  Error handling issues  (CHECKED_RETURN)
/src/cares_wrap.cc: 467 in node::cares_wrap::<unnamed>::QueryWrap::Callback(void *, int, int, unsigned char *, int)()


________________________________________________________________________________________________________
*** CID 169571:  Error handling issues  (CHECKED_RETURN)
/src/cares_wrap.cc: 467 in node::cares_wrap::<unnamed>::QueryWrap::Callback(void *, int, int, unsigned char *, int)()
461         data->wrap = wrap;
462         data->is_host = false;
463         data->data.buf = buf_copy;
464         data->len = answer_len;
465
466         uv_async_t* async_handle = &data->async_handle;
>>>     CID 169571:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "uv_async_init" without checking return value (as is done elsewhere 9 out of 11 times).
467         uv_async_init(wrap->env()->event_loop(), async_handle, CaresAsyncCb);
468
469         async_handle->data = data;
470         uv_async_send(async_handle);
471       }
472

** CID 169570:  Error handling issues  (CHECKED_RETURN)
/src/cares_wrap.cc: 490 in node::cares_wrap::<unnamed>::QueryWrap::Callback(void *, int, int, hostent *)()


________________________________________________________________________________________________________
*** CID 169570:  Error handling issues  (CHECKED_RETURN)
/src/cares_wrap.cc: 490 in node::cares_wrap::<unnamed>::QueryWrap::Callback(void *, int, int, hostent *)()
484         data->status = status;
485         data->data.host = host_copy;
486         data->wrap = wrap;
487         data->is_host = true;
488
489         uv_async_t* async_handle = &data->async_handle;
>>>     CID 169570:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "uv_async_init" without checking return value (as is done elsewhere 9 out of 11 times).
490         uv_async_init(wrap->env()->event_loop(), async_handle, CaresAsyncCb);
491
492         async_handle->data = data;
493         uv_async_send(async_handle);
494       }
495

[cjihrig]

@sam-github #13116

[MylesBorins]

It would appear that this is causing build errors on v6.x. I have labelled it as dont-land

If it does land it needs to come with 06f62eb

edit: I left dont land off for now until we hear that someone can say if this needs to land