Fix potential fd leak in FwmarkServer.
There's a subtle bug in the previous use of cmsg to receive file
descriptors: on 64-bit, CMSG_SPACE rounds up to the nearest
alignof(cmsghdr), and cmsghdr contains a size_t, so
CMSG_SPACE(sizeof(int)) is the same as CMSG_SPACE(2 * sizeof(int)).
This means it's possible for us to receive *two* file descriptors in a
single recvmsg call. We check that cmsghdr::cmsg_len equals
CMSG_LEN(sizeof(int)), but when it doesn't because we received two fds,
we treat it as if we received none, and leak the fds we received.
Switch to android::base::ReceiveFileDescriptorVector, which handles this
case properly.
Bug: http://b/122047630
Test: atest bpf_module_test clatd_test libbpf_android_test libnetdbpf_test
netd_integration_test netd_unit_test netdutils_test resolv_integration_test
resolv_unit_test
Change-Id: I58b7fa1e4c35973a68d12a8983574d5798d1a64b
diff --git a/server/FwmarkServer.cpp b/server/FwmarkServer.cpp
index 51d5398..bf7fe95 100644
--- a/server/FwmarkServer.cpp
+++ b/server/FwmarkServer.cpp
@@ -22,6 +22,8 @@
#include <unistd.h>
#include <utils/String16.h>
+#include <android-base/cmsg.h>
+#include <android-base/logging.h>
#include <binder/IServiceManager.h>
#include <netd_resolv/resolv.h> // NETID_UNSET
@@ -32,6 +34,8 @@
#include "TrafficController.h"
using android::String16;
+using android::base::ReceiveFileDescriptorVector;
+using android::base::unique_fd;
using android::net::metrics::INetdEventListener;
namespace android {
@@ -85,29 +89,20 @@
FwmarkCommand command;
FwmarkConnectInfo connectInfo;
- iovec iov[2] = {
- { &command, sizeof(command) },
- { &connectInfo, sizeof(connectInfo) },
- };
- msghdr message;
- memset(&message, 0, sizeof(message));
- message.msg_iov = iov;
- message.msg_iovlen = ARRAY_SIZE(iov);
+ char buf[sizeof(command) + sizeof(connectInfo)];
+ std::vector<unique_fd> received_fds;
+ ssize_t messageLength =
+ ReceiveFileDescriptorVector(client->getSocket(), buf, sizeof(buf), 1, &received_fds);
- union {
- cmsghdr cmh;
- char cmsg[CMSG_SPACE(sizeof(*socketFd))];
- } cmsgu;
-
- memset(cmsgu.cmsg, 0, sizeof(cmsgu.cmsg));
- message.msg_control = cmsgu.cmsg;
- message.msg_controllen = sizeof(cmsgu.cmsg);
-
- int messageLength = TEMP_FAILURE_RETRY(recvmsg(client->getSocket(), &message, MSG_CMSG_CLOEXEC));
- if (messageLength <= 0) {
+ if (messageLength < 0) {
return -errno;
+ } else if (messageLength == 0) {
+ return -ESHUTDOWN;
}
+ memcpy(&command, buf, sizeof(command));
+ memcpy(&connectInfo, buf + sizeof(command), sizeof(connectInfo));
+
if (!((command.cmdId != FwmarkCommand::ON_CONNECT_COMPLETE && messageLength == sizeof(command))
|| (command.cmdId == FwmarkCommand::ON_CONNECT_COMPLETE
&& messageLength == sizeof(command) + sizeof(connectInfo)))) {
@@ -131,16 +126,16 @@
return mTrafficCtrl->deleteTagData(command.trafficCtrlInfo, command.uid, client->getUid());
}
- cmsghdr* const cmsgh = CMSG_FIRSTHDR(&message);
- if (cmsgh && cmsgh->cmsg_level == SOL_SOCKET && cmsgh->cmsg_type == SCM_RIGHTS &&
- cmsgh->cmsg_len == CMSG_LEN(sizeof(*socketFd))) {
- memcpy(socketFd, CMSG_DATA(cmsgh), sizeof(*socketFd));
- }
-
- if (*socketFd < 0) {
+ if (received_fds.size() != 1) {
+ LOG(ERROR) << "FwmarkServer received " << received_fds.size() << " fds from client?";
+ return -EBADF;
+ } else if (received_fds[0] < 0) {
+ LOG(ERROR) << "FwmarkServer received fd -1 from ReceiveFileDescriptorVector?";
return -EBADF;
}
+ *socketFd = received_fds[0].release();
+
int family;
socklen_t familyLen = sizeof(family);
if (getsockopt(*socketFd, SOL_SOCKET, SO_DOMAIN, &family, &familyLen) == -1) {