From d77c99ebc3d896501531e9522b690c4a0e971aff Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Fri, 9 Aug 2019 15:22:55 -0700 Subject: [PATCH] [base] Make MappedFile work with OS file handles adb uses its own implementation of Windows HANDLE->int mapping, and it doesn't play well with _get_osfhandle() function used in MappedFile::FromFd(). This CL adds another function that accepts raw handle, so adb can pass it directly + make constant functions 'const' + make the MappedFile movable, as nothing prevents it from being one Test: libbase_test Change-Id: Ifde4a4094b910e9c7b431126ecf3fef5aa3bb4a6 --- base/include/android-base/mapped_file.h | 25 ++++++++- base/mapped_file.cpp | 73 +++++++++++++++++++------ base/mapped_file_test.cpp | 5 +- 3 files changed, 81 insertions(+), 22 deletions(-) diff --git a/base/include/android-base/mapped_file.h b/base/include/android-base/mapped_file.h index 2ab49ab1f..6a19f1b73 100644 --- a/base/include/android-base/mapped_file.h +++ b/base/include/android-base/mapped_file.h @@ -28,15 +28,17 @@ #include #define PROT_READ 1 #define PROT_WRITE 2 +using os_handle = HANDLE; #else #include +using os_handle = int; #endif namespace android { namespace base { /** - * A region of a file mapped into memory, also known as MmapFile. + * A region of a file mapped into memory (for grepping: also known as MmapFile or file mapping). */ class MappedFile { public: @@ -48,17 +50,34 @@ class MappedFile { static std::unique_ptr FromFd(borrowed_fd fd, off64_t offset, size_t length, int prot); + /** + * Same thing, but using the raw OS file handle instead of a CRT wrapper. + */ + static MappedFile FromOsHandle(os_handle h, off64_t offset, size_t length, int prot); + /** * Removes the mapping. */ ~MappedFile(); - char* data() { return base_ + offset_; } - size_t size() { return size_; } + /** + * Not copyable but movable. + */ + MappedFile(MappedFile&& other); + MappedFile& operator=(MappedFile&& other); + + char* data() const { return base_ + offset_; } + size_t size() const { return size_; } + + bool isValid() const { return base_ != nullptr; } + + explicit operator bool() const { return isValid(); } private: DISALLOW_IMPLICIT_CONSTRUCTORS(MappedFile); + void Close(); + char* base_; size_t size_; diff --git a/base/mapped_file.cpp b/base/mapped_file.cpp index f60de56f7..862b73bc2 100644 --- a/base/mapped_file.cpp +++ b/base/mapped_file.cpp @@ -16,13 +16,15 @@ #include "android-base/mapped_file.h" -#include +#include -#include "android-base/unique_fd.h" +#include namespace android { namespace base { +static constexpr char kEmptyBuffer[] = {'0'}; + static off64_t InitPageSize() { #if defined(_WIN32) SYSTEM_INFO si; @@ -35,51 +37,86 @@ static off64_t InitPageSize() { std::unique_ptr MappedFile::FromFd(borrowed_fd fd, off64_t offset, size_t length, int prot) { - static off64_t page_size = InitPageSize(); +#if defined(_WIN32) + auto file = + FromOsHandle(reinterpret_cast(_get_osfhandle(fd.get())), offset, length, prot); +#else + auto file = FromOsHandle(fd.get(), offset, length, prot); +#endif + return file ? std::make_unique(std::move(file)) : std::unique_ptr{}; +} + +MappedFile MappedFile::FromOsHandle(os_handle h, off64_t offset, size_t length, int prot) { + static const off64_t page_size = InitPageSize(); size_t slop = offset % page_size; off64_t file_offset = offset - slop; off64_t file_length = length + slop; #if defined(_WIN32) - HANDLE handle = - CreateFileMapping(reinterpret_cast(_get_osfhandle(fd.get())), nullptr, - (prot & PROT_WRITE) ? PAGE_READWRITE : PAGE_READONLY, 0, 0, nullptr); + HANDLE handle = CreateFileMappingW( + h, nullptr, (prot & PROT_WRITE) ? PAGE_READWRITE : PAGE_READONLY, 0, 0, nullptr); if (handle == nullptr) { // http://b/119818070 "app crashes when reading asset of zero length". // Return a MappedFile that's only valid for reading the size. - if (length == 0) { - return std::unique_ptr(new MappedFile{nullptr, 0, 0, nullptr}); + if (length == 0 && ::GetLastError() == ERROR_FILE_INVALID) { + return MappedFile{const_cast(kEmptyBuffer), 0, 0, nullptr}; } - return nullptr; + return MappedFile(nullptr, 0, 0, nullptr); } void* base = MapViewOfFile(handle, (prot & PROT_WRITE) ? FILE_MAP_ALL_ACCESS : FILE_MAP_READ, 0, file_offset, file_length); if (base == nullptr) { CloseHandle(handle); - return nullptr; + return MappedFile(nullptr, 0, 0, nullptr); } - return std::unique_ptr( - new MappedFile{static_cast(base), length, slop, handle}); + return MappedFile{static_cast(base), length, slop, handle}; #else - void* base = mmap(nullptr, file_length, prot, MAP_SHARED, fd.get(), file_offset); + void* base = mmap(nullptr, file_length, prot, MAP_SHARED, h, file_offset); if (base == MAP_FAILED) { // http://b/119818070 "app crashes when reading asset of zero length". // mmap fails with EINVAL for a zero length region. if (errno == EINVAL && length == 0) { - return std::unique_ptr(new MappedFile{nullptr, 0, 0}); + return MappedFile{const_cast(kEmptyBuffer), 0, 0}; } - return nullptr; + return MappedFile(nullptr, 0, 0); } - return std::unique_ptr(new MappedFile{static_cast(base), length, slop}); + return MappedFile{static_cast(base), length, slop}; #endif } +MappedFile::MappedFile(MappedFile&& other) + : base_(std::exchange(other.base_, nullptr)), + size_(std::exchange(other.size_, 0)), + offset_(std::exchange(other.offset_, 0)) +#ifdef _WIN32 + , + handle_(std::exchange(other.handle_, nullptr)) +#endif +{ +} + +MappedFile& MappedFile::operator=(MappedFile&& other) { + Close(); + base_ = std::exchange(other.base_, nullptr); + size_ = std::exchange(other.size_, 0); + offset_ = std::exchange(other.offset_, 0); +#ifdef _WIN32 + handle_ = std::exchange(other.handle_, nullptr); +#endif + return *this; +} + MappedFile::~MappedFile() { + Close(); +} + +void MappedFile::Close() { #if defined(_WIN32) - if (base_ != nullptr) UnmapViewOfFile(base_); + if (base_ != nullptr && size_ != 0) UnmapViewOfFile(base_); if (handle_ != nullptr) CloseHandle(handle_); + handle_ = nullptr; #else - if (base_ != nullptr) munmap(base_, size_ + offset_); + if (base_ != nullptr && size_ != 0) munmap(base_, size_ + offset_); #endif base_ = nullptr; diff --git a/base/mapped_file_test.cpp b/base/mapped_file_test.cpp index cfde73c74..3629108c9 100644 --- a/base/mapped_file_test.cpp +++ b/base/mapped_file_test.cpp @@ -44,5 +44,8 @@ TEST(mapped_file, zero_length_mapping) { ASSERT_TRUE(tf.fd != -1); auto m = android::base::MappedFile::FromFd(tf.fd, 4096, 0, PROT_READ); - ASSERT_EQ(0u, m->size()); + ASSERT_NE(nullptr, m); + EXPECT_TRUE((bool)*m); + EXPECT_EQ(0u, m->size()); + EXPECT_NE(nullptr, m->data()); }