Skip to content

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

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

Closed
XadillaX 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 2 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
198 changes: 189 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,121 @@ Local<Array> HostentToNames(Environment* env, struct hostent* host) {
return scope.Escape(names);
}

/* copies a hostent structure*, returns 0 on success, -1 on error */
/* this function refers to OpenSIPS */
Copy link
Member

@addaleax addaleax May 17, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@XadillaX XadillaX May 17, 2017

Choose a reason for hiding this comment

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

Okay, I'll rewrite this function then.

Copy link
Member

@addaleax addaleax May 17, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@XadillaX XadillaX May 17, 2017

Choose a reason for hiding this comment

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

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

int cares_wrap_hostent_cpy(struct hostent *dst, struct hostent* src) {
unsigned int len, len2, i, r;

/* start copying the host entry.. */
/* copy h_name */
len = strlen(src->h_name) + 1;
dst->h_name = reinterpret_cast<char*>(node::Malloc(sizeof(char) * len));
if (dst->h_name) {
strncpy(dst->h_name, src->h_name, len);
} else {
return -1;
}

/* copy h_aliases */
len = 0;
if (src->h_aliases) {
for (; src->h_aliases[len]; len++) {}
}

dst->h_aliases = reinterpret_cast<char**>(
node::Malloc(sizeof(char*) * (len + 1)));

if (dst->h_aliases == 0) {
free(dst->h_name);
return -1;
}

memset(reinterpret_cast<void*>(dst->h_aliases), 0, sizeof(char*) * (len + 1));
for (i = 0; i < len; i++) {
len2 = strlen(src->h_aliases[i]) + 1;
dst->h_aliases[i] = reinterpret_cast<char*>(
node::Malloc(sizeof(char) * len2));

if (dst->h_aliases[i] == 0) {
free(dst->h_name);
for (r = 0; r < i; r++) free(dst->h_aliases[r]);
free(dst->h_aliases);
return -1;
}

strncpy(dst->h_aliases[i], src->h_aliases[i], len2);
}

/* copy h_addr_list */
len = 0;
if (src->h_addr_list) {
for (; src->h_addr_list[len]; len++) {}
}

dst->h_addr_list = reinterpret_cast<char**>(
node::Malloc(sizeof(char*) * (len + 1)));

if (dst->h_addr_list == 0) {
free(dst->h_name);
for (r = 0; dst->h_aliases[r]; r++) free(dst->h_aliases[r]);
free(dst->h_aliases);
return -1;
}

memset(reinterpret_cast<void*>(
dst->h_addr_list), 0, sizeof(char*) * (len + 1));

for (i=0; i < len; i++) {
dst->h_addr_list[i] = reinterpret_cast<char*>(
node::Malloc(sizeof(char) * src->h_length));
if (dst->h_addr_list[i] == 0) {
free(dst->h_name);
for (r = 0; dst->h_aliases[r]; r++) free(dst->h_aliases[r]);
free(dst->h_aliases);
for (r = 0; r < i; r++) free(dst->h_addr_list[r]);
free(dst->h_addr_list);
return -1;
}

memcpy(dst->h_addr_list[i], src->h_addr_list[i], src->h_length);
}

/* copy h_addr_type & length */
dst->h_addrtype = src->h_addrtype;
dst->h_length = src->h_length;
/*finished hostent copy */

return 0;
}

/* this function refers to OpenSIPS */
void cares_wrap_free_hostent(struct hostent *dst) {
int r;
if (dst->h_name) free(dst->h_name);
if (dst->h_aliases) {
for (r = 0; dst->h_aliases[r]; r++) {
free(dst->h_aliases[r]);
}
free(dst->h_aliases);
}
if (dst->h_addr_list) {
for (r = 0; dst->h_addr_list[r]; r++) {
free(dst->h_addr_list[r]);
}
free(dst->h_addr_list);
}
}

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

@addaleax addaleax May 17, 2017

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 +447,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

@addaleax addaleax May 17, 2017

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

@addaleax addaleax May 17, 2017

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);
cares_wrap_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

@addaleax addaleax May 17, 2017

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

@addaleax addaleax May 17, 2017

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;
int copy_ret = 0;

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

@addaleax addaleax May 17, 2017

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

@addaleax addaleax May 17, 2017

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

@addaleax addaleax May 17, 2017

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 == 0) {
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

@cjihrig cjihrig May 17, 2017

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

@XadillaX XadillaX May 17, 2017

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
}));