From 6c593a38c19f1ee81190009bf5247f43294d42ea Mon Sep 17 00:00:00 2001 From: Otto Date: Fri, 29 Oct 2021 09:46:17 +0200 Subject: [PATCH 1/3] Move to a stream based socket for the control channel --- pdns/pdns_recursor.cc | 25 ++++---- pdns/rec_channel.cc | 131 ++++++++++++------------------------------ pdns/rec_channel.hh | 4 +- pdns/rec_control.cc | 4 +- 4 files changed, 54 insertions(+), 110 deletions(-) diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc index c449c1ed3f73..9f0e8e4d04b5 100644 --- a/pdns/pdns_recursor.cc +++ b/pdns/pdns_recursor.cc @@ -4206,22 +4206,20 @@ template ThreadTimes broadcastAccFunction(const boost::function& static void handleRCC(int fd, FDMultiplexer::funcparam_t& var) { + int clientfd; try { - string remote; - string msg = s_rcc.recv(&remote).d_str; + clientfd = accept(fd, nullptr, nullptr); + if (clientfd == -1) { + throw PDNSException("accept failed"); + } + string msg = s_rcc.recv(clientfd).d_str; + g_log << Logger::Info << "Received rec_control command '" << msg << "' via controlsocket" << endl; + RecursorControlParser rcp; RecursorControlParser::func_t* command; + auto answer = rcp.getAnswer(clientfd, msg, &command); - g_log << Logger::Info << "Received rec_control command '" << msg << "' from control socket" << endl; - auto answer = rcp.getAnswer(fd, msg, &command); - - // If we are inside a chroot, we need to strip - if (!arg()["chroot"].empty()) { - size_t len = arg()["chroot"].length(); - remote = remote.substr(len); - } - - s_rcc.send(answer, &remote); + s_rcc.send(clientfd, answer); command(); } catch(const std::exception& e) { @@ -4230,6 +4228,9 @@ static void handleRCC(int fd, FDMultiplexer::funcparam_t& var) catch(const PDNSException& ae) { g_log<& pid) diff --git a/pdns/rec_channel.cc b/pdns/rec_channel.cc index 15a1c954da30..4f8742022c1e 100644 --- a/pdns/rec_channel.cc +++ b/pdns/rec_channel.cc @@ -34,34 +34,9 @@ RecursorControlChannel::~RecursorControlChannel() unlink(d_local.sun_path); } -static void setSocketBuffer(int fd, int optname, uint32_t size) -{ - uint32_t psize = 0; - socklen_t len = sizeof(psize); - - if (getsockopt(fd, SOL_SOCKET, optname, (void*)&psize, &len)) - throw PDNSException("Unable to getsocket buffer size: " + stringerror()); - - if (psize > size) - return; - - // failure to raise is not fatal - (void)setsockopt(fd, SOL_SOCKET, optname, (const void*)&size, sizeof(size)); -} - -static void setSocketReceiveBuffer(int fd, uint32_t size) -{ - setSocketBuffer(fd, SO_RCVBUF, size); -} - -static void setSocketSendBuffer(int fd, uint32_t size) -{ - setSocketBuffer(fd, SO_SNDBUF, size); -} - int RecursorControlChannel::listen(const string& fname) { - d_fd = socket(AF_UNIX, SOCK_DGRAM, 0); + d_fd = socket(AF_UNIX, SOCK_STREAM, 0); setCloseOnExec(d_fd); if (d_fd < 0) @@ -80,11 +55,9 @@ int RecursorControlChannel::listen(const string& fname) if (bind(d_fd, (sockaddr*)&d_local, sizeof(d_local)) < 0) throw PDNSException("Unable to bind to controlsocket '" + fname + "': " + stringerror()); - - // receive buf should be size of max datagram plus address size - setSocketReceiveBuffer(d_fd, 60 * 1024); - setSocketSendBuffer(d_fd, 64 * 1024); - + if (::listen(d_fd, 0) == -1) { + throw PDNSException("Unable to listen on controlsocket '" + fname + "': " + stringerror()); + } return d_fd; } @@ -92,7 +65,7 @@ void RecursorControlChannel::connect(const string& path, const string& fname) { struct sockaddr_un remote; - d_fd = socket(AF_UNIX, SOCK_DGRAM, 0); + d_fd = socket(AF_UNIX, SOCK_STREAM, 0); setCloseOnExec(d_fd); if (d_fd < 0) @@ -103,24 +76,6 @@ void RecursorControlChannel::connect(const string& path, const string& fname) if (setsockopt(d_fd, SOL_SOCKET, SO_REUSEADDR, (char*)&tmp, sizeof tmp) < 0) throw PDNSException("Setsockopt failed: " + stringerror()); - string localname = path + "/lsockXXXXXX"; - *d_local.sun_path = 0; - if (makeUNsockaddr(localname, &d_local)) - throw PDNSException("Unable to bind to local temporary file, path '" + localname + "' is not a valid UNIX socket path."); - - if (mkstemp(d_local.sun_path) < 0) - throw PDNSException("Unable to generate local temporary file in directory '" + path + "': " + stringerror()); - - int err = unlink(d_local.sun_path); - if (err < 0 && errno != ENOENT) - throw PDNSException("Unable to remove local controlsocket: " + stringerror()); - - if (bind(d_fd, (sockaddr*)&d_local, sizeof(d_local)) < 0) - throw PDNSException("Unable to bind to local temporary file: " + stringerror()); - - if (chmod(d_local.sun_path, 0666) < 0) // make sure that pdns can reply! - throw PDNSException("Unable to chmod local temporary socket: " + stringerror()); - string remotename = path + "/" + fname; if (makeUNsockaddr(remotename, &remote)) throw PDNSException("Unable to connect to controlsocket, path '" + remotename + "' is not a valid UNIX socket path."); @@ -130,10 +85,6 @@ void RecursorControlChannel::connect(const string& path, const string& fname) unlink(d_local.sun_path); throw PDNSException("Unable to connect to remote '" + string(remote.sun_path) + "': " + stringerror()); } - - // receive buf should be size of max datagram plus address size - setSocketReceiveBuffer(d_fd, 60 * 1024); - setSocketSendBuffer(d_fd, 64 * 1024); } catch (...) { close(d_fd); @@ -143,7 +94,7 @@ void RecursorControlChannel::connect(const string& path, const string& fname) } } -static void sendfd(int s, int fd, const string* remote) +static void sendfd(int s, int fd) { struct msghdr msg; struct cmsghdr* cmsg; @@ -159,10 +110,6 @@ static void sendfd(int s, int fd, const string* remote) io_vector[0].iov_len = 1; memset(&msg, 0, sizeof(msg)); - if (remote) { - msg.msg_name = const_cast(remote->c_str()); - msg.msg_namelen = remote->length(); - } msg.msg_control = &cmsgbuf.buf; msg.msg_controllen = sizeof(cmsgbuf.buf); msg.msg_iov = io_vector; @@ -179,9 +126,9 @@ static void sendfd(int s, int fd, const string* remote) } } -void RecursorControlChannel::send(const Answer& msg, const std::string* remote, unsigned int timeout, int fd) +void RecursorControlChannel::send(int fd, const Answer& msg, unsigned int timeout, int fd_to_pass) { - int ret = waitForRWData(d_fd, false, timeout, 0); + int ret = waitForRWData(fd, false, timeout, 0); if (ret == 0) { throw PDNSException("Timeout sending message over control channel"); } @@ -189,52 +136,48 @@ void RecursorControlChannel::send(const Answer& msg, const std::string* remote, throw PDNSException("Error sending message over control channel:" + stringerror()); } - if (remote) { - struct sockaddr_un remoteaddr; - memset(&remoteaddr, 0, sizeof(remoteaddr)); - - remoteaddr.sun_family = AF_UNIX; - strncpy(remoteaddr.sun_path, remote->c_str(), sizeof(remoteaddr.sun_path) - 1); - remoteaddr.sun_path[sizeof(remoteaddr.sun_path) - 1] = '\0'; - - if (::sendto(d_fd, &msg.d_ret, sizeof(msg.d_ret), 0, (struct sockaddr*)&remoteaddr, sizeof(remoteaddr)) < 0) - throw PDNSException("Unable to send message over control channel '" + string(remoteaddr.sun_path) + "': " + stringerror()); - if (::sendto(d_fd, msg.d_str.c_str(), msg.d_str.length(), 0, (struct sockaddr*)&remoteaddr, sizeof(remoteaddr)) < 0) - throw PDNSException("Unable to send message over control channel '" + string(remoteaddr.sun_path) + "': " + stringerror()); + if (::send(fd, &msg.d_ret, sizeof(msg.d_ret), 0) < 0) { + throw PDNSException("Unable to send return code over control channel: " + stringerror()); } - else { - if (::send(d_fd, &msg.d_ret, sizeof(msg.d_ret), 0) < 0) - throw PDNSException("Unable to send message over control channel: " + stringerror()); - if (::send(d_fd, msg.d_str.c_str(), msg.d_str.length(), 0) < 0) - throw PDNSException("Unable to send message over control channel: " + stringerror()); + size_t len = msg.d_str.length(); + if (::send(fd, &len, sizeof(len), 0) < 0) { + throw PDNSException("Unable to send length over control channel: " + stringerror()); } - if (fd != -1) { - sendfd(d_fd, fd, remote); + if (::send(fd, msg.d_str.c_str(), len, 0) != static_cast(len)) { + throw PDNSException("Unable to send message over control channel: " + stringerror()); + } + + if (fd_to_pass != -1) { + sendfd(fd, fd_to_pass); } } -RecursorControlChannel::Answer RecursorControlChannel::recv(std::string* remote, unsigned int timeout) +RecursorControlChannel::Answer RecursorControlChannel::recv(int fd, unsigned int timeout) { - char buffer[16384]; - ssize_t len; - struct sockaddr_un remoteaddr; - socklen_t addrlen = sizeof(remoteaddr); - - int ret = waitForData(d_fd, timeout, 0); + int ret = waitForData(fd, timeout, 0); if (ret == 0) { throw PDNSException("Timeout waiting for answer from control channel"); } int err; - if (::recvfrom(d_fd, &err, sizeof(err), 0, (struct sockaddr*)&remoteaddr, &addrlen) != sizeof(err)) { - throw PDNSException("Unable to receive return status over control channel1: " + stringerror()); + if (::recv(fd, &err, sizeof(err), 0) != sizeof(err)) { + throw PDNSException("Unable to receive return status over control channel: " + stringerror()); } - if ((len = ::recvfrom(d_fd, buffer, sizeof(buffer), 0, (struct sockaddr*)&remoteaddr, &addrlen)) < 0) { - throw PDNSException("Unable to receive message over control channel2: " + stringerror()); + size_t len; + if (::recv(fd, &len, sizeof(len), 0) != sizeof(len)) { + throw PDNSException("Unable to receive length over control channel: " + stringerror()); } - if (remote) { - *remote = remoteaddr.sun_path; + string str; + str.reserve(len); + while (str.length() < len) { + char buffer[1024]; + ssize_t recvd = ::recv(fd, buffer, sizeof(buffer), 0); + if (recvd <= 0) { + // EOF means we have a length error + throw PDNSException("Unable to receive message over control channel: " + stringerror()); + } + str.append(buffer, recvd); } - return {err, string(buffer, buffer + len)}; + return {err, str}; } diff --git a/pdns/rec_channel.hh b/pdns/rec_channel.hh index f3c850a274cf..14d5e392bc4a 100644 --- a/pdns/rec_channel.hh +++ b/pdns/rec_channel.hh @@ -65,8 +65,8 @@ public: std::string d_str; }; - void send(const Answer&, const std::string* remote = nullptr, unsigned int timeout = 5, int fd = -1); - RecursorControlChannel::Answer recv(std::string* remote = nullptr, unsigned int timeout = 5); + void send(int remote, const Answer&, unsigned int timeout = 5, int fd_to_pass = -1); + RecursorControlChannel::Answer recv(int fd, unsigned int timeout = 5); int d_fd; static std::atomic stop; diff --git a/pdns/rec_control.cc b/pdns/rec_control.cc index 77ab3e246882..75d99c9813c6 100644 --- a/pdns/rec_control.cc +++ b/pdns/rec_control.cc @@ -176,9 +176,9 @@ int main(int argc, char** argv) auto timeout = arg().asNum("timeout"); RecursorControlChannel rccS; rccS.connect(arg()["socket-dir"], sockname); - rccS.send({0, command}, nullptr, timeout, fd); + rccS.send(rccS.d_fd, {0, command}, timeout, fd); - auto receive = rccS.recv(0, timeout); + auto receive = rccS.recv(rccS.d_fd, timeout); if (receive.d_ret != 0) { cerr << receive.d_str; } From ac3f1f35a03704284b3f71b0aa3d28448aefeb7a Mon Sep 17 00:00:00 2001 From: Otto Date: Fri, 29 Oct 2021 15:41:34 +0200 Subject: [PATCH 2/3] Use FDWrapper --- pdns/pdns_recursor.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc index 9f0e8e4d04b5..79e30ee9c018 100644 --- a/pdns/pdns_recursor.cc +++ b/pdns/pdns_recursor.cc @@ -4206,9 +4206,8 @@ template ThreadTimes broadcastAccFunction(const boost::function& static void handleRCC(int fd, FDMultiplexer::funcparam_t& var) { - int clientfd; try { - clientfd = accept(fd, nullptr, nullptr); + FDWrapper clientfd = accept(fd, nullptr, nullptr); if (clientfd == -1) { throw PDNSException("accept failed"); } @@ -4228,9 +4227,6 @@ static void handleRCC(int fd, FDMultiplexer::funcparam_t& var) catch(const PDNSException& ae) { g_log<& pid) From c25917cf32af4a82b38f6d925edc7793028c4c49 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 1 Nov 2021 08:28:18 +0100 Subject: [PATCH 3/3] Do not read further than the length we received, the string might be followed by a passed fd. Interesting to see that OpenBSD chops up recvs based on the sends, while Linux is happy to read more than was passed to the corresponding send call if another send was called after that. --- pdns/rec_channel.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pdns/rec_channel.cc b/pdns/rec_channel.cc index 4f8742022c1e..b288abe83259 100644 --- a/pdns/rec_channel.cc +++ b/pdns/rec_channel.cc @@ -171,7 +171,8 @@ RecursorControlChannel::Answer RecursorControlChannel::recv(int fd, unsigned int str.reserve(len); while (str.length() < len) { char buffer[1024]; - ssize_t recvd = ::recv(fd, buffer, sizeof(buffer), 0); + size_t toRead = std::min(len - str.length(), sizeof(buffer)); + ssize_t recvd = ::recv(fd, buffer, toRead, 0); if (recvd <= 0) { // EOF means we have a length error throw PDNSException("Unable to receive message over control channel: " + stringerror());