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

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0c51d0a
dns: fix crash while using dns.setServers after dns.resolve4
XadillaX May 17, 2017
c6ac529
fix a bug may occure memory leaking (this commit will be rebased)
XadillaX May 17, 2017
fd610db
rewrite cares_wrap_hostent_cpy by myself
XadillaX May 17, 2017
56aff34
update after review (this commit will be rebased)
XadillaX May 17, 2017
00c2c11
update after review again (this commit will be rebased)
XadillaX May 18, 2017
7390ee0
add the forgotten ending nullptr
XadillaX May 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 192 additions & 9 deletions src/cares_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ using v8::Value;

namespace {

#define ARES_HOSTENT_COPY_ERROR -1000

inline const char* ToErrorCodeString(int status) {
switch (status) {
#define V(code) case ARES_##code: return #code;
Expand Down Expand Up @@ -96,8 +98,10 @@ inline const char* ToErrorCodeString(int status) {
V(EREFUSED)
V(ESERVFAIL)
V(ETIMEOUT)
V(HOSTENT_COPY_ERROR)
#undef V
}

return "UNKNOWN_ARES_ERROR";
}

Expand Down Expand Up @@ -296,6 +300,124 @@ Local<Array> HostentToNames(Environment* env, struct hostent* host) {
return scope.Escape(names);
}

void safe_free_hostent(struct hostent* host) {
int idx;

if (host->h_addr_list) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a style nit, but we do prefer explicit != nullptr or == nullptr for these checks

idx = 0;
while (host->h_addr_list[idx]) {
free(host->h_addr_list[idx++]);
}
free(host->h_addr_list);
host->h_addr_list = 0;
}

if (host->h_aliases) {
idx = 0;
while (host->h_aliases[idx]) {
free(host->h_aliases[idx++]);
}
free(host->h_aliases);
host->h_aliases = 0;
}

if (host->h_name) {
free(host->h_name);
}

host->h_addrtype = host->h_length = 0;
}

bool cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) {
dest->h_addr_list = nullptr;
dest->h_addrtype = 0;
dest->h_aliases = nullptr;
dest->h_length = 0;
dest->h_name = nullptr;

/* copy `h_name` */
unsigned int name_size = (strlen(src->h_name) + 1) * sizeof(char);
Copy link
Member

Choose a reason for hiding this comment

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

sizeof(char) is guaranteed to be 1, you can drop it :)

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.

There should be no need to cast here

if (!dest->h_name) return false;
memcpy(dest->h_name, src->h_name, name_size);

/* copy `h_aliases` */
unsigned int alias_count;
unsigned int alias_size;
unsigned int cur_alias_length;
Copy link
Member

Choose a reason for hiding this comment

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

I think size_t would be the right for all of these (and other vars probably, too?)

for (alias_count = 0;
src->h_aliases[alias_count] != nullptr;
alias_count++) {
}
alias_size = sizeof(char*) * (alias_count + 1);

dest->h_aliases = reinterpret_cast<char**>(node::Malloc(alias_size));
Copy link
Member

Choose a reason for hiding this comment

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

h_aliases = node::Malloc<char*>(alias_count+1) should work fine

if (dest->h_aliases == nullptr) {
safe_free_hostent(dest);
return false;
}

memset(reinterpret_cast<void*>(dest->h_aliases), 0, alias_size);
Copy link
Member

Choose a reason for hiding this comment

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

reinterpret_cast → static_cast?

for (unsigned int i = 0; i < alias_count; i++) {
cur_alias_length = strlen(src->h_aliases[i]);
dest->h_aliases[i] = reinterpret_cast<char*>(
node::Malloc(sizeof(char) * (cur_alias_length + 1)));

if (dest->h_aliases[i] == nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

node::Malloc() aborts if it sees an OOM – I personally like code that does checking, but you’ll want node::UncheckedMalloc() for the variant that can return a nullptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmmm, so what's your suggestion here?

Use node::UncheckedMalloc or use malloc directly? Or leave node::Malloc here and delete the nullptr checking?

safe_free_hostent(dest);
return false;
}

memcpy(dest->h_aliases[i],
src->h_aliases[i],
sizeof(char) * (cur_alias_length + 1));
Copy link
Member

Choose a reason for hiding this comment

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

We do like to align our arguments vertically :) (i.e. make src-> come directly below dest->)

}

/* copy `h_addr_list` */
unsigned int list_count;
Copy link
Member

Choose a reason for hiding this comment

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

size_t?

unsigned int list_size;
unsigned int node_size;
for (list_count = 0;
src->h_addr_list[list_count] != nullptr;
list_count++) {
}
list_size = sizeof(char*) * (list_count + 1);

dest->h_addr_list = reinterpret_cast<char**>(node::Malloc(list_size));
if (dest->h_addr_list == nullptr) {
safe_free_hostent(dest);
return false;
}

node_size = sizeof(char) * src->h_length;
memset(reinterpret_cast<void*>(dest->h_addr_list), 0, list_size);
for (unsigned int i = 0; i < list_count; i++) {
dest->h_addr_list[i] = reinterpret_cast<char*>(node::Malloc(node_size));
if (dest->h_addr_list[i] == nullptr) {
safe_free_hostent(dest);
return false;
}

memcpy(dest->h_addr_list[i], src->h_addr_list, node_size);
}

/* work after work */
dest->h_length = src->h_length;
dest->h_addrtype = src->h_addrtype;

return true;
}

class QueryWrap;
struct CaresAsyncData {
QueryWrap* wrap;
int status;
bool is_host;
void* buf;
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth turning this into a union { hostent* h; unsigned char* buf }, what do you think?

int len;

uv_async_t async_handle;
};

class QueryWrap : public AsyncWrap {
public:
Expand Down Expand Up @@ -328,30 +450,91 @@ class QueryWrap : public AsyncWrap {
return static_cast<void*>(this);
}

static void Callback(void *arg, int status, int timeouts,
unsigned char* answer_buf, int answer_len) {
QueryWrap* wrap = static_cast<QueryWrap*>(arg);
static void CaresAsyncClose(uv_handle_t* handle) {
uv_async_t* async = reinterpret_cast<uv_async_t*>(handle);
struct CaresAsyncData* data =
static_cast<struct CaresAsyncData*>(async->data);
Copy link
Member

Choose a reason for hiding this comment

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

I think in this case it should be fine to just use auto data =, the type is obvious from the static_cast

delete data->wrap;
delete data;
}

static void CaresAsyncCb(uv_async_t* handle) {
struct CaresAsyncData* data =
static_cast<struct CaresAsyncData*>(handle->data);

QueryWrap* wrap = data->wrap;
int status = data->status;

if (status != ARES_SUCCESS) {
wrap->ParseError(status);
} else if (!data->is_host) {
unsigned char* buf = reinterpret_cast<unsigned char*>(data->buf);
Copy link
Member

Choose a reason for hiding this comment

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

static_cast should do fine here

wrap->Parse(buf, data->len);
free(buf);
} else {
wrap->Parse(answer_buf, answer_len);
hostent* host = static_cast<struct hostent*>(data->buf);
wrap->Parse(host);
safe_free_hostent(host);
free(host);
}

delete wrap;
uv_close(reinterpret_cast<uv_handle_t*>(handle), CaresAsyncClose);
}

static void Callback(void *arg, int status, int timeouts,
unsigned char* answer_buf, int answer_len) {
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: Can you align the arguments vertically here, too?

QueryWrap* wrap = static_cast<QueryWrap*>(arg);

unsigned char* buf_copy = nullptr;
if (status == ARES_SUCCESS) {
buf_copy = (unsigned char*)node::Malloc(
sizeof(unsigned char) * answer_len);
Copy link
Member

Choose a reason for hiding this comment

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

node::Malloc<unsigned char>(answer_len) should be fine – we generally avoid C-style casts

memcpy(buf_copy, answer_buf, sizeof(unsigned char) * answer_len);
}

struct CaresAsyncData* data = new struct CaresAsyncData();
data->status = status;
data->wrap = wrap;
data->is_host = false;
data->buf = buf_copy;
data->len = answer_len;

uv_async_t* async_handle = &data->async_handle;
uv_async_init(wrap->env()->event_loop(), async_handle, CaresAsyncCb);

async_handle->data = data;
uv_async_send(async_handle);
}

static void Callback(void *arg, int status, int timeouts,
struct hostent* host) {
QueryWrap* wrap = static_cast<QueryWrap*>(arg);

if (status != ARES_SUCCESS) {
wrap->ParseError(status);
struct hostent* host_copy = nullptr;
bool copy_ret = false;

if (status == ARES_SUCCESS) {
host_copy = (struct hostent*)node::Malloc(sizeof(hostent));
Copy link
Member

Choose a reason for hiding this comment

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

node::Malloc<hostent>() (okay, I’m going to stop commenting, you get the idea. :))

copy_ret = cares_wrap_hostent_cpy(host_copy, host);
Copy link
Member

Choose a reason for hiding this comment

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

copy_ret can’t be false anymore, right? That should make everything a bit simpler.

}

struct CaresAsyncData* data = new struct CaresAsyncData();
Copy link
Member

Choose a reason for hiding this comment

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

style nit: drop the struct (either just the one after new or both), it’s not necessary in C++ and we don’t do that elsewhere

if (copy_ret) {
data->status = status;
data->buf = host_copy;
} else {
wrap->Parse(host);
data->status = ARES_HOSTENT_COPY_ERROR;
data->buf = nullptr;
free(host_copy);
}
data->wrap = wrap;
data->is_host = true;

delete wrap;
uv_async_t* async_handle = &data->async_handle;
uv_async_init(wrap->env()->event_loop(), async_handle, CaresAsyncCb);

async_handle->data = data;
uv_async_send(async_handle);
}

void CallOnComplete(Local<Value> answer,
Expand Down
17 changes: 17 additions & 0 deletions test/internet/test-dns-setserver-in-callback-of-resolve4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';

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

dns.resolve4('google.com', common.mustCall(function(/* err, nameServers */) {
// do not care about `err` and `nameServers`,
// both failed and succeeded would be OK.
//
// just to test crash

dns.setServers([ '8.8.8.8' ]);

// the test case shouldn't crash here
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments can probably be dropped, but if you want to keep them, could you move them to the top like this. Note the Refs: notation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. √

// see https://github.com/nodejs/node/pull/13050 and
// https://github.com/nodejs/node/issues/894
}));