Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit ea26ac0

Browse filesBrowse files
committed
dns: refactor QueryWrap lifetime management
- Prefer RAII-style management over manual resource management. - Prefer `env->SetImmediate()` over a separate `uv_async_t`. - Perform `ares_destroy()` before possibly tearing down c-ares state. - Verify that the number of active queries is non-negative. - Let pending callbacks know when their underlying `QueryWrap` object has been destroyed. The last item has been a real bug, in that when Workers shut down during currently running DNS queries, they may run into use-after-free situations because: 1. Shutting the `Worker` down leads to the cleanup code deleting the `QueryWrap` objects first; then 2. deleting the `ChannelWrap` object (as it has been created before the `QueryWrap`s), whose destructor runs `ares_destroy()`, which in turn invokes all pending query callbacks with `ARES_ECANCELLED`, 3. which lead to use-after-free, as the callback tried to access the deleted `QueryWrap` object. The added test verifies that this is no longer an issue. PR-URL: #26253 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 4561cf3 commit ea26ac0
Copy full SHA for ea26ac0

File tree

Expand file treeCollapse file tree

2 files changed

+87
-66
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+87
-66
lines changed
Open diff view settings
Collapse file

‎src/cares_wrap.cc‎

Copy file name to clipboardExpand all lines: src/cares_wrap.cc
+64-66Lines changed: 64 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -407,14 +407,11 @@ void safe_free_hostent(struct hostent* host) {
407407
host->h_aliases = nullptr;
408408
}
409409

410-
if (host->h_name != nullptr) {
411-
free(host->h_name);
412-
}
413-
414-
host->h_addrtype = host->h_length = 0;
410+
free(host->h_name);
411+
free(host);
415412
}
416413

417-
void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) {
414+
void cares_wrap_hostent_cpy(struct hostent* dest, const struct hostent* src) {
418415
dest->h_addr_list = nullptr;
419416
dest->h_addrtype = 0;
420417
dest->h_aliases = nullptr;
@@ -461,18 +458,6 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) {
461458
}
462459

463460
class QueryWrap;
464-
struct CaresAsyncData {
465-
QueryWrap* wrap;
466-
int status;
467-
bool is_host;
468-
union {
469-
hostent* host;
470-
unsigned char* buf;
471-
} data;
472-
int len;
473-
474-
uv_async_t async_handle;
475-
};
476461

477462
void ChannelWrap::Setup() {
478463
struct ares_options options;
@@ -525,20 +510,21 @@ void ChannelWrap::CloseTimer() {
525510
}
526511

527512
ChannelWrap::~ChannelWrap() {
513+
ares_destroy(channel_);
514+
528515
if (library_inited_) {
529516
Mutex::ScopedLock lock(ares_library_mutex);
530517
// This decreases the reference counter increased by ares_library_init().
531518
ares_library_cleanup();
532519
}
533520

534-
ares_destroy(channel_);
535521
CloseTimer();
536522
}
537523

538524

539525
void ChannelWrap::ModifyActivityQueryCount(int count) {
540526
active_query_count_ += count;
541-
if (active_query_count_ < 0) active_query_count_ = 0;
527+
CHECK_GE(active_query_count_, 0);
542528
}
543529

544530

@@ -602,6 +588,10 @@ class QueryWrap : public AsyncWrap {
602588

603589
~QueryWrap() override {
604590
CHECK_EQ(false, persistent().IsEmpty());
591+
592+
// Let Callback() know that this object no longer exists.
593+
if (callback_ptr_ != nullptr)
594+
*callback_ptr_ = nullptr;
605595
}
606596

607597
// Subclasses should implement the appropriate Send method.
@@ -624,89 +614,93 @@ class QueryWrap : public AsyncWrap {
624614
TRACING_CATEGORY_NODE2(dns, native), trace_name_, this,
625615
"name", TRACE_STR_COPY(name));
626616
ares_query(channel_->cares_channel(), name, dnsclass, type, Callback,
627-
static_cast<void*>(this));
617+
MakeCallbackPointer());
628618
}
629619

630-
static void CaresAsyncClose(uv_async_t* async) {
631-
auto data = static_cast<struct CaresAsyncData*>(async->data);
632-
delete data->wrap;
633-
delete data;
634-
}
620+
struct ResponseData {
621+
int status;
622+
bool is_host;
623+
DeleteFnPtr<hostent, safe_free_hostent> host;
624+
MallocedBuffer<unsigned char> buf;
625+
};
635626

636-
static void CaresAsyncCb(uv_async_t* handle) {
637-
auto data = static_cast<struct CaresAsyncData*>(handle->data);
627+
void AfterResponse() {
628+
CHECK(response_data_);
638629

639-
QueryWrap* wrap = data->wrap;
640-
int status = data->status;
630+
const int status = response_data_->status;
641631

642632
if (status != ARES_SUCCESS) {
643-
wrap->ParseError(status);
644-
} else if (!data->is_host) {
645-
unsigned char* buf = data->data.buf;
646-
wrap->Parse(buf, data->len);
647-
free(buf);
633+
ParseError(status);
634+
} else if (!response_data_->is_host) {
635+
Parse(response_data_->buf.data, response_data_->buf.size);
648636
} else {
649-
hostent* host = data->data.host;
650-
wrap->Parse(host);
651-
safe_free_hostent(host);
652-
free(host);
637+
Parse(response_data_->host.get());
653638
}
654639

655-
wrap->env()->CloseHandle(handle, CaresAsyncClose);
640+
delete this;
641+
}
642+
643+
void* MakeCallbackPointer() {
644+
CHECK_NULL(callback_ptr_);
645+
callback_ptr_ = new QueryWrap*(this);
646+
return callback_ptr_;
647+
}
648+
649+
static QueryWrap* FromCallbackPointer(void* arg) {
650+
std::unique_ptr<QueryWrap*> wrap_ptr { static_cast<QueryWrap**>(arg) };
651+
QueryWrap* wrap = *wrap_ptr.get();
652+
if (wrap == nullptr) return nullptr;
653+
wrap->callback_ptr_ = nullptr;
654+
return wrap;
656655
}
657656

658657
static void Callback(void* arg, int status, int timeouts,
659658
unsigned char* answer_buf, int answer_len) {
660-
QueryWrap* wrap = static_cast<QueryWrap*>(arg);
659+
QueryWrap* wrap = FromCallbackPointer(arg);
660+
if (wrap == nullptr) return;
661661

662662
unsigned char* buf_copy = nullptr;
663663
if (status == ARES_SUCCESS) {
664664
buf_copy = node::Malloc<unsigned char>(answer_len);
665665
memcpy(buf_copy, answer_buf, answer_len);
666666
}
667667

668-
CaresAsyncData* data = new CaresAsyncData();
668+
wrap->response_data_.reset(new ResponseData());
669+
ResponseData* data = wrap->response_data_.get();
669670
data->status = status;
670-
data->wrap = wrap;
671671
data->is_host = false;
672-
data->data.buf = buf_copy;
673-
data->len = answer_len;
674-
675-
uv_async_t* async_handle = &data->async_handle;
676-
CHECK_EQ(0, uv_async_init(wrap->env()->event_loop(),
677-
async_handle,
678-
CaresAsyncCb));
672+
data->buf = MallocedBuffer<unsigned char>(buf_copy, answer_len);
679673

680-
wrap->channel_->set_query_last_ok(status != ARES_ECONNREFUSED);
681-
wrap->channel_->ModifyActivityQueryCount(-1);
682-
async_handle->data = data;
683-
uv_async_send(async_handle);
674+
wrap->QueueResponseCallback(status);
684675
}
685676

686677
static void Callback(void* arg, int status, int timeouts,
687678
struct hostent* host) {
688-
QueryWrap* wrap = static_cast<QueryWrap*>(arg);
679+
QueryWrap* wrap = FromCallbackPointer(arg);
680+
if (wrap == nullptr) return;
689681

690682
struct hostent* host_copy = nullptr;
691683
if (status == ARES_SUCCESS) {
692684
host_copy = node::Malloc<hostent>(1);
693685
cares_wrap_hostent_cpy(host_copy, host);
694686
}
695687

696-
CaresAsyncData* data = new CaresAsyncData();
688+
wrap->response_data_.reset(new ResponseData());
689+
ResponseData* data = wrap->response_data_.get();
697690
data->status = status;
698-
data->data.host = host_copy;
699-
data->wrap = wrap;
691+
data->host.reset(host_copy);
700692
data->is_host = true;
701693

702-
uv_async_t* async_handle = &data->async_handle;
703-
CHECK_EQ(0, uv_async_init(wrap->env()->event_loop(),
704-
async_handle,
705-
CaresAsyncCb));
694+
wrap->QueueResponseCallback(status);
695+
}
696+
697+
void QueueResponseCallback(int status) {
698+
env()->SetImmediate([](Environment*, void* data) {
699+
static_cast<QueryWrap*>(data)->AfterResponse();
700+
}, this, object());
706701

707-
wrap->channel_->set_query_last_ok(status != ARES_ECONNREFUSED);
708-
async_handle->data = data;
709-
uv_async_send(async_handle);
702+
channel_->set_query_last_ok(status != ARES_ECONNREFUSED);
703+
channel_->ModifyActivityQueryCount(-1);
710704
}
711705

712706
void CallOnComplete(Local<Value> answer,
@@ -749,7 +743,11 @@ class QueryWrap : public AsyncWrap {
749743
ChannelWrap* channel_;
750744

751745
private:
746+
std::unique_ptr<ResponseData> response_data_;
752747
const char* trace_name_;
748+
// Pointer to pointer to 'this' that can be reset from the destructor,
749+
// in order to let Callback() know that 'this' no longer exists.
750+
QueryWrap** callback_ptr_ = nullptr;
753751
};
754752

755753

@@ -1768,7 +1766,7 @@ class GetHostByAddrWrap: public QueryWrap {
17681766
length,
17691767
family,
17701768
Callback,
1771-
static_cast<void*>(static_cast<QueryWrap*>(this)));
1769+
MakeCallbackPointer());
17721770
return 0;
17731771
}
17741772

Collapse file
+23Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
'use strict';
2+
const common = require('../common');
3+
const { Resolver } = require('dns');
4+
const dgram = require('dgram');
5+
const { Worker, isMainThread } = require('worker_threads');
6+
7+
// Test that Workers can terminate while DNS queries are outstanding.
8+
9+
if (isMainThread) {
10+
return new Worker(__filename);
11+
}
12+
13+
const socket = dgram.createSocket('udp4');
14+
15+
socket.bind(0, common.mustCall(() => {
16+
const resolver = new Resolver();
17+
resolver.setServers([`127.0.0.1:${socket.address().port}`]);
18+
resolver.resolve4('example.org', common.mustNotCall());
19+
}));
20+
21+
socket.on('message', common.mustCall(() => {
22+
process.exit();
23+
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.