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.

Sign up for GitHub

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

Jump to bottom

(v6.x backport) dns: fix crash while using dns.setServers after dns.resolve4 #13724

Closed
wants to merge 3 commits into from
Closed

(v6.x backport) dns: fix crash while using dns.setServers after dns.resolve4 #13724

wants to merge 3 commits into from

Conversation

XadillaX
Copy link
Contributor

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 added 2 commits June 17, 2017 02:35
@XadillaX
dns: fix crash using dns.setServers after resolve4
510e626 
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
make it compilable
9d69519
@nodejs-github-bot 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. v6.x labels Jun 16, 2017
@XadillaX XadillaX changed the title (v4.x backport) dns: fix crash while using dns.setServers after dns.resolve4 (v6.x backport) dns: fix crash while using dns.setServers after dns.resolve4 Jun 16, 2017
@gibfahn
Copy link
Member

gibfahn commented Jun 19, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-commit/10667/

CI was green.

@gibfahn
Copy link
Member

gibfahn commented Jun 20, 2017

@addaleax LGTY (you reviewed the original)?

@gibfahn gibfahn force-pushed the v6.x-staging branch 2 times, most recently from 6ef8c5b to 4c2fa3d Compare June 20, 2017 19:04
addaleax
addaleax reviewed Jun 21, 2017
View reviewed changes
src/cares_wrap.cc Outdated
@@ -318,7 +318,7 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) {

/* copy `h_name` */
size_t name_size = strlen(src->h_name) + 1;
dest->h_name = node::Malloc<char>(name_size);
dest->h_name = reinterpret_cast<char*>(node::Malloc(name_size));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you avoid reinterpret_cast when a static_cast does the job just fine? Alternatively, you could also backport the overloads of node::Malloc & similar methods as they are on master, that would help with backporting other patches in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax #8482 it shows dont-land-on-v6.x and v4.x. May I still make a backport for it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XadillaX Yes, you’ll just need to try and see how to resolve the conflicts that you’re going to get. I suspect that will be a bit of work, so only do that if you have the time; for now, just changing to static_cast here is definitely easier. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax I found a backport chain for #8482, it will be a big backport. So I decide to only backport this PR but not #8482 and its dependencies.

@addaleax
Copy link
Member

@gibfahn Mostly LGTM, apart from what I just commented. This should also land together with 06f62eb, just so you have that on your radar.

@gibfahn
Copy link
Member

gibfahn commented Jun 22, 2017

CI 2: https://ci.nodejs.org/job/node-test-commit/10732/

@MylesBorins
Copy link
Contributor

Is landing this within our backporting contract? If so someone from @nodejs/backporting can feel free to land on staging and we will put it out in a future maintenance release

@XadillaX
Copy link
Contributor Author

XadillaX commented Jul 3, 2017 •
edited
Loading

Do remember to backport 06f62eb before release.

@MylesBorins
Copy link
Contributor

landed in d3caea7

MylesBorins pushed a commit that referenced this pull request Jul 10, 2017
@XadillaX @MylesBorins
dns: fix crash using dns.setServers after resolve4
d3caea7 
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 pushed a commit that referenced this pull request Jul 11, 2017
@XadillaX @MylesBorins
dns: fix crash using dns.setServers after resolve4
18cbee2 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@XadillaX @gibfahn @addaleax @MylesBorins @jasnell @nodejs-github-bot