From 49750f15ed2db55247cc03170fec1eed617d9454 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 11 Jul 2018 16:32:10 -0700 Subject: applypatch: Fix the return type of FreeSpaceForFile(). Prior to this CL, FreeSpaceForFile() was returning `size_t`, which may overflow on ILP32 when called on a partition with 4GiB+ free space. Additionally, it was returning static_cast(-1) on error, but the caller in freecache.cpp didn't check for that. This CL changes its return type to `int64_t`, and moves the function into freecache.cpp since there's no external caller. Test: Run recovery_unit_test on marlin. Test: Code search shows no external user of FreeSpaceForFile(). Change-Id: I00f501a057726e1f1ab69f367c46c77b30f2d774 --- applypatch/applypatch.cpp | 10 ---------- applypatch/freecache.cpp | 32 ++++++++++++++++++++++++++++-- applypatch/include/applypatch/applypatch.h | 10 +++------- tests/unit/applypatch_test.cpp | 24 ++++++++++++++++------ 4 files changed, 51 insertions(+), 25 deletions(-) diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp index eb0a2a7b5..13e4b1ae0 100644 --- a/applypatch/applypatch.cpp +++ b/applypatch/applypatch.cpp @@ -23,7 +23,6 @@ #include #include #include -#include #include #include @@ -421,15 +420,6 @@ static size_t FileSink(const unsigned char* data, size_t len, int fd) { return done; } -size_t FreeSpaceForFile(const std::string& filename) { - struct statfs sf; - if (statfs(filename.c_str(), &sf) != 0) { - PLOG(ERROR) << "Failed to statfs " << filename; - return -1; - } - return sf.f_bsize * sf.f_bavail; -} - int CacheSizeCheck(size_t bytes) { if (MakeFreeSpaceOnCache(bytes) < 0) { LOG(ERROR) << "Failed to make " << bytes << " bytes available on /cache"; diff --git a/applypatch/freecache.cpp b/applypatch/freecache.cpp index 5a08a63ed..4989b7496 100644 --- a/applypatch/freecache.cpp +++ b/applypatch/freecache.cpp @@ -16,10 +16,12 @@ #include #include +#include #include #include #include #include +#include #include #include @@ -130,6 +132,24 @@ static unsigned int GetLogIndex(const std::string& log_name) { return std::numeric_limits::max(); } +// Returns the amount of free space (in bytes) on the filesystem containing filename, or -1 on +// error. +static int64_t FreeSpaceForFile(const std::string& filename) { + struct statfs sf; + if (statfs(filename.c_str(), &sf) == -1) { + PLOG(ERROR) << "Failed to statfs " << filename; + return -1; + } + + int64_t free_space = static_cast(sf.f_bsize) * sf.f_bavail; + if (sf.f_bsize == 0 || free_space / sf.f_bsize != sf.f_bavail) { + LOG(ERROR) << "Invalid block size or overflow (sf.f_bsize " << sf.f_bsize << ", sf.f_bavail " + << sf.f_bavail << ")"; + return -1; + } + return free_space; +} + int MakeFreeSpaceOnCache(size_t bytes_needed) { #ifndef __ANDROID__ // TODO(xunchang): Implement a heuristic cache size check during host simulation. @@ -149,7 +169,7 @@ int MakeFreeSpaceOnCache(size_t bytes_needed) { } bool RemoveFilesInDirectory(size_t bytes_needed, const std::string& dirname, - const std::function& space_checker) { + const std::function& space_checker) { struct stat st; if (stat(dirname.c_str(), &st) == -1) { PLOG(ERROR) << "Failed to stat " << dirname; @@ -160,7 +180,11 @@ bool RemoveFilesInDirectory(size_t bytes_needed, const std::string& dirname, return false; } - size_t free_now = space_checker(dirname); + int64_t free_now = space_checker(dirname); + if (free_now == -1) { + LOG(ERROR) << "Failed to check free space for " << dirname; + return false; + } LOG(INFO) << free_now << " bytes free on " << dirname << " (" << bytes_needed << " needed)"; if (free_now >= bytes_needed) { @@ -201,6 +225,10 @@ bool RemoveFilesInDirectory(size_t bytes_needed, const std::string& dirname, } free_now = space_checker(dirname); + if (free_now == -1) { + LOG(ERROR) << "Failed to check free space for " << dirname; + return false; + } LOG(INFO) << "Deleted " << file << "; now " << free_now << " bytes free"; if (free_now >= bytes_needed) { return true; diff --git a/applypatch/include/applypatch/applypatch.h b/applypatch/include/applypatch/applypatch.h index 92db59c3a..88659b86a 100644 --- a/applypatch/include/applypatch/applypatch.h +++ b/applypatch/include/applypatch/applypatch.h @@ -40,10 +40,6 @@ using SinkFn = std::function; int ShowLicenses(); -// Returns the amount of free space (in bytes) on the filesystem containing filename, or -1 on -// error. filename must exist. -size_t FreeSpaceForFile(const std::string& filename); - // Checks whether /cache partition has at least 'bytes'-byte free space. Returns 0 on having // sufficient space. int CacheSizeCheck(size_t bytes); @@ -119,8 +115,8 @@ int ApplyImagePatch(const unsigned char* old_data, size_t old_size, const Value& int MakeFreeSpaceOnCache(size_t bytes_needed); -// Removes the files in |dirname| until we have at least |bytes_needed| bytes of free space on -// the partition. The size of the free space is returned by calling |space_checker|. +// Removes the files in |dirname| until we have at least |bytes_needed| bytes of free space on the +// partition. |space_checker| should return the size of the free space, or -1 on error. bool RemoveFilesInDirectory(size_t bytes_needed, const std::string& dirname, - const std::function& space_checker); + const std::function& space_checker); #endif diff --git a/tests/unit/applypatch_test.cpp b/tests/unit/applypatch_test.cpp index 5cc03bc7b..4b757feab 100644 --- a/tests/unit/applypatch_test.cpp +++ b/tests/unit/applypatch_test.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -29,6 +30,7 @@ #include #include +#include #include #include #include @@ -176,7 +178,7 @@ class FreeCacheTest : public ::testing::Test { // A mock method to calculate the free space. It assumes the partition has a total size of 40960 // bytes and all files are 4096 bytes in size. - size_t MockFreeSpaceChecker(const std::string& dirname) { + int64_t MockFreeSpaceChecker(const std::string& dirname) { std::vector files = FindFilesInDir(dirname); return PARTITION_SIZE - 4096 * files.size(); } @@ -191,7 +193,7 @@ TEST_F(FreeCacheTest, FreeCacheSmoke) { ASSERT_EQ(files, FindFilesInDir(mock_cache.path)); ASSERT_EQ(4096 * 7, MockFreeSpaceChecker(mock_cache.path)); - ASSERT_TRUE(RemoveFilesInDirectory(4096 * 9, mock_cache.path, [&](const std::string& dir) { + ASSERT_TRUE(RemoveFilesInDirectory(4096 * 9, mock_cache.path, [this](const std::string& dir) { return this->MockFreeSpaceChecker(dir); })); @@ -199,6 +201,16 @@ TEST_F(FreeCacheTest, FreeCacheSmoke) { ASSERT_EQ(4096 * 9, MockFreeSpaceChecker(mock_cache.path)); } +TEST_F(FreeCacheTest, FreeCacheFreeSpaceCheckerError) { + std::vector files{ "file1", "file2", "file3" }; + AddFilesToDir(mock_cache.path, files); + ASSERT_EQ(files, FindFilesInDir(mock_cache.path)); + ASSERT_EQ(4096 * 7, MockFreeSpaceChecker(mock_cache.path)); + + ASSERT_FALSE( + RemoveFilesInDirectory(4096 * 9, mock_cache.path, [](const std::string&) { return -1; })); +} + TEST_F(FreeCacheTest, FreeCacheOpenFile) { std::vector files = { "file1", "file2" }; AddFilesToDir(mock_cache.path, files); @@ -209,7 +221,7 @@ TEST_F(FreeCacheTest, FreeCacheOpenFile) { android::base::unique_fd fd(open(file1_path.c_str(), O_RDONLY)); // file1 can't be deleted as it's opened by us. - ASSERT_FALSE(RemoveFilesInDirectory(4096 * 10, mock_cache.path, [&](const std::string& dir) { + ASSERT_FALSE(RemoveFilesInDirectory(4096 * 10, mock_cache.path, [this](const std::string& dir) { return this->MockFreeSpaceChecker(dir); })); @@ -222,7 +234,7 @@ TEST_F(FreeCacheTest, FreeCacheLogsSmoke) { AddFilesToDir(mock_log_dir.path, log_files); ASSERT_EQ(4096 * 5, MockFreeSpaceChecker(mock_log_dir.path)); - ASSERT_TRUE(RemoveFilesInDirectory(4096 * 8, mock_log_dir.path, [&](const std::string& dir) { + ASSERT_TRUE(RemoveFilesInDirectory(4096 * 8, mock_log_dir.path, [this](const std::string& dir) { return this->MockFreeSpaceChecker(dir); })); @@ -238,7 +250,7 @@ TEST_F(FreeCacheTest, FreeCacheLogsStringComparison) { AddFilesToDir(mock_log_dir.path, log_files); ASSERT_EQ(4096 * 6, MockFreeSpaceChecker(mock_log_dir.path)); - ASSERT_TRUE(RemoveFilesInDirectory(4096 * 9, mock_log_dir.path, [&](const std::string& dir) { + ASSERT_TRUE(RemoveFilesInDirectory(4096 * 9, mock_log_dir.path, [this](const std::string& dir) { return this->MockFreeSpaceChecker(dir); })); @@ -255,7 +267,7 @@ TEST_F(FreeCacheTest, FreeCacheLogsOtherFiles) { AddFilesToDir(mock_log_dir.path, log_files); ASSERT_EQ(4096 * 5, MockFreeSpaceChecker(mock_log_dir.path)); - ASSERT_FALSE(RemoveFilesInDirectory(4096 * 8, mock_log_dir.path, [&](const std::string& dir) { + ASSERT_FALSE(RemoveFilesInDirectory(4096 * 8, mock_log_dir.path, [this](const std::string& dir) { return this->MockFreeSpaceChecker(dir); })); -- cgit v1.2.3