From b656a154ea497c1a179079f082b0a0701453bec5 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Tue, 18 Apr 2017 23:54:29 -0700 Subject: [PATCH] Move sysMapFile and sysReleaseMap into MemMapping class. Test: recovery_component_test Test: recovery_unit_test Test: Apply an OTA on angler. Change-Id: I7170f03e4ce1fe06184ca1d7bcce0a695f33ac4d --- install.cpp | 6 +- otautil/SysUtil.cpp | 126 ++++++++++++++---------------- otautil/SysUtil.h | 48 ++++++------ tests/component/updater_test.cpp | 4 +- tests/component/verifier_test.cpp | 2 +- tests/unit/sysutil_test.cpp | 60 ++++++-------- tests/unit/zip_test.cpp | 5 +- updater/updater.cpp | 3 +- 8 files changed, 113 insertions(+), 141 deletions(-) diff --git a/install.cpp b/install.cpp index d86f46ca..689f4a0c 100644 --- a/install.cpp +++ b/install.cpp @@ -569,7 +569,7 @@ static int really_install_package(const std::string& path, bool* wipe_cache, boo } MemMapping map; - if (sysMapFile(path.c_str(), &map) != 0) { + if (!map.MapFile(path)) { LOG(ERROR) << "failed to map file"; return INSTALL_CORRUPT; } @@ -577,7 +577,6 @@ static int really_install_package(const std::string& path, bool* wipe_cache, boo // Verify package. if (!verify_package(map.addr, map.length)) { log_buffer->push_back(android::base::StringPrintf("error: %d", kZipVerificationFailure)); - sysReleaseMap(&map); return INSTALL_CORRUPT; } @@ -588,7 +587,6 @@ static int really_install_package(const std::string& path, bool* wipe_cache, boo LOG(ERROR) << "Can't open " << path << " : " << ErrorCodeString(err); log_buffer->push_back(android::base::StringPrintf("error: %d", kZipOpenFailure)); - sysReleaseMap(&map); CloseArchive(zip); return INSTALL_CORRUPT; } @@ -596,7 +594,6 @@ static int really_install_package(const std::string& path, bool* wipe_cache, boo // Additionally verify the compatibility of the package. if (!verify_package_compatibility(zip)) { log_buffer->push_back(android::base::StringPrintf("error: %d", kPackageCompatibilityFailure)); - sysReleaseMap(&map); CloseArchive(zip); return INSTALL_CORRUPT; } @@ -611,7 +608,6 @@ static int really_install_package(const std::string& path, bool* wipe_cache, boo ui->SetEnableReboot(true); ui->Print("\n"); - sysReleaseMap(&map); CloseArchive(zip); return result; } diff --git a/otautil/SysUtil.cpp b/otautil/SysUtil.cpp index a2133b95..dfa21507 100644 --- a/otautil/SysUtil.cpp +++ b/otautil/SysUtil.cpp @@ -16,14 +16,12 @@ #include "SysUtil.h" -#include #include -#include +#include // SIZE_MAX #include #include #include -#include #include #include @@ -32,9 +30,7 @@ #include #include -static bool sysMapFD(int fd, MemMapping* pMap) { - CHECK(pMap != nullptr); - +bool MemMapping::MapFD(int fd) { struct stat sb; if (fstat(fd, &sb) == -1) { PLOG(ERROR) << "fstat(" << fd << ") failed"; @@ -47,50 +43,49 @@ static bool sysMapFD(int fd, MemMapping* pMap) { return false; } - pMap->addr = static_cast(memPtr); - pMap->length = sb.st_size; - pMap->ranges.push_back({ memPtr, static_cast(sb.st_size) }); + addr = static_cast(memPtr); + length = sb.st_size; + ranges_.clear(); + ranges_.emplace_back(MappedRange{ memPtr, static_cast(sb.st_size) }); return true; } // A "block map" which looks like this (from uncrypt/uncrypt.cpp): // -// /dev/block/platform/msm_sdcc.1/by-name/userdata # block device -// 49652 4096 # file size in bytes, block size -// 3 # count of block ranges -// 1000 1008 # block range 0 -// 2100 2102 # ... block range 1 -// 30 33 # ... block range 2 +// /dev/block/platform/msm_sdcc.1/by-name/userdata # block device +// 49652 4096 # file size in bytes, block size +// 3 # count of block ranges +// 1000 1008 # block range 0 +// 2100 2102 # ... block range 1 +// 30 33 # ... block range 2 // -// Each block range represents a half-open interval; the line "30 33" -// reprents the blocks [30, 31, 32]. -static int sysMapBlockFile(const char* filename, MemMapping* pMap) { - CHECK(pMap != nullptr); - +// Each block range represents a half-open interval; the line "30 33" reprents the blocks +// [30, 31, 32]. +bool MemMapping::MapBlockFile(const std::string& filename) { std::string content; if (!android::base::ReadFileToString(filename, &content)) { PLOG(ERROR) << "Failed to read " << filename; - return -1; + return false; } std::vector lines = android::base::Split(android::base::Trim(content), "\n"); if (lines.size() < 4) { LOG(ERROR) << "Block map file is too short: " << lines.size(); - return -1; + return false; } size_t size; - unsigned int blksize; - if (sscanf(lines[1].c_str(), "%zu %u", &size, &blksize) != 2) { + size_t blksize; + if (sscanf(lines[1].c_str(), "%zu %zu", &size, &blksize) != 2) { LOG(ERROR) << "Failed to parse file size and block size: " << lines[1]; - return -1; + return false; } size_t range_count; if (sscanf(lines[2].c_str(), "%zu", &range_count) != 1) { LOG(ERROR) << "Failed to parse block map header: " << lines[2]; - return -1; + return false; } size_t blocks; @@ -101,14 +96,14 @@ static int sysMapBlockFile(const char* filename, MemMapping* pMap) { lines.size() != 3 + range_count) { LOG(ERROR) << "Invalid data in block map file: size " << size << ", blksize " << blksize << ", range_count " << range_count << ", lines " << lines.size(); - return -1; + return false; } // Reserve enough contiguous address space for the whole file. void* reserve = mmap64(nullptr, blocks * blksize, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0); if (reserve == MAP_FAILED) { PLOG(ERROR) << "failed to reserve address space"; - return -1; + return false; } const std::string& block_dev = lines[0]; @@ -116,10 +111,10 @@ static int sysMapBlockFile(const char* filename, MemMapping* pMap) { if (fd == -1) { PLOG(ERROR) << "failed to open block device " << block_dev; munmap(reserve, blocks * blksize); - return -1; + return false; } - pMap->ranges.resize(range_count); + ranges_.clear(); unsigned char* next = static_cast(reserve); size_t remaining_size = blocks * blksize; @@ -129,84 +124,79 @@ static int sysMapBlockFile(const char* filename, MemMapping* pMap) { size_t start, end; if (sscanf(line.c_str(), "%zu %zu\n", &start, &end) != 2) { - LOG(ERROR) << "failed to parse range " << i << " in block map: " << line; + LOG(ERROR) << "failed to parse range " << i << ": " << line; success = false; break; } - size_t length = (end - start) * blksize; - if (end <= start || (end - start) > SIZE_MAX / blksize || length > remaining_size) { - LOG(ERROR) << "unexpected range in block map: " << start << " " << end; + size_t range_size = (end - start) * blksize; + if (end <= start || (end - start) > SIZE_MAX / blksize || range_size > remaining_size) { + LOG(ERROR) << "Invalid range: " << start << " " << end; success = false; break; } - void* addr = mmap64(next, length, PROT_READ, MAP_PRIVATE | MAP_FIXED, fd, - static_cast(start) * blksize); - if (addr == MAP_FAILED) { - PLOG(ERROR) << "failed to map block " << i; + void* range_start = mmap64(next, range_size, PROT_READ, MAP_PRIVATE | MAP_FIXED, fd, + static_cast(start) * blksize); + if (range_start == MAP_FAILED) { + PLOG(ERROR) << "failed to map range " << i << ": " << line; success = false; break; } - pMap->ranges[i].addr = addr; - pMap->ranges[i].length = length; + ranges_.emplace_back(MappedRange{ range_start, range_size }); - next += length; - remaining_size -= length; + next += range_size; + remaining_size -= range_size; } if (success && remaining_size != 0) { - LOG(ERROR) << "ranges in block map are invalid: remaining_size = " << remaining_size; + LOG(ERROR) << "Invalid ranges: remaining_size " << remaining_size; success = false; } if (!success) { munmap(reserve, blocks * blksize); - return -1; + return false; } - pMap->addr = static_cast(reserve); - pMap->length = size; + addr = static_cast(reserve); + length = size; LOG(INFO) << "mmapped " << range_count << " ranges"; - return 0; + return true; } -int sysMapFile(const char* fn, MemMapping* pMap) { - if (fn == nullptr || pMap == nullptr) { - LOG(ERROR) << "Invalid argument(s)"; - return -1; +bool MemMapping::MapFile(const std::string& fn) { + if (fn.empty()) { + LOG(ERROR) << "Empty filename"; + return false; } - *pMap = {}; - if (fn[0] == '@') { - if (sysMapBlockFile(fn + 1, pMap) != 0) { + // Block map file "@/cache/recovery/block.map". + if (!MapBlockFile(fn.substr(1))) { LOG(ERROR) << "Map of '" << fn << "' failed"; - return -1; + return false; } } else { // This is a regular file. - android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(fn, O_RDONLY))); + android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(fn.c_str(), O_RDONLY))); if (fd == -1) { PLOG(ERROR) << "Unable to open '" << fn << "'"; - return -1; + return false; } - if (!sysMapFD(fd, pMap)) { + if (!MapFD(fd)) { LOG(ERROR) << "Map of '" << fn << "' failed"; - return -1; + return false; } } - return 0; + return true; } -/* - * Release a memory mapping. - */ -void sysReleaseMap(MemMapping* pMap) { - std::for_each(pMap->ranges.cbegin(), pMap->ranges.cend(), [](const MappedRange& range) { +MemMapping::~MemMapping() { + for (const auto& range : ranges_) { if (munmap(range.addr, range.length) == -1) { - PLOG(ERROR) << "munmap(" << range.addr << ", " << range.length << ") failed"; + PLOG(ERROR) << "Failed to munmap(" << range.addr << ", " << range.length << ")"; } - }); - pMap->ranges.clear(); + }; + ranges_.clear(); } diff --git a/otautil/SysUtil.h b/otautil/SysUtil.h index 6a79bf31..52f6d20a 100644 --- a/otautil/SysUtil.h +++ b/otautil/SysUtil.h @@ -19,37 +19,35 @@ #include +#include #include -struct MappedRange { - void* addr; - size_t length; -}; - /* * Use this to keep track of mapped segments. */ -struct MemMapping { - unsigned char* addr; /* start of data */ - size_t length; /* length of data */ +class MemMapping { + public: + ~MemMapping(); + // Map a file into a private, read-only memory segment. If 'filename' begins with an '@' + // character, it is a map of blocks to be mapped, otherwise it is treated as an ordinary file. + bool MapFile(const std::string& filename); + size_t ranges() const { + return ranges_.size(); + }; - std::vector ranges; + unsigned char* addr; // start of data + size_t length; // length of data + + private: + struct MappedRange { + void* addr; + size_t length; + }; + + bool MapBlockFile(const std::string& filename); + bool MapFD(int fd); + + std::vector ranges_; }; -/* - * Map a file into a private, read-only memory segment. If 'fn' - * begins with an '@' character, it is a map of blocks to be mapped, - * otherwise it is treated as an ordinary file. - * - * On success, "pMap" is filled in, and zero is returned. - */ -int sysMapFile(const char* fn, MemMapping* pMap); - -/* - * Release the pages associated with a shared memory segment. - * - * This does not free "pMap"; it just releases the memory. - */ -void sysReleaseMap(MemMapping* pMap); - #endif // _OTAUTIL_SYSUTIL diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp index dc4b5d72..35e87fd5 100644 --- a/tests/component/updater_test.cpp +++ b/tests/component/updater_test.cpp @@ -570,7 +570,7 @@ TEST_F(UpdaterTest, block_image_update) { ASSERT_EQ(0, fclose(zip_file_ptr)); MemMapping map; - ASSERT_EQ(0, sysMapFile(zip_file.path, &map)); + ASSERT_TRUE(map.MapFile(zip_file.path)); ZipArchiveHandle handle; ASSERT_EQ(0, OpenArchiveFromMemory(map.addr, map.length, zip_file.path, &handle)); @@ -646,7 +646,7 @@ TEST_F(UpdaterTest, new_data_short_write) { ASSERT_EQ(0, fclose(zip_file_ptr)); MemMapping map; - ASSERT_EQ(0, sysMapFile(zip_file.path, &map)); + ASSERT_TRUE(map.MapFile(zip_file.path)); ZipArchiveHandle handle; ASSERT_EQ(0, OpenArchiveFromMemory(map.addr, map.length, zip_file.path, &handle)); diff --git a/tests/component/verifier_test.cpp b/tests/component/verifier_test.cpp index 2cfb6d30..5338f05c 100644 --- a/tests/component/verifier_test.cpp +++ b/tests/component/verifier_test.cpp @@ -38,7 +38,7 @@ class VerifierTest : public testing::TestWithParam> { void SetUp() override { std::vector args = GetParam(); std::string package = from_testdata_base(args[0]); - if (sysMapFile(package.c_str(), &memmap) != 0) { + if (!memmap.MapFile(package)) { FAIL() << "Failed to mmap " << package << ": " << strerror(errno) << "\n"; } diff --git a/tests/unit/sysutil_test.cpp b/tests/unit/sysutil_test.cpp index f4699664..434ee25b 100644 --- a/tests/unit/sysutil_test.cpp +++ b/tests/unit/sysutil_test.cpp @@ -27,27 +27,23 @@ TEST(SysUtilTest, InvalidArgs) { MemMapping mapping; // Invalid argument. - ASSERT_EQ(-1, sysMapFile(nullptr, &mapping)); - ASSERT_EQ(-1, sysMapFile("/somefile", nullptr)); + ASSERT_FALSE(mapping.MapFile("")); } -TEST(SysUtilTest, sysMapFileRegularFile) { +TEST(SysUtilTest, MapFileRegularFile) { TemporaryFile temp_file1; std::string content = "abc"; ASSERT_TRUE(android::base::WriteStringToFile(content, temp_file1.path)); - // sysMapFile() should map the file to one range. + // MemMapping::MapFile() should map the file to one range. MemMapping mapping; - ASSERT_EQ(0, sysMapFile(temp_file1.path, &mapping)); + ASSERT_TRUE(mapping.MapFile(temp_file1.path)); ASSERT_NE(nullptr, mapping.addr); ASSERT_EQ(content.size(), mapping.length); - ASSERT_EQ(1U, mapping.ranges.size()); - - sysReleaseMap(&mapping); - ASSERT_EQ(0U, mapping.ranges.size()); + ASSERT_EQ(1U, mapping.ranges()); } -TEST(SysUtilTest, sysMapFileBlockMap) { +TEST(SysUtilTest, MapFileBlockMap) { // Create a file that has 10 blocks. TemporaryFile package; std::string content; @@ -63,78 +59,72 @@ TEST(SysUtilTest, sysMapFileBlockMap) { std::string block_map_content = std::string(package.path) + "\n40960 4096\n1\n0 10\n"; ASSERT_TRUE(android::base::WriteStringToFile(block_map_content, block_map_file.path)); - ASSERT_EQ(0, sysMapFile(filename.c_str(), &mapping)); + ASSERT_TRUE(mapping.MapFile(filename)); ASSERT_EQ(file_size, mapping.length); - ASSERT_EQ(1U, mapping.ranges.size()); + ASSERT_EQ(1U, mapping.ranges()); // It's okay to not have the trailing '\n'. block_map_content = std::string(package.path) + "\n40960 4096\n1\n0 10"; ASSERT_TRUE(android::base::WriteStringToFile(block_map_content, block_map_file.path)); - ASSERT_EQ(0, sysMapFile(filename.c_str(), &mapping)); + ASSERT_TRUE(mapping.MapFile(filename)); ASSERT_EQ(file_size, mapping.length); - ASSERT_EQ(1U, mapping.ranges.size()); + ASSERT_EQ(1U, mapping.ranges()); // Or having multiple trailing '\n's. block_map_content = std::string(package.path) + "\n40960 4096\n1\n0 10\n\n\n"; ASSERT_TRUE(android::base::WriteStringToFile(block_map_content, block_map_file.path)); - ASSERT_EQ(0, sysMapFile(filename.c_str(), &mapping)); + ASSERT_TRUE(mapping.MapFile(filename)); ASSERT_EQ(file_size, mapping.length); - ASSERT_EQ(1U, mapping.ranges.size()); + ASSERT_EQ(1U, mapping.ranges()); // Multiple ranges. block_map_content = std::string(package.path) + "\n40960 4096\n3\n0 3\n3 5\n5 10\n"; ASSERT_TRUE(android::base::WriteStringToFile(block_map_content, block_map_file.path)); - ASSERT_EQ(0, sysMapFile(filename.c_str(), &mapping)); + ASSERT_TRUE(mapping.MapFile(filename)); ASSERT_EQ(file_size, mapping.length); - ASSERT_EQ(3U, mapping.ranges.size()); - - sysReleaseMap(&mapping); - ASSERT_EQ(0U, mapping.ranges.size()); + ASSERT_EQ(3U, mapping.ranges()); } -TEST(SysUtilTest, sysMapFileBlockMapInvalidBlockMap) { +TEST(SysUtilTest, MapFileBlockMapInvalidBlockMap) { MemMapping mapping; TemporaryFile temp_file; std::string filename = std::string("@") + temp_file.path; // Block map file is too short. ASSERT_TRUE(android::base::WriteStringToFile("/somefile\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); ASSERT_TRUE(android::base::WriteStringToFile("/somefile\n4096 4096\n0\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); // Block map file has unexpected number of lines. ASSERT_TRUE(android::base::WriteStringToFile("/somefile\n4096 4096\n1\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); ASSERT_TRUE(android::base::WriteStringToFile("/somefile\n4096 4096\n2\n0 1\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); // Invalid size/blksize/range_count. ASSERT_TRUE(android::base::WriteStringToFile("/somefile\nabc 4096\n1\n0 1\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); ASSERT_TRUE(android::base::WriteStringToFile("/somefile\n4096 4096\n\n0 1\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); // size/blksize/range_count don't match. ASSERT_TRUE(android::base::WriteStringToFile("/somefile\n0 4096\n1\n0 1\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); ASSERT_TRUE(android::base::WriteStringToFile("/somefile\n4096 0\n1\n0 1\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); ASSERT_TRUE(android::base::WriteStringToFile("/somefile\n4096 4096\n0\n0 1\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); + ASSERT_FALSE(mapping.MapFile(filename)); // Invalid block dev path. ASSERT_TRUE(android::base::WriteStringToFile("/doesntexist\n4096 4096\n1\n0 1\n", temp_file.path)); - ASSERT_EQ(-1, sysMapFile(filename.c_str(), &mapping)); - - sysReleaseMap(&mapping); - ASSERT_EQ(0U, mapping.ranges.size()); + ASSERT_FALSE(mapping.MapFile(filename)); } diff --git a/tests/unit/zip_test.cpp b/tests/unit/zip_test.cpp index 4a1a49b9..df4e38ca 100644 --- a/tests/unit/zip_test.cpp +++ b/tests/unit/zip_test.cpp @@ -66,9 +66,9 @@ TEST(ZipTest, ExtractPackageRecursive) { } TEST(ZipTest, OpenFromMemory) { - MemMapping map; std::string zip_path = from_testdata_base("ziptest_dummy-update.zip"); - ASSERT_EQ(0, sysMapFile(zip_path.c_str(), &map)); + MemMapping map; + ASSERT_TRUE(map.MapFile(zip_path)); // Map an update package into memory and open the archive from there. ZipArchiveHandle handle; @@ -85,6 +85,5 @@ TEST(ZipTest, OpenFromMemory) { ASSERT_EQ(0, ExtractEntryToFile(handle, &binary_entry, tmp_binary.fd)); CloseArchive(handle); - sysReleaseMap(&map); } diff --git a/updater/updater.cpp b/updater/updater.cpp index c09e267a..749c86a1 100644 --- a/updater/updater.cpp +++ b/updater/updater.cpp @@ -89,7 +89,7 @@ int main(int argc, char** argv) { const char* package_filename = argv[3]; MemMapping map; - if (sysMapFile(package_filename, &map) != 0) { + if (!map.MapFile(package_filename)) { LOG(ERROR) << "failed to map package " << argv[3]; return 3; } @@ -213,7 +213,6 @@ int main(int argc, char** argv) { if (updater_info.package_zip) { CloseArchive(updater_info.package_zip); } - sysReleaseMap(&map); return 0; }