From 1b49634f830ddf6f45fbccbc69d3064367d62076 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 17 Jul 2018 11:08:48 -0700 Subject: [PATCH 1/2] libziparchive: use fdsan in ZipArchive. Test: treehugger Change-Id: I8586b8ad27c4f1eda1a5266867da8dbbf4870c5e --- libziparchive/zip_archive.cc | 42 +++++++++++++++++++++++++++++ libziparchive/zip_archive_private.h | 30 +++------------------ 2 files changed, 45 insertions(+), 27 deletions(-) diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc index 5e5e7afd1..9536fc7ae 100644 --- a/libziparchive/zip_archive.cc +++ b/libziparchive/zip_archive.cc @@ -33,6 +33,10 @@ #include #include +#if defined(__BIONIC__) +#include +#endif + #include #include #include // TEMP_FAILURE_RETRY may or may not be in unistd @@ -165,6 +169,44 @@ static int32_t AddToHash(ZipString* hash_table, const uint64_t hash_table_size, return 0; } +ZipArchive::ZipArchive(const int fd, bool assume_ownership) + : mapped_zip(fd), + close_file(assume_ownership), + directory_offset(0), + central_directory(), + directory_map(new android::FileMap()), + num_entries(0), + hash_table_size(0), + hash_table(nullptr) { +#if defined(__BIONIC__) + if (assume_ownership) { + android_fdsan_exchange_owner_tag(fd, 0, reinterpret_cast(this)); + } +#endif +} + +ZipArchive::ZipArchive(void* address, size_t length) + : mapped_zip(address, length), + close_file(false), + directory_offset(0), + central_directory(), + directory_map(new android::FileMap()), + num_entries(0), + hash_table_size(0), + hash_table(nullptr) {} + +ZipArchive::~ZipArchive() { + if (close_file && mapped_zip.GetFileDescriptor() >= 0) { +#if defined(__BIONIC__) + android_fdsan_close_with_tag(mapped_zip.GetFileDescriptor(), reinterpret_cast(this)); +#else + close(mapped_zip.GetFileDescriptor()); +#endif + } + + free(hash_table); +} + static int32_t MapCentralDirectory0(const char* debug_file_name, ZipArchive* archive, off64_t file_length, off64_t read_amount, uint8_t* scan_buffer) { const off64_t search_start = file_length - read_amount; diff --git a/libziparchive/zip_archive_private.h b/libziparchive/zip_archive_private.h index 18e02291d..0a73300eb 100644 --- a/libziparchive/zip_archive_private.h +++ b/libziparchive/zip_archive_private.h @@ -156,33 +156,9 @@ struct ZipArchive { uint32_t hash_table_size; ZipString* hash_table; - ZipArchive(const int fd, bool assume_ownership) - : mapped_zip(fd), - close_file(assume_ownership), - directory_offset(0), - central_directory(), - directory_map(new android::FileMap()), - num_entries(0), - hash_table_size(0), - hash_table(nullptr) {} - - ZipArchive(void* address, size_t length) - : mapped_zip(address, length), - close_file(false), - directory_offset(0), - central_directory(), - directory_map(new android::FileMap()), - num_entries(0), - hash_table_size(0), - hash_table(nullptr) {} - - ~ZipArchive() { - if (close_file && mapped_zip.GetFileDescriptor() >= 0) { - close(mapped_zip.GetFileDescriptor()); - } - - free(hash_table); - } + ZipArchive(const int fd, bool assume_ownership); + ZipArchive(void* address, size_t length); + ~ZipArchive(); bool InitializeCentralDirectory(const char* debug_file_name, off64_t cd_start_offset, size_t cd_size); From 2d08ae57d46fe1626459fd8b67bb50526f3a97ae Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 18 Jul 2018 17:26:24 -0700 Subject: [PATCH 2/2] 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.