From 2d08ae57d46fe1626459fd8b67bb50526f3a97ae Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 18 Jul 2018 17:26:24 -0700 Subject: [PATCH] libutils: switch Looper's fds to unique_fd. Switch Looper to using unique_fd for its owned file descriptors, to benefit from fdsan. Bug: http://b/111560345 Test: treehugger Change-Id: I8efff7741ed19fd71f82f7e604b4f1c66fc5ea2b --- libutils/Android.bp | 1 + libutils/Looper.cpp | 52 ++++++++++++++++----------------- libutils/include/utils/Looper.h | 6 ++-- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/libutils/Android.bp b/libutils/Android.bp index bbfa9d857..d635e6559 100644 --- a/libutils/Android.bp +++ b/libutils/Android.bp @@ -155,6 +155,7 @@ cc_library { ], }, linux: { + shared_libs: ["libbase"], srcs: [ "Looper.cpp", ], diff --git a/libutils/Looper.cpp b/libutils/Looper.cpp index 7bc239797..102fdf037 100644 --- a/libutils/Looper.cpp +++ b/libutils/Looper.cpp @@ -60,24 +60,22 @@ static const int EPOLL_MAX_EVENTS = 16; static pthread_once_t gTLSOnce = PTHREAD_ONCE_INIT; static pthread_key_t gTLSKey = 0; -Looper::Looper(bool allowNonCallbacks) : - mAllowNonCallbacks(allowNonCallbacks), mSendingMessage(false), - mPolling(false), mEpollFd(-1), mEpollRebuildRequired(false), - mNextRequestSeq(0), mResponseIndex(0), mNextMessageUptime(LLONG_MAX) { - mWakeEventFd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); - LOG_ALWAYS_FATAL_IF(mWakeEventFd < 0, "Could not make wake event fd: %s", - strerror(errno)); +Looper::Looper(bool allowNonCallbacks) + : mAllowNonCallbacks(allowNonCallbacks), + mSendingMessage(false), + mPolling(false), + mEpollRebuildRequired(false), + mNextRequestSeq(0), + mResponseIndex(0), + mNextMessageUptime(LLONG_MAX) { + mWakeEventFd.reset(eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC)); + LOG_ALWAYS_FATAL_IF(mWakeEventFd.get() < 0, "Could not make wake event fd: %s", strerror(errno)); AutoMutex _l(mLock); rebuildEpollLocked(); } Looper::~Looper() { - close(mWakeEventFd); - mWakeEventFd = -1; - if (mEpollFd >= 0) { - close(mEpollFd); - } } void Looper::initTLSKey() { @@ -137,18 +135,18 @@ void Looper::rebuildEpollLocked() { #if DEBUG_CALLBACKS ALOGD("%p ~ rebuildEpollLocked - rebuilding epoll set", this); #endif - close(mEpollFd); + mEpollFd.reset(); } // Allocate the new epoll instance and register the wake pipe. - mEpollFd = epoll_create(EPOLL_SIZE_HINT); + mEpollFd.reset(epoll_create(EPOLL_SIZE_HINT)); LOG_ALWAYS_FATAL_IF(mEpollFd < 0, "Could not create epoll instance: %s", strerror(errno)); struct epoll_event eventItem; memset(& eventItem, 0, sizeof(epoll_event)); // zero out unused members of data field union eventItem.events = EPOLLIN; - eventItem.data.fd = mWakeEventFd; - int result = epoll_ctl(mEpollFd, EPOLL_CTL_ADD, mWakeEventFd, & eventItem); + eventItem.data.fd = mWakeEventFd.get(); + int result = epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, mWakeEventFd.get(), &eventItem); LOG_ALWAYS_FATAL_IF(result != 0, "Could not add wake event fd to epoll instance: %s", strerror(errno)); @@ -157,7 +155,7 @@ void Looper::rebuildEpollLocked() { struct epoll_event eventItem; request.initEventItem(&eventItem); - int epollResult = epoll_ctl(mEpollFd, EPOLL_CTL_ADD, request.fd, & eventItem); + int epollResult = epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, request.fd, &eventItem); if (epollResult < 0) { ALOGE("Error adding epoll events for fd %d while rebuilding epoll set: %s", request.fd, strerror(errno)); @@ -239,7 +237,7 @@ int Looper::pollInner(int timeoutMillis) { mPolling = true; struct epoll_event eventItems[EPOLL_MAX_EVENTS]; - int eventCount = epoll_wait(mEpollFd, eventItems, EPOLL_MAX_EVENTS, timeoutMillis); + int eventCount = epoll_wait(mEpollFd.get(), eventItems, EPOLL_MAX_EVENTS, timeoutMillis); // No longer idling. mPolling = false; @@ -281,7 +279,7 @@ int Looper::pollInner(int timeoutMillis) { for (int i = 0; i < eventCount; i++) { int fd = eventItems[i].data.fd; uint32_t epollEvents = eventItems[i].events; - if (fd == mWakeEventFd) { + if (fd == mWakeEventFd.get()) { if (epollEvents & EPOLLIN) { awoken(); } else { @@ -401,11 +399,11 @@ void Looper::wake() { #endif uint64_t inc = 1; - ssize_t nWrite = TEMP_FAILURE_RETRY(write(mWakeEventFd, &inc, sizeof(uint64_t))); + ssize_t nWrite = TEMP_FAILURE_RETRY(write(mWakeEventFd.get(), &inc, sizeof(uint64_t))); if (nWrite != sizeof(uint64_t)) { if (errno != EAGAIN) { - LOG_ALWAYS_FATAL("Could not write wake signal to fd %d: %s", - mWakeEventFd, strerror(errno)); + LOG_ALWAYS_FATAL("Could not write wake signal to fd %d: %s", mWakeEventFd.get(), + strerror(errno)); } } } @@ -416,7 +414,7 @@ void Looper::awoken() { #endif uint64_t counter; - TEMP_FAILURE_RETRY(read(mWakeEventFd, &counter, sizeof(uint64_t))); + TEMP_FAILURE_RETRY(read(mWakeEventFd.get(), &counter, sizeof(uint64_t))); } void Looper::pushResponse(int events, const Request& request) { @@ -467,14 +465,14 @@ int Looper::addFd(int fd, int ident, int events, const sp& callb ssize_t requestIndex = mRequests.indexOfKey(fd); if (requestIndex < 0) { - int epollResult = epoll_ctl(mEpollFd, EPOLL_CTL_ADD, fd, & eventItem); + int epollResult = epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, fd, &eventItem); if (epollResult < 0) { ALOGE("Error adding epoll events for fd %d: %s", fd, strerror(errno)); return -1; } mRequests.add(fd, request); } else { - int epollResult = epoll_ctl(mEpollFd, EPOLL_CTL_MOD, fd, & eventItem); + int epollResult = epoll_ctl(mEpollFd.get(), EPOLL_CTL_MOD, fd, &eventItem); if (epollResult < 0) { if (errno == ENOENT) { // Tolerate ENOENT because it means that an older file descriptor was @@ -495,7 +493,7 @@ int Looper::addFd(int fd, int ident, int events, const sp& callb "being recycled, falling back on EPOLL_CTL_ADD: %s", this, strerror(errno)); #endif - epollResult = epoll_ctl(mEpollFd, EPOLL_CTL_ADD, fd, & eventItem); + epollResult = epoll_ctl(mEpollFd.get(), EPOLL_CTL_ADD, fd, &eventItem); if (epollResult < 0) { ALOGE("Error modifying or adding epoll events for fd %d: %s", fd, strerror(errno)); @@ -542,7 +540,7 @@ int Looper::removeFd(int fd, int seq) { // updating the epoll set so that we avoid accidentally leaking callbacks. mRequests.removeItemsAt(requestIndex); - int epollResult = epoll_ctl(mEpollFd, EPOLL_CTL_DEL, fd, nullptr); + int epollResult = epoll_ctl(mEpollFd.get(), EPOLL_CTL_DEL, fd, nullptr); if (epollResult < 0) { if (seq != -1 && (errno == EBADF || errno == ENOENT)) { // Tolerate EBADF or ENOENT when the sequence number is known because it diff --git a/libutils/include/utils/Looper.h b/libutils/include/utils/Looper.h index 4509d75ff..c439c5ce0 100644 --- a/libutils/include/utils/Looper.h +++ b/libutils/include/utils/Looper.h @@ -24,6 +24,8 @@ #include +#include + namespace android { /* @@ -447,7 +449,7 @@ private: const bool mAllowNonCallbacks; // immutable - int mWakeEventFd; // immutable + android::base::unique_fd mWakeEventFd; // immutable Mutex mLock; Vector mMessageEnvelopes; // guarded by mLock @@ -457,7 +459,7 @@ private: // any use of it is racy anyway. volatile bool mPolling; - int mEpollFd; // guarded by mLock but only modified on the looper thread + android::base::unique_fd mEpollFd; // guarded by mLock but only modified on the looper thread bool mEpollRebuildRequired; // guarded by mLock // Locked list of file descriptor monitoring requests.