Skip to content

Commit 76203ba

Browse files
apolcynapolcyn
authored
[c-ares DNS resolver] Fix file descriptor use-after-close bug when c-ares writes succeed but subsequent read fails (#33871)
Normally, c-ares related fds are destroyed after all DNS resolution is finished in [this code path](https://github.com/grpc/grpc/blob/c82d31677aeea66c128a5b912ad87efcd5f74d67/src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc#L210). Also there are some fds that c-ares may fail to open or write to initially, and c-ares will close them internally before grpc ever knows about them. But if: 1) c-ares opens a socket and successfully writes a request on it 2) then a subsequent read fails Then c-ares will close the fd in [this code path](https://github.com/c-ares/c-ares/blob/bad62225b7f6b278b92e8e85a255600b629ef517/src/lib/ares_process.c#L740), but gRPC will have a reference on the fd and will still use it afterwards. Fix here is to leverage the c-ares socket-override API to properly track fd ownership between c-ares and grpc. Related: internal issue b/292203138
1 parent 98104bb commit 76203ba

12 files changed

Lines changed: 490 additions & 220 deletions

‎CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎build_autogenerated.yaml‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver_posix.cc‎

Lines changed: 101 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,28 +17,36 @@
1717
//
1818
#include <grpc/support/port_platform.h>
1919

20-
#include <memory>
21-
#include <string>
22-
23-
#include "absl/base/thread_annotations.h"
24-
25-
#include "src/core/lib/gprpp/sync.h"
26-
#include "src/core/lib/iomgr/closure.h"
27-
#include "src/core/lib/iomgr/error.h"
28-
#include "src/core/lib/iomgr/iomgr_fwd.h"
2920
#include "src/core/lib/iomgr/port.h"
21+
3022
#if GRPC_ARES == 1 && defined(GRPC_POSIX_SOCKET_ARES_EV_DRIVER)
3123

32-
#include <string.h>
24+
#include <ares_build.h>
3325
#include <sys/ioctl.h>
26+
#include <sys/socket.h>
27+
#include <sys/uio.h>
28+
#include <unistd.h>
29+
30+
#include <memory>
31+
#include <string>
32+
#include <unordered_set>
33+
#include <utility>
3434

3535
#include <ares.h>
3636

37+
#include "absl/base/thread_annotations.h"
3738
#include "absl/strings/str_cat.h"
3839

40+
#include <grpc/support/log.h>
41+
3942
#include "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_ev_driver.h"
4043
#include "src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h"
44+
#include "src/core/lib/gprpp/sync.h"
45+
#include "src/core/lib/iomgr/closure.h"
46+
#include "src/core/lib/iomgr/error.h"
4147
#include "src/core/lib/iomgr/ev_posix.h"
48+
#include "src/core/lib/iomgr/iomgr_fwd.h"
49+
#include "src/core/lib/iomgr/socket_utils_posix.h"
4250

4351
namespace grpc_core {
4452

@@ -98,12 +106,94 @@ class GrpcPolledFdPosix : public GrpcPolledFd {
98106

99107
class GrpcPolledFdFactoryPosix : public GrpcPolledFdFactory {
100108
public:
109+
~GrpcPolledFdFactoryPosix() override {
110+
for (auto& fd : owned_fds_) {
111+
close(fd);
112+
}
113+
}
114+
101115
GrpcPolledFd* NewGrpcPolledFdLocked(
102116
ares_socket_t as, grpc_pollset_set* driver_pollset_set) override {
117+
auto insert_result = owned_fds_.insert(as);
118+
GPR_ASSERT(insert_result.second);
103119
return new GrpcPolledFdPosix(as, driver_pollset_set);
104120
}
105121

106-
void ConfigureAresChannelLocked(ares_channel /*channel*/) override {}
122+
void ConfigureAresChannelLocked(ares_channel channel) override {
123+
ares_set_socket_functions(channel, &kSockFuncs, this);
124+
ares_set_socket_configure_callback(
125+
channel, &GrpcPolledFdFactoryPosix::ConfigureSocket, nullptr);
126+
}
127+
128+
private:
129+
/// Overridden socket API for c-ares
130+
static ares_socket_t Socket(int af, int type, int protocol,
131+
void* /*user_data*/) {
132+
return socket(af, type, protocol);
133+
}
134+
135+
/// Overridden connect API for c-ares
136+
static int Connect(ares_socket_t as, const struct sockaddr* target,
137+
ares_socklen_t target_len, void* /*user_data*/) {
138+
return connect(as, target, target_len);
139+
}
140+
141+
/// Overridden writev API for c-ares
142+
static ares_ssize_t WriteV(ares_socket_t as, const struct iovec* iov,
143+
int iovec_count, void* /*user_data*/) {
144+
return writev(as, iov, iovec_count);
145+
}
146+
147+
/// Overridden recvfrom API for c-ares
148+
static ares_ssize_t RecvFrom(ares_socket_t as, void* data, size_t data_len,
149+
int flags, struct sockaddr* from,
150+
ares_socklen_t* from_len, void* /*user_data*/) {
151+
return recvfrom(as, data, data_len, flags, from, from_len);
152+
}
153+
154+
/// Overridden close API for c-ares
155+
static int Close(ares_socket_t as, void* user_data) {
156+
GrpcPolledFdFactoryPosix* self =
157+
static_cast<GrpcPolledFdFactoryPosix*>(user_data);
158+
if (self->owned_fds_.find(as) == self->owned_fds_.end()) {
159+
// c-ares owns this fd, grpc has never seen it
160+
return close(as);
161+
}
162+
return 0;
163+
}
164+
165+
/// Because we're using socket API overrides, c-ares won't
166+
/// perform its typical configuration on the socket. See
167+
/// https://github.com/c-ares/c-ares/blob/bad62225b7f6b278b92e8e85a255600b629ef517/src/lib/ares_process.c#L1018.
168+
/// So we use the configure socket callback override and copy default
169+
/// settings that c-ares would normally apply on posix platforms:
170+
/// - non-blocking
171+
/// - cloexec flag
172+
/// - disable nagle */
173+
static int ConfigureSocket(ares_socket_t fd, int type, void* /*user_data*/) {
174+
grpc_error_handle err;
175+
err = grpc_set_socket_nonblocking(fd, true);
176+
if (!err.ok()) return -1;
177+
err = grpc_set_socket_cloexec(fd, true);
178+
if (!err.ok()) return -1;
179+
if (type == SOCK_STREAM) {
180+
err = grpc_set_socket_low_latency(fd, true);
181+
if (!err.ok()) return -1;
182+
}
183+
return 0;
184+
}
185+
186+
const struct ares_socket_functions kSockFuncs = {
187+
&GrpcPolledFdFactoryPosix::Socket /* socket */,
188+
&GrpcPolledFdFactoryPosix::Close /* close */,
189+
&GrpcPolledFdFactoryPosix::Connect /* connect */,
190+
&GrpcPolledFdFactoryPosix::RecvFrom /* recvfrom */,
191+
&GrpcPolledFdFactoryPosix::WriteV /* writev */,
192+
};
193+
194+
// fds that are used/owned by grpc - we (grpc) will close them rather than
195+
// c-ares
196+
std::unordered_set<ares_socket_t> owned_fds_;
107197
};
108198

109199
std::unique_ptr<GrpcPolledFdFactory> NewGrpcPolledFdFactory(Mutex* /* mu */) {

‎src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,8 @@ static void noop_inject_channel_config(ares_channel* /*channel*/) {}
515515
void (*grpc_ares_test_only_inject_config)(ares_channel* channel) =
516516
noop_inject_channel_config;
517517

518+
bool g_grpc_ares_test_only_force_tcp = false;
519+
518520
grpc_error_handle grpc_ares_ev_driver_create_locked(
519521
grpc_ares_ev_driver** ev_driver, grpc_pollset_set* pollset_set,
520522
int query_timeout_ms, grpc_ares_request* request)
@@ -523,6 +525,9 @@ grpc_error_handle grpc_ares_ev_driver_create_locked(
523525
ares_options opts;
524526
memset(&opts, 0, sizeof(opts));
525527
opts.flags |= ARES_FLAG_STAYOPEN;
528+
if (g_grpc_ares_test_only_force_tcp) {
529+
opts.flags |= ARES_FLAG_USEVC;
530+
}
526531
int status = ares_init_options(&(*ev_driver)->channel, &opts, ARES_OPT_FLAGS);
527532
grpc_ares_test_only_inject_config(&(*ev_driver)->channel);
528533
GRPC_CARES_TRACE_LOG("request:%p grpc_ares_ev_driver_create_locked", request);

‎src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,4 +133,7 @@ void grpc_cares_wrapper_address_sorting_sort(
133133
// Exposed in this header for C-core tests only
134134
extern void (*grpc_ares_test_only_inject_config)(ares_channel* channel);
135135

136+
// Exposed in this header for C-core tests only
137+
extern bool g_grpc_ares_test_only_force_tcp;
138+
136139
#endif // GRPC_SRC_CORE_EXT_FILTERS_CLIENT_CHANNEL_RESOLVER_DNS_C_ARES_GRPC_ARES_WRAPPER_H

‎test/core/util/BUILD‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,34 @@ grpc_cc_library(
363363
],
364364
)
365365

366+
grpc_cc_library(
367+
name = "socket_use_after_close_detector",
368+
srcs = ["socket_use_after_close_detector.cc"],
369+
hdrs = ["socket_use_after_close_detector.h"],
370+
external_deps = ["gtest"],
371+
language = "C++",
372+
deps = [
373+
"grpc_test_util",
374+
"//:gpr",
375+
"//:grpc",
376+
"//src/core:grpc_sockaddr",
377+
],
378+
)
379+
380+
grpc_cc_library(
381+
name = "socket_use_after_close_detector_unsecure",
382+
srcs = ["socket_use_after_close_detector.cc"],
383+
hdrs = ["socket_use_after_close_detector.h"],
384+
external_deps = ["gtest"],
385+
language = "C++",
386+
deps = [
387+
"grpc_test_util_unsecure",
388+
"//:gpr",
389+
"//:grpc",
390+
"//src/core:grpc_sockaddr",
391+
],
392+
)
393+
366394
grpc_cc_library(
367395
name = "build",
368396
srcs = ["build.cc"],

0 commit comments

Comments
 (0)