From 14f9500a35390e2e0252ccc1c03c9c850444e2b6 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 7 Jan 2019 19:16:21 -0800 Subject: [PATCH 1/3] base: add helpers for sending/receiving file descriptors. Almost all of the uses of cmsg(3) in our source tree are wrong in at least one of the following ways: - not aligning the cmsg buffer - leaking fds if more fds are received than expected - blindly dereferencing CMSG_DATA without checking the header - using CMSG_SPACE instead of CMSG_LEN for .cmsg_len - using CMSG_LEN instead of CMSG_SPACE for .msg_controllen - using a length specified in number of fds instead of bytes Implement wrapper functions that implement this dumpster fire of an API correctly. Bug: http://b/122047630 Test: libbase_test32 Change-Id: I6ac34d67bbbf1bfa9727ab598248fc178ea19df9 --- base/Android.bp | 5 + base/cmsg.cpp | 151 ++++++++++++++++++ base/cmsg_test.cpp | 202 ++++++++++++++++++++++++ base/include/android-base/cmsg.h | 106 +++++++++++++ base/include/android-base/collections.h | 60 +++++++ 5 files changed, 524 insertions(+) create mode 100644 base/cmsg.cpp create mode 100644 base/cmsg_test.cpp create mode 100644 base/include/android-base/cmsg.h create mode 100644 base/include/android-base/collections.h diff --git a/base/Android.bp b/base/Android.bp index b0181aa6a..38f301a1b 100644 --- a/base/Android.bp +++ b/base/Android.bp @@ -46,6 +46,7 @@ cc_defaults { defaults: ["libbase_cflags_defaults"], srcs: [ "chrono_utils.cpp", + "cmsg.cpp", "file.cpp", "logging.cpp", "mapped_file.cpp", @@ -85,6 +86,9 @@ cc_defaults { "errors_windows.cpp", "utf8.cpp", ], + exclude_srcs: [ + "cmsg.cpp", + ], enabled: true, }, }, @@ -121,6 +125,7 @@ cc_test { defaults: ["libbase_cflags_defaults"], host_supported: true, srcs: [ + "cmsg_test.cpp", "endian_test.cpp", "errors_test.cpp", "file_test.cpp", diff --git a/base/cmsg.cpp b/base/cmsg.cpp new file mode 100644 index 000000000..1879d4473 --- /dev/null +++ b/base/cmsg.cpp @@ -0,0 +1,151 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include +#include +#include +#include +#include + +#include + +#include + +namespace android { +namespace base { + +ssize_t SendFileDescriptorVector(int sockfd, const void* data, size_t len, + const std::vector& fds) { + size_t cmsg_space = CMSG_SPACE(sizeof(int) * fds.size()); + size_t cmsg_len = CMSG_LEN(sizeof(int) * fds.size()); + if (cmsg_space >= PAGE_SIZE) { + errno = ENOMEM; + return -1; + } + + alignas(struct cmsghdr) char cmsg_buf[cmsg_space]; + iovec iov = {.iov_base = const_cast(data), .iov_len = len}; + msghdr msg = { + .msg_name = nullptr, + .msg_namelen = 0, + .msg_iov = &iov, + .msg_iovlen = 1, + .msg_control = cmsg_buf, + .msg_controllen = cmsg_space, + .msg_flags = 0, + }; + + struct cmsghdr* cmsg = CMSG_FIRSTHDR(&msg); + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + cmsg->cmsg_len = cmsg_len; + + int* cmsg_fds = reinterpret_cast(CMSG_DATA(cmsg)); + for (size_t i = 0; i < fds.size(); ++i) { + cmsg_fds[i] = fds[i]; + } + + return TEMP_FAILURE_RETRY(sendmsg(sockfd, &msg, MSG_NOSIGNAL)); +} + +ssize_t ReceiveFileDescriptorVector(int sockfd, void* data, size_t len, size_t max_fds, + std::vector* fds) { + fds->clear(); + + size_t cmsg_space = CMSG_SPACE(sizeof(int) * max_fds); + if (cmsg_space >= PAGE_SIZE) { + errno = ENOMEM; + return -1; + } + + alignas(struct cmsghdr) char cmsg_buf[cmsg_space]; + iovec iov = {.iov_base = const_cast(data), .iov_len = len}; + msghdr msg = { + .msg_name = nullptr, + .msg_namelen = 0, + .msg_iov = &iov, + .msg_iovlen = 1, + .msg_control = cmsg_buf, + .msg_controllen = cmsg_space, + .msg_flags = 0, + }; + + ssize_t rc = TEMP_FAILURE_RETRY( + recvmsg(sockfd, &msg, MSG_TRUNC | MSG_CTRUNC | MSG_CMSG_CLOEXEC | MSG_NOSIGNAL)); + if (rc == -1) { + return -1; + } + + int error = 0; + if ((msg.msg_flags & MSG_TRUNC)) { + LOG(ERROR) << "message was truncated when receiving file descriptors"; + error = EMSGSIZE; + } else if ((msg.msg_flags & MSG_CTRUNC)) { + LOG(ERROR) << "control message was truncated when receiving file descriptors"; + error = EMSGSIZE; + } + + std::vector received_fds; + struct cmsghdr* cmsg; + for (cmsg = CMSG_FIRSTHDR(&msg); cmsg != nullptr; cmsg = CMSG_NXTHDR(&msg, cmsg)) { + if (cmsg->cmsg_level != SOL_SOCKET || cmsg->cmsg_type != SCM_RIGHTS) { + LOG(ERROR) << "received unexpected cmsg: [" << cmsg->cmsg_level << ", " << cmsg->cmsg_type + << "]"; + error = EBADMSG; + continue; + } + + // There isn't a macro that does the inverse of CMSG_LEN, so hack around it ourselves, with + // some static asserts to ensure that CMSG_LEN behaves as we expect. + static_assert(CMSG_LEN(0) + 1 * sizeof(int) == CMSG_LEN(1 * sizeof(int))); + static_assert(CMSG_LEN(0) + 2 * sizeof(int) == CMSG_LEN(2 * sizeof(int))); + static_assert(CMSG_LEN(0) + 3 * sizeof(int) == CMSG_LEN(3 * sizeof(int))); + static_assert(CMSG_LEN(0) + 4 * sizeof(int) == CMSG_LEN(4 * sizeof(int))); + + if (cmsg->cmsg_len % sizeof(int) != 0) { + LOG(FATAL) << "cmsg_len(" << cmsg->cmsg_len << ") not aligned to sizeof(int)"; + } else if (cmsg->cmsg_len <= CMSG_LEN(0)) { + LOG(FATAL) << "cmsg_len(" << cmsg->cmsg_len << ") not long enough to hold any data"; + } + + int* cmsg_fds = reinterpret_cast(CMSG_DATA(cmsg)); + size_t cmsg_fdcount = static_cast(cmsg->cmsg_len - CMSG_LEN(0)) / sizeof(int); + for (size_t i = 0; i < cmsg_fdcount; ++i) { + received_fds.emplace_back(cmsg_fds[i]); + } + } + + if (error != 0) { + errno = error; + return -1; + } + + if (received_fds.size() > max_fds) { + LOG(ERROR) << "received too many file descriptors, expected " << fds->size() << ", received " + << received_fds.size(); + errno = EMSGSIZE; + return -1; + } + + *fds = std::move(received_fds); + return rc; +} + +} // namespace base +} // namespace android diff --git a/base/cmsg_test.cpp b/base/cmsg_test.cpp new file mode 100644 index 000000000..9ee5c8253 --- /dev/null +++ b/base/cmsg_test.cpp @@ -0,0 +1,202 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include +#include +#include + +#if !defined(_WIN32) + +using android::base::ReceiveFileDescriptors; +using android::base::SendFileDescriptors; +using android::base::unique_fd; + +static ino_t GetInode(int fd) { + struct stat st; + if (fstat(fd, &st) != 0) { + PLOG(FATAL) << "fstat failed"; + } + + return st.st_ino; +} + +struct CmsgTest : ::testing::TestWithParam { + bool Seqpacket() { return GetParam(); } + + void SetUp() override { + ASSERT_TRUE( + android::base::Socketpair(Seqpacket() ? SOCK_SEQPACKET : SOCK_STREAM, &send, &recv)); + int dup1 = dup(tmp1.fd); + ASSERT_NE(-1, dup1); + int dup2 = dup(tmp2.fd); + ASSERT_NE(-1, dup2); + + fd1.reset(dup1); + fd2.reset(dup2); + + ino1 = GetInode(dup1); + ino2 = GetInode(dup2); + } + + unique_fd send; + unique_fd recv; + + TemporaryFile tmp1; + TemporaryFile tmp2; + + unique_fd fd1; + unique_fd fd2; + + ino_t ino1; + ino_t ino2; +}; + +TEST_P(CmsgTest, smoke) { + ASSERT_EQ(1, SendFileDescriptors(send.get(), "x", 1, fd1.get())); + + char buf[2]; + unique_fd received; + ASSERT_EQ(1, ReceiveFileDescriptors(recv.get(), buf, 2, &received)); + ASSERT_EQ('x', buf[0]); + ASSERT_NE(-1, received.get()); + + ASSERT_EQ(ino1, GetInode(received.get())); +} + +TEST_P(CmsgTest, msg_trunc) { + ASSERT_EQ(2, SendFileDescriptors(send.get(), "ab", 2, fd1.get(), fd2.get())); + + char buf[2]; + unique_fd received1, received2; + + ssize_t rc = ReceiveFileDescriptors(recv.get(), buf, 1, &received1, &received2); + if (Seqpacket()) { + ASSERT_EQ(-1, rc); + ASSERT_EQ(EMSGSIZE, errno); + ASSERT_EQ(-1, received1.get()); + ASSERT_EQ(-1, received2.get()); + } else { + ASSERT_EQ(1, rc); + ASSERT_NE(-1, received1.get()); + ASSERT_NE(-1, received2.get()); + ASSERT_EQ(ino1, GetInode(received1.get())); + ASSERT_EQ(ino2, GetInode(received2.get())); + ASSERT_EQ(1, read(recv.get(), buf, 2)); + } +} + +TEST_P(CmsgTest, msg_ctrunc) { + ASSERT_EQ(1, SendFileDescriptors(send.get(), "a", 1, fd1.get(), fd2.get())); + + char buf[2]; + unique_fd received; + ASSERT_EQ(-1, ReceiveFileDescriptors(recv.get(), buf, 1, &received)); + ASSERT_EQ(EMSGSIZE, errno); + ASSERT_EQ(-1, received.get()); +} + +TEST_P(CmsgTest, peek) { + ASSERT_EQ(1, SendFileDescriptors(send.get(), "a", 1, fd1.get())); + + char buf[2]; + ASSERT_EQ(1, ::recv(recv.get(), buf, sizeof(buf), MSG_PEEK)); + ASSERT_EQ('a', buf[0]); + + unique_fd received; + ASSERT_EQ(1, ReceiveFileDescriptors(recv.get(), buf, 1, &received)); + ASSERT_EQ(ino1, GetInode(received.get())); +} + +TEST_P(CmsgTest, stream_fd_association) { + if (Seqpacket()) { + return; + } + + // fds are associated with the first byte of the write. + ASSERT_EQ(1, TEMP_FAILURE_RETRY(write(send.get(), "a", 1))); + ASSERT_EQ(2, SendFileDescriptors(send.get(), "bc", 2, fd1.get())); + ASSERT_EQ(1, SendFileDescriptors(send.get(), "d", 1, fd2.get())); + char buf[2]; + ASSERT_EQ(2, TEMP_FAILURE_RETRY(read(recv.get(), buf, 2))); + ASSERT_EQ(0, memcmp(buf, "ab", 2)); + + std::vector received1; + ssize_t rc = ReceiveFileDescriptorVector(recv.get(), buf, 1, 1, &received1); + ASSERT_EQ(1, rc); + ASSERT_EQ('c', buf[0]); + ASSERT_TRUE(received1.empty()); + + unique_fd received2; + rc = ReceiveFileDescriptors(recv.get(), buf, 1, &received2); + ASSERT_EQ(1, rc); + ASSERT_EQ('d', buf[0]); + ASSERT_EQ(ino2, GetInode(received2.get())); +} + +TEST_P(CmsgTest, multiple_fd_ordering) { + ASSERT_EQ(1, SendFileDescriptors(send.get(), "a", 1, fd1.get(), fd2.get())); + + char buf[2]; + unique_fd received1, received2; + ASSERT_EQ(1, ReceiveFileDescriptors(recv.get(), buf, 1, &received1, &received2)); + + ASSERT_NE(-1, received1.get()); + ASSERT_NE(-1, received2.get()); + + ASSERT_EQ(ino1, GetInode(received1.get())); + ASSERT_EQ(ino2, GetInode(received2.get())); +} + +TEST_P(CmsgTest, separate_fd_ordering) { + ASSERT_EQ(1, SendFileDescriptors(send.get(), "a", 1, fd1.get())); + ASSERT_EQ(1, SendFileDescriptors(send.get(), "b", 1, fd2.get())); + + char buf[2]; + unique_fd received1, received2; + ASSERT_EQ(1, ReceiveFileDescriptors(recv.get(), buf, 1, &received1)); + ASSERT_EQ(1, ReceiveFileDescriptors(recv.get(), buf, 1, &received2)); + + ASSERT_NE(-1, received1.get()); + ASSERT_NE(-1, received2.get()); + + ASSERT_EQ(ino1, GetInode(received1.get())); + ASSERT_EQ(ino2, GetInode(received2.get())); +} + +TEST_P(CmsgTest, separate_fds_no_coalescing) { + unique_fd sent1(dup(tmp1.fd)); + unique_fd sent2(dup(tmp2.fd)); + + ASSERT_EQ(1, SendFileDescriptors(send.get(), "", 1, fd1.get())); + ASSERT_EQ(1, SendFileDescriptors(send.get(), "", 1, fd2.get())); + + char buf[2]; + std::vector received; + ASSERT_EQ(1, ReceiveFileDescriptorVector(recv.get(), buf, 2, 2, &received)); + ASSERT_EQ(1U, received.size()); + ASSERT_EQ(ino1, GetInode(received[0].get())); + + ASSERT_EQ(1, ReceiveFileDescriptorVector(recv.get(), buf, 2, 2, &received)); + ASSERT_EQ(1U, received.size()); + ASSERT_EQ(ino2, GetInode(received[0].get())); +} + +INSTANTIATE_TEST_CASE_P(CmsgTest, CmsgTest, testing::Bool()); + +#endif diff --git a/base/include/android-base/cmsg.h b/base/include/android-base/cmsg.h new file mode 100644 index 000000000..7f93ddc3b --- /dev/null +++ b/base/include/android-base/cmsg.h @@ -0,0 +1,106 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include + +#include +#include + +#include +#include +#include + +namespace android { +namespace base { + +#if !defined(_WIN32) + +// Helpers for sending and receiving file descriptors across Unix domain sockets. +// +// The cmsg(3) API is very hard to get right, with multiple landmines that can +// lead to death. Almost all of the uses of cmsg in Android make at least one of +// the following mistakes: +// +// - not aligning the cmsg buffer +// - leaking fds if more fds are received than expected +// - blindly dereferencing CMSG_DATA without checking the header +// - using CMSG_SPACE instead of CMSG_LEN for .cmsg_len +// - using CMSG_LEN instead of CMSG_SPACE for .msg_controllen +// - using a length specified in number of fds instead of bytes +// +// These functions wrap the hard-to-use cmsg API with an easier to use abstraction. + +// Send file descriptors across a Unix domain socket. +// +// Note that the write can return short if the socket type is SOCK_STREAM. When +// this happens, file descriptors are still sent to the other end, but with +// truncated data. For this reason, using SOCK_SEQPACKET or SOCK_DGRAM is recommended. +ssize_t SendFileDescriptorVector(int sock, const void* data, size_t len, + const std::vector& fds); + +// Receive file descriptors from a Unix domain socket. +// +// If more FDs (or bytes, for datagram sockets) are received than expected, +// -1 is returned with errno set to EMSGSIZE, and all received FDs are thrown away. +ssize_t ReceiveFileDescriptorVector(int sock, void* data, size_t len, size_t max_fds, + std::vector* fds); + +// Helper for SendFileDescriptorVector that constructs a std::vector for you, e.g.: +// SendFileDescriptors(sock, "foo", 3, std::move(fd1), std::move(fd2)) +template +ssize_t SendFileDescriptors(int sock, const void* data, size_t len, Args&&... sent_fds) { + // Do not allow implicit conversion to int: people might try to do something along the lines of: + // SendFileDescriptors(..., std::move(a_unique_fd)) + // and be surprised when the unique_fd isn't closed afterwards. + AssertType(std::forward(sent_fds)...); + std::vector fds; + Append(fds, std::forward(sent_fds)...); + return SendFileDescriptorVector(sock, data, len, fds); +} + +// Helper for ReceiveFileDescriptorVector that receives an exact number of file descriptors. +// If more file descriptors are received than requested, -1 is returned with errno set to EMSGSIZE. +// If fewer file descriptors are received than requested, -1 is returned with errno set to ENOMSG. +// In both cases, all arguments are cleared and any received FDs are thrown away. +template +ssize_t ReceiveFileDescriptors(int sock, void* data, size_t len, Args&&... received_fds) { + std::vector fds; + Append(fds, std::forward(received_fds)...); + + std::vector result; + ssize_t rc = ReceiveFileDescriptorVector(sock, data, len, fds.size(), &result); + if (rc == -1 || result.size() != fds.size()) { + int err = rc == -1 ? errno : ENOMSG; + for (unique_fd* fd : fds) { + fd->reset(); + } + errno = err; + return -1; + } + + for (size_t i = 0; i < fds.size(); ++i) { + *fds[i] = std::move(result[i]); + } + return rc; +} + +#endif + +} // namespace base +} // namespace android diff --git a/base/include/android-base/collections.h b/base/include/android-base/collections.h new file mode 100644 index 000000000..be0683ab9 --- /dev/null +++ b/base/include/android-base/collections.h @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +namespace android { +namespace base { + +// Helpers for converting a variadic template parameter pack to a homogeneous collection. +// Parameters must be implictly convertible to the contained type (including via move/copy ctors). +// +// Use as follows: +// +// template +// std::vector CreateVector(Args&&... args) { +// std::vector result; +// Append(result, std::forward(args)...); +// return result; +// } +template +void Append(CollectionType& collection, T&& arg) { + collection.push_back(std::forward(arg)); +} + +template +void Append(CollectionType& collection, T&& arg, Args&&... args) { + collection.push_back(std::forward(arg)); + return Append(collection, std::forward(args)...); +} + +// Assert that all of the arguments in a variadic template parameter pack are of a given type +// after std::decay. +template +void AssertType(Arg&&) { + static_assert(std::is_same::type>::value); +} + +template +void AssertType(Arg&&, Args&&... args) { + static_assert(std::is_same::type>::value); + AssertType(std::forward(args)...); +} + +} // namespace base +} // namespace android From 5f87bbdb0afebac6ad46a849e94460dbd0c41014 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 9 Jan 2019 17:01:49 -0800 Subject: [PATCH 2/3] debuggerd: switch to base::{Send,Receive}FileDescriptors. Bug: http://b/12204763 Test: debuggerd_test Change-Id: I0be40916214de51ab36fd6bd6d44090a84312e51 --- debuggerd/client/debuggerd_client.cpp | 9 ++- debuggerd/debuggerd_test.cpp | 9 ++- debuggerd/tombstoned/intercept_manager.cpp | 5 +- debuggerd/tombstoned/tombstoned.cpp | 7 ++- debuggerd/tombstoned/tombstoned_client.cpp | 4 +- debuggerd/util.cpp | 64 ---------------------- debuggerd/util.h | 24 -------- 7 files changed, 26 insertions(+), 96 deletions(-) diff --git a/debuggerd/client/debuggerd_client.cpp b/debuggerd/client/debuggerd_client.cpp index 610b96b22..60eb24131 100644 --- a/debuggerd/client/debuggerd_client.cpp +++ b/debuggerd/client/debuggerd_client.cpp @@ -26,6 +26,7 @@ #include +#include #include #include #include @@ -41,6 +42,7 @@ using namespace std::chrono_literals; +using android::base::SendFileDescriptors; using android::base::unique_fd; static bool send_signal(pid_t pid, const DebuggerdDumpType dump_type) { @@ -146,15 +148,16 @@ bool debuggerd_trigger_dump(pid_t tid, DebuggerdDumpType dump_type, unsigned int PLOG(ERROR) << "failed to set pipe buffer size"; } - if (send_fd(set_timeout(sockfd), &req, sizeof(req), std::move(pipe_write)) != sizeof(req)) { + ssize_t rc = SendFileDescriptors(set_timeout(sockfd), &req, sizeof(req), pipe_write.get()); + pipe_write.reset(); + if (rc != sizeof(req)) { PLOG(ERROR) << "libdebuggerd_client: failed to send output fd to tombstoned"; return false; } // Check to make sure we've successfully registered. InterceptResponse response; - ssize_t rc = - TEMP_FAILURE_RETRY(recv(set_timeout(sockfd.get()), &response, sizeof(response), MSG_TRUNC)); + rc = TEMP_FAILURE_RETRY(recv(set_timeout(sockfd.get()), &response, sizeof(response), MSG_TRUNC)); if (rc == 0) { LOG(ERROR) << "libdebuggerd_client: failed to read initial response from tombstoned: " << "timeout reached?"; diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index bea8b43ca..64df53e9e 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -32,6 +32,7 @@ #include #include +#include #include #include #include @@ -53,6 +54,8 @@ #include "util.h" using namespace std::chrono_literals; + +using android::base::SendFileDescriptors; using android::base::unique_fd; #if defined(__LP64__) @@ -123,12 +126,14 @@ static void tombstoned_intercept(pid_t target_pid, unique_fd* intercept_fd, uniq ASSERT_GE(pipe_buffer_size, 1024 * 1024); - if (send_fd(intercept_fd->get(), &req, sizeof(req), std::move(output_pipe_write)) != sizeof(req)) { + ssize_t rc = SendFileDescriptors(intercept_fd->get(), &req, sizeof(req), output_pipe_write.get()); + output_pipe_write.reset(); + if (rc != sizeof(req)) { FAIL() << "failed to send output fd to tombstoned: " << strerror(errno); } InterceptResponse response; - ssize_t rc = TEMP_FAILURE_RETRY(read(intercept_fd->get(), &response, sizeof(response))); + rc = TEMP_FAILURE_RETRY(read(intercept_fd->get(), &response, sizeof(response))); if (rc == -1) { FAIL() << "failed to read response from tombstoned: " << strerror(errno); } else if (rc == 0) { diff --git a/debuggerd/tombstoned/intercept_manager.cpp b/debuggerd/tombstoned/intercept_manager.cpp index c446dbba8..7d25c50a8 100644 --- a/debuggerd/tombstoned/intercept_manager.cpp +++ b/debuggerd/tombstoned/intercept_manager.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -31,6 +32,7 @@ #include "protocol.h" #include "util.h" +using android::base::ReceiveFileDescriptors; using android::base::unique_fd; static void intercept_close_cb(evutil_socket_t sockfd, short event, void* arg) { @@ -96,7 +98,8 @@ static void intercept_request_cb(evutil_socket_t sockfd, short ev, void* arg) { { unique_fd rcv_fd; InterceptRequest intercept_request; - ssize_t result = recv_fd(sockfd, &intercept_request, sizeof(intercept_request), &rcv_fd); + ssize_t result = + ReceiveFileDescriptors(sockfd, &intercept_request, sizeof(intercept_request), &rcv_fd); if (result == -1) { PLOG(WARNING) << "failed to read from intercept socket"; diff --git a/debuggerd/tombstoned/tombstoned.cpp b/debuggerd/tombstoned/tombstoned.cpp index ad9206702..bbeb181c3 100644 --- a/debuggerd/tombstoned/tombstoned.cpp +++ b/debuggerd/tombstoned/tombstoned.cpp @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -45,6 +46,7 @@ #include "intercept_manager.h" using android::base::GetIntProperty; +using android::base::SendFileDescriptors; using android::base::StringPrintf; using android::base::unique_fd; @@ -224,7 +226,10 @@ static void perform_request(Crash* crash) { TombstonedCrashPacket response = { .packet_type = CrashPacketType::kPerformDump }; - ssize_t rc = send_fd(crash->crash_socket_fd, &response, sizeof(response), std::move(output_fd)); + ssize_t rc = + SendFileDescriptors(crash->crash_socket_fd, &response, sizeof(response), output_fd.get()); + output_fd.reset(); + if (rc == -1) { PLOG(WARNING) << "failed to send response to CrashRequest"; goto fail; diff --git a/debuggerd/tombstoned/tombstoned_client.cpp b/debuggerd/tombstoned/tombstoned_client.cpp index bdb4c1a6f..2c23c98f3 100644 --- a/debuggerd/tombstoned/tombstoned_client.cpp +++ b/debuggerd/tombstoned/tombstoned_client.cpp @@ -21,6 +21,7 @@ #include +#include #include #include #include @@ -28,6 +29,7 @@ #include "protocol.h" #include "util.h" +using android::base::ReceiveFileDescriptors; using android::base::unique_fd; bool tombstoned_connect(pid_t pid, unique_fd* tombstoned_socket, unique_fd* output_fd, @@ -53,7 +55,7 @@ bool tombstoned_connect(pid_t pid, unique_fd* tombstoned_socket, unique_fd* outp } unique_fd tmp_output_fd; - ssize_t rc = recv_fd(sockfd, &packet, sizeof(packet), &tmp_output_fd); + ssize_t rc = ReceiveFileDescriptors(sockfd, &packet, sizeof(packet), &tmp_output_fd); if (rc == -1) { async_safe_format_log(ANDROID_LOG_ERROR, "libc", "failed to read response to DumpRequest packet: %s", strerror(errno)); diff --git a/debuggerd/util.cpp b/debuggerd/util.cpp index 50c5efc99..a37b3b93b 100644 --- a/debuggerd/util.cpp +++ b/debuggerd/util.cpp @@ -24,73 +24,9 @@ #include #include #include -#include #include #include "protocol.h" -using android::base::unique_fd; - -ssize_t send_fd(int sockfd, const void* data, size_t len, unique_fd fd) { - char cmsg_buf[CMSG_SPACE(sizeof(int))]; - - iovec iov = { .iov_base = const_cast(data), .iov_len = len }; - msghdr msg = { - .msg_iov = &iov, .msg_iovlen = 1, .msg_control = cmsg_buf, .msg_controllen = sizeof(cmsg_buf), - }; - auto cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - cmsg->cmsg_len = CMSG_LEN(sizeof(int)); - *reinterpret_cast(CMSG_DATA(cmsg)) = fd.get(); - - return TEMP_FAILURE_RETRY(sendmsg(sockfd, &msg, 0)); -} - -ssize_t recv_fd(int sockfd, void* _Nonnull data, size_t len, unique_fd* _Nullable out_fd) { - char cmsg_buf[CMSG_SPACE(sizeof(int))]; - - iovec iov = { .iov_base = const_cast(data), .iov_len = len }; - msghdr msg = { - .msg_iov = &iov, - .msg_iovlen = 1, - .msg_control = cmsg_buf, - .msg_controllen = sizeof(cmsg_buf), - .msg_flags = 0, - }; - auto cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - cmsg->cmsg_len = CMSG_LEN(sizeof(int)); - - ssize_t result = TEMP_FAILURE_RETRY(recvmsg(sockfd, &msg, 0)); - if (result == -1) { - return -1; - } - - unique_fd fd; - bool received_fd = msg.msg_controllen == sizeof(cmsg_buf); - if (received_fd) { - fd.reset(*reinterpret_cast(CMSG_DATA(cmsg))); - } - - if ((msg.msg_flags & MSG_TRUNC) != 0) { - errno = EFBIG; - return -1; - } else if ((msg.msg_flags & MSG_CTRUNC) != 0) { - errno = ERANGE; - return -1; - } - - if (out_fd) { - *out_fd = std::move(fd); - } else if (received_fd) { - errno = ERANGE; - return -1; - } - - return result; -} - std::string get_process_name(pid_t pid) { std::string result = ""; android::base::ReadFileToString(android::base::StringPrintf("/proc/%d/cmdline", pid), &result); diff --git a/debuggerd/util.h b/debuggerd/util.h index 8260b4496..e96442360 100644 --- a/debuggerd/util.h +++ b/debuggerd/util.h @@ -21,29 +21,5 @@ #include #include -#include - -// *** WARNING *** -// tombstoned's sockets are SOCK_SEQPACKET sockets. -// Short reads are treated as errors and short writes are assumed to not happen. - -// Sends a packet with an attached fd. -ssize_t send_fd(int sockfd, const void* _Nonnull data, size_t len, android::base::unique_fd fd); - -// Receives a packet and optionally, its attached fd. -// If out_fd is non-null, packets can optionally have an attached fd. -// If out_fd is null, received packets must not have an attached fd. -// -// Errors: -// EOVERFLOW: sockfd is SOCK_DGRAM or SOCK_SEQPACKET and buffer is too small. -// The first len bytes of the packet are stored in data, but the -// rest of the packet is dropped. -// ERANGE: too many file descriptors were attached to the packet. -// ENOMSG: not enough file descriptors were attached to the packet. -// -// plus any errors returned by the underlying recvmsg. -ssize_t recv_fd(int sockfd, void* _Nonnull data, size_t len, - android::base::unique_fd* _Nullable out_fd); - std::string get_process_name(pid_t pid); std::string get_thread_name(pid_t tid); From 8e0af5f1b506fb668517dbf79ec059aae494b366 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 10 Jan 2019 14:29:29 -0800 Subject: [PATCH 3/3] adb: switch to base::{Send,Receive}FileDescriptors. Test: test_device.py Test: adb abb package list packages Change-Id: I080dc8620d77b0e6144c895fd2a0cbf7b8063c53 --- adb/adb_io.cpp | 76 ------------------------------------- adb/adb_io.h | 9 ----- adb/daemon/abb.cpp | 6 ++- adb/daemon/abb_service.cpp | 7 +++- adb/daemon/jdwp_service.cpp | 4 +- 5 files changed, 12 insertions(+), 90 deletions(-) diff --git a/adb/adb_io.cpp b/adb/adb_io.cpp index 605d27dc4..91b0d1f62 100644 --- a/adb/adb_io.cpp +++ b/adb/adb_io.cpp @@ -187,79 +187,3 @@ bool ReadOrderlyShutdown(int fd) { return false; } } - -#if defined(__linux__) -bool SendFileDescriptor(int socket_fd, int fd) { - struct msghdr msg; - struct iovec iov; - char dummy = '!'; - union { - cmsghdr cm; - char buffer[CMSG_SPACE(sizeof(int))]; - } cm_un; - - iov.iov_base = &dummy; - iov.iov_len = 1; - msg.msg_name = nullptr; - msg.msg_namelen = 0; - msg.msg_iov = &iov; - msg.msg_iovlen = 1; - msg.msg_flags = 0; - msg.msg_control = cm_un.buffer; - msg.msg_controllen = sizeof(cm_un.buffer); - - cmsghdr* cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_len = CMSG_LEN(sizeof(int)); - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - ((int*)CMSG_DATA(cmsg))[0] = fd; - - int ret = TEMP_FAILURE_RETRY(sendmsg(socket_fd, &msg, 0)); - if (ret < 0) { - D("sending file descriptor via socket %d failed: %s", socket_fd, strerror(errno)); - return false; - } - - D("sent file descriptor %d to via socket %d", fd, socket_fd); - return true; -} - -bool ReceiveFileDescriptor(int socket_fd, unique_fd* fd, std::string* error) { - char dummy = '!'; - union { - cmsghdr cm; - char buffer[CMSG_SPACE(sizeof(int))]; - } cm_un; - - iovec iov; - iov.iov_base = &dummy; - iov.iov_len = 1; - - msghdr msg; - msg.msg_name = nullptr; - msg.msg_namelen = 0; - msg.msg_iov = &iov; - msg.msg_iovlen = 1; - msg.msg_flags = 0; - msg.msg_control = cm_un.buffer; - msg.msg_controllen = sizeof(cm_un.buffer); - - cmsghdr* cmsg = CMSG_FIRSTHDR(&msg); - cmsg->cmsg_len = CMSG_LEN(sizeof(int)); - cmsg->cmsg_level = SOL_SOCKET; - cmsg->cmsg_type = SCM_RIGHTS; - ((int*)(CMSG_DATA(cmsg)))[0] = -1; - - int rc = TEMP_FAILURE_RETRY(recvmsg(socket_fd, &msg, 0)); - if (rc <= 0) { - *error = perror_str("receiving file descriptor via socket failed"); - D("receiving file descriptor via socket %d failed: %s", socket_fd, strerror(errno)); - return false; - } - - fd->reset(((int*)(CMSG_DATA(cmsg)))[0]); - D("received file descriptor %d to via socket %d", fd->get(), socket_fd); - - return true; -} -#endif diff --git a/adb/adb_io.h b/adb/adb_io.h index 2ccaa3233..e2df1b1ef 100644 --- a/adb/adb_io.h +++ b/adb/adb_io.h @@ -74,13 +74,4 @@ bool WriteFdExactly(int fd, const std::string& s); // Same as above, but formats the string to send. bool WriteFdFmt(int fd, const char* fmt, ...) __attribute__((__format__(__printf__, 2, 3))); - -#if !ADB_HOST -// Sends an FD via Unix domain socket. -bool SendFileDescriptor(int socket_fd, int fd); - -// Receives an FD via Unix domain socket. -bool ReceiveFileDescriptor(int socket_fd, unique_fd* fd, std::string* error); -#endif - #endif /* ADB_IO_H */ diff --git a/adb/daemon/abb.cpp b/adb/daemon/abb.cpp index f69babe6c..d949dd118 100644 --- a/adb/daemon/abb.cpp +++ b/adb/daemon/abb.cpp @@ -22,6 +22,8 @@ #include +#include + namespace { class AdbFdTextOutput : public android::TextOutput { @@ -83,8 +85,8 @@ int main(int argc, char* const argv[]) { break; } - auto result = StartCommandInProcess(std::move(data), &execCmd); - if (!SendFileDescriptor(fd, result)) { + unique_fd result = StartCommandInProcess(std::move(data), &execCmd); + if (android::base::SendFileDescriptors(fd, "", 1, result.get()) != 1) { PLOG(ERROR) << "Failed to send an inprocess fd for command: " << data; break; } diff --git a/adb/daemon/abb_service.cpp b/adb/daemon/abb_service.cpp index 817aea1c7..d32bf52fb 100644 --- a/adb/daemon/abb_service.cpp +++ b/adb/daemon/abb_service.cpp @@ -20,6 +20,8 @@ #include "adb_utils.h" #include "shell_service.h" +#include + namespace { struct AbbProcess; @@ -59,8 +61,9 @@ unique_fd AbbProcess::sendCommand(std::string_view command) { unique_fd fd; std::string error; - if (!ReceiveFileDescriptor(socket_fd_, &fd, &error)) { - LOG(ERROR) << "failed to receive FD from abb: " << error; + char buf; + if (android::base::ReceiveFileDescriptors(socket_fd_, &buf, 1, &fd) != 1) { + PLOG(ERROR) << "failed to receive FD from abb"; socket_fd_.reset(); continue; } diff --git a/adb/daemon/jdwp_service.cpp b/adb/daemon/jdwp_service.cpp index 032ee42a2..66bfc0d81 100644 --- a/adb/daemon/jdwp_service.cpp +++ b/adb/daemon/jdwp_service.cpp @@ -32,6 +32,8 @@ #include #include +#include + #include "adb.h" #include "adb_io.h" #include "adb_unique_fd.h" @@ -237,7 +239,7 @@ static void jdwp_process_event(int socket, unsigned events, void* _proc) { CHECK(!proc->out_fds.empty()); int fd = proc->out_fds.back().get(); - if (!SendFileDescriptor(socket, fd)) { + if (android::base::SendFileDescriptors(socket, "", 1, fd) != 1) { D("sending new file descriptor to JDWP %d failed: %s", proc->pid, strerror(errno)); goto CloseProcess; }