From 0150d013eb096a85e541294ee9df320ef4227df0 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 1 May 2017 11:31:28 -0700 Subject: adb_install: Stop passing RecoveryUI as a parameter. It's already a global declared in common.h which is included by adb_install.cpp. Remove '#include "minadbd/fuse_adb_provider.h"' that's not needed by adb_install.cpp (minadbd takes care of that). Test: mmma bootable/recovery Change-Id: I6d08b7abc706b4b05de2ef46a57ced2204ad297e --- adb_install.cpp | 189 ++++++++++++++++++++++++++++---------------------------- adb_install.h | 4 +- recovery.cpp | 4 +- 3 files changed, 96 insertions(+), 101 deletions(-) diff --git a/adb_install.cpp b/adb_install.cpp index 79b8df91b..e7d7758f9 100644 --- a/adb_install.cpp +++ b/adb_install.cpp @@ -14,124 +14,121 @@ * limitations under the License. */ -#include -#include +#include "adb_install.h" + #include +#include +#include #include #include +#include #include #include -#include -#include -#include +#include + +#include -#include "ui.h" -#include "install.h" #include "common.h" -#include "adb_install.h" -#include "minadbd/fuse_adb_provider.h" #include "fuse_sideload.h" +#include "install.h" +#include "ui.h" -#include - -static void set_usb_driver(RecoveryUI* ui, bool enabled) { - int fd = open("/sys/class/android_usb/android0/enable", O_WRONLY); - if (fd < 0) { - ui->Print("failed to open driver control: %s\n", strerror(errno)); - return; - } - if (TEMP_FAILURE_RETRY(write(fd, enabled ? "1" : "0", 1)) == -1) { - ui->Print("failed to set driver control: %s\n", strerror(errno)); - } - if (close(fd) < 0) { - ui->Print("failed to close driver control: %s\n", strerror(errno)); - } +static void set_usb_driver(bool enabled) { + int fd = open("/sys/class/android_usb/android0/enable", O_WRONLY); + if (fd < 0) { + ui->Print("failed to open driver control: %s\n", strerror(errno)); + return; + } + if (TEMP_FAILURE_RETRY(write(fd, enabled ? "1" : "0", 1)) == -1) { + ui->Print("failed to set driver control: %s\n", strerror(errno)); + } + if (close(fd) < 0) { + ui->Print("failed to close driver control: %s\n", strerror(errno)); + } } -static void stop_adbd(RecoveryUI* ui) { - ui->Print("Stopping adbd...\n"); - android::base::SetProperty("ctl.stop", "adbd"); - set_usb_driver(ui, false); +static void stop_adbd() { + ui->Print("Stopping adbd...\n"); + android::base::SetProperty("ctl.stop", "adbd"); + set_usb_driver(false); } -static void maybe_restart_adbd(RecoveryUI* ui) { - if (is_ro_debuggable()) { - ui->Print("Restarting adbd...\n"); - set_usb_driver(ui, true); - android::base::SetProperty("ctl.start", "adbd"); - } +static void maybe_restart_adbd() { + if (is_ro_debuggable()) { + ui->Print("Restarting adbd...\n"); + set_usb_driver(true); + android::base::SetProperty("ctl.start", "adbd"); + } } -// How long (in seconds) we wait for the host to start sending us a -// package, before timing out. -#define ADB_INSTALL_TIMEOUT 300 - -int apply_from_adb(RecoveryUI* ui, bool* wipe_cache, const char* install_file) { - modified_flash = true; - - stop_adbd(ui); - set_usb_driver(ui, true); - - ui->Print("\n\nNow send the package you want to apply\n" - "to the device with \"adb sideload \"...\n"); - - pid_t child; - if ((child = fork()) == 0) { - execl("/sbin/recovery", "recovery", "--adbd", NULL); - _exit(EXIT_FAILURE); +int apply_from_adb(bool* wipe_cache, const char* install_file) { + modified_flash = true; + + stop_adbd(); + set_usb_driver(true); + + ui->Print( + "\n\nNow send the package you want to apply\n" + "to the device with \"adb sideload \"...\n"); + + pid_t child; + if ((child = fork()) == 0) { + execl("/sbin/recovery", "recovery", "--adbd", nullptr); + _exit(EXIT_FAILURE); + } + + // How long (in seconds) we wait for the host to start sending us a package, before timing out. + static constexpr int ADB_INSTALL_TIMEOUT = 300; + + // FUSE_SIDELOAD_HOST_PATHNAME will start to exist once the host connects and starts serving a + // package. Poll for its appearance. (Note that inotify doesn't work with FUSE.) + int result = INSTALL_ERROR; + int status; + bool waited = false; + for (int i = 0; i < ADB_INSTALL_TIMEOUT; ++i) { + if (waitpid(child, &status, WNOHANG) != 0) { + result = INSTALL_ERROR; + waited = true; + break; } - // FUSE_SIDELOAD_HOST_PATHNAME will start to exist once the host - // connects and starts serving a package. Poll for its - // appearance. (Note that inotify doesn't work with FUSE.) - int result = INSTALL_ERROR; - int status; - bool waited = false; struct stat st; - for (int i = 0; i < ADB_INSTALL_TIMEOUT; ++i) { - if (waitpid(child, &status, WNOHANG) != 0) { - result = INSTALL_ERROR; - waited = true; - break; - } - - if (stat(FUSE_SIDELOAD_HOST_PATHNAME, &st) != 0) { - if (errno == ENOENT && i < ADB_INSTALL_TIMEOUT-1) { - sleep(1); - continue; - } else { - ui->Print("\nTimed out waiting for package.\n\n"); - result = INSTALL_ERROR; - kill(child, SIGKILL); - break; - } - } - result = install_package(FUSE_SIDELOAD_HOST_PATHNAME, wipe_cache, install_file, false, 0); + if (stat(FUSE_SIDELOAD_HOST_PATHNAME, &st) != 0) { + if (errno == ENOENT && i < ADB_INSTALL_TIMEOUT - 1) { + sleep(1); + continue; + } else { + ui->Print("\nTimed out waiting for package.\n\n"); + result = INSTALL_ERROR; + kill(child, SIGKILL); break; + } } + result = install_package(FUSE_SIDELOAD_HOST_PATHNAME, wipe_cache, install_file, false, 0); + break; + } - if (!waited) { - // Calling stat() on this magic filename signals the minadbd - // subprocess to shut down. - stat(FUSE_SIDELOAD_HOST_EXIT_PATHNAME, &st); - - // TODO(dougz): there should be a way to cancel waiting for a - // package (by pushing some button combo on the device). For now - // you just have to 'adb sideload' a file that's not a valid - // package, like "/dev/null". - waitpid(child, &status, 0); - } - - if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { - if (WEXITSTATUS(status) == 3) { - ui->Print("\nYou need adb 1.0.32 or newer to sideload\nto this device.\n\n"); - } else if (!WIFSIGNALED(status)) { - ui->Print("\n(adbd status %d)\n", WEXITSTATUS(status)); - } + if (!waited) { + // Calling stat() on this magic filename signals the minadbd subprocess to shut down. + struct stat st; + stat(FUSE_SIDELOAD_HOST_EXIT_PATHNAME, &st); + + // TODO: there should be a way to cancel waiting for a package (by pushing some button combo on + // the device). For now you just have to 'adb sideload' a file that's not a valid package, like + // "/dev/null". + waitpid(child, &status, 0); + } + + if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { + if (WEXITSTATUS(status) == 3) { + ui->Print("\nYou need adb 1.0.32 or newer to sideload\nto this device.\n\n"); + } else if (!WIFSIGNALED(status)) { + ui->Print("\n(adbd status %d)\n", WEXITSTATUS(status)); } + } - set_usb_driver(ui, false); - maybe_restart_adbd(ui); + set_usb_driver(false); + maybe_restart_adbd(); - return result; + return result; } diff --git a/adb_install.h b/adb_install.h index efad436fa..e654c893d 100644 --- a/adb_install.h +++ b/adb_install.h @@ -17,8 +17,6 @@ #ifndef _ADB_INSTALL_H #define _ADB_INSTALL_H -class RecoveryUI; - -int apply_from_adb(RecoveryUI* h, bool* wipe_cache, const char* install_file); +int apply_from_adb(bool* wipe_cache, const char* install_file); #endif diff --git a/recovery.cpp b/recovery.cpp index 3041d6cac..944c24086 100644 --- a/recovery.cpp +++ b/recovery.cpp @@ -1155,7 +1155,7 @@ static Device::BuiltinAction prompt_and_wait(Device* device, int status) { { bool adb = (chosen_action == Device::APPLY_ADB_SIDELOAD); if (adb) { - status = apply_from_adb(ui, &should_wipe_cache, TEMPORARY_INSTALL_FILE); + status = apply_from_adb(&should_wipe_cache, TEMPORARY_INSTALL_FILE); } else { status = apply_from_sdcard(device, &should_wipe_cache); } @@ -1584,7 +1584,7 @@ int main(int argc, char **argv) { if (!sideload_auto_reboot) { ui->ShowText(true); } - status = apply_from_adb(ui, &should_wipe_cache, TEMPORARY_INSTALL_FILE); + status = apply_from_adb(&should_wipe_cache, TEMPORARY_INSTALL_FILE); if (status == INSTALL_SUCCESS && should_wipe_cache) { if (!wipe_cache(false, device)) { status = INSTALL_ERROR; -- cgit v1.2.3 From 29ee69bf27cc7ad7ef7e604684110b3f562ed42d Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 1 May 2017 12:23:17 -0700 Subject: recovery: Change install_package() to take std::string. Also change the parameter type for log_buffer from reference to pointer, so the styles for parameters look consistent. Test: mmma bootable/recovery Test: sideload a package with the new recovery image Change-Id: I8f25580ccf22977624648b3e2181cca44dd67c1b --- install.cpp | 141 +++++++++++++++++++++++++++++++----------------------------- install.h | 11 +++-- 2 files changed, 77 insertions(+), 75 deletions(-) diff --git a/install.cpp b/install.cpp index e945d13ab..d86f46caa 100644 --- a/install.cpp +++ b/install.cpp @@ -100,7 +100,7 @@ bool read_metadata_from_package(ZipArchiveHandle zip, std::string* metadata) { } // Read the build.version.incremental of src/tgt from the metadata and log it to last_install. -static void read_source_target_build(ZipArchiveHandle zip, std::vector& log_buffer) { +static void read_source_target_build(ZipArchiveHandle zip, std::vector* log_buffer) { std::string metadata; if (!read_metadata_from_package(zip, &metadata)) { return; @@ -114,12 +114,12 @@ static void read_source_target_build(ZipArchiveHandle zip, std::vectorpush_back(android::base::StringPrintf("source_build: %d", source_build)); } } else if (android::base::StartsWith(str, "post-build-incremental")) { int target_build = parse_build_number(str); if (target_build != -1) { - log_buffer.push_back(android::base::StringPrintf("target_build: %d", target_build)); + log_buffer->push_back(android::base::StringPrintf("target_build: %d", target_build)); } } } @@ -308,8 +308,8 @@ static void log_max_temperature(int* max_temperature) { } // If the package contains an update binary, extract it and run it. -static int try_update_binary(const char* path, ZipArchiveHandle zip, bool* wipe_cache, - std::vector& log_buffer, int retry_count, +static int try_update_binary(const std::string& path, ZipArchiveHandle zip, bool* wipe_cache, + std::vector* log_buffer, int retry_count, int* max_temperature) { read_source_target_build(zip, log_buffer); @@ -452,7 +452,7 @@ static int try_update_binary(const char* path, ZipArchiveHandle zip, bool* wipe_ } else if (command == "log") { if (!args.empty()) { // Save the logging request from updater and write to last_install later. - log_buffer.push_back(args); + log_buffer->push_back(args); } else { LOG(ERROR) << "invalid \"log\" parameters: " << line; } @@ -547,78 +547,81 @@ bool verify_package_compatibility(ZipArchiveHandle package_zip) { return false; } -static int -really_install_package(const char *path, bool* wipe_cache, bool needs_mount, - std::vector& log_buffer, int retry_count, int* max_temperature) -{ - ui->SetBackground(RecoveryUI::INSTALLING_UPDATE); - ui->Print("Finding update package...\n"); - // Give verification half the progress bar... - ui->SetProgressType(RecoveryUI::DETERMINATE); - ui->ShowProgress(VERIFICATION_PROGRESS_FRACTION, VERIFICATION_PROGRESS_TIME); - LOG(INFO) << "Update location: " << path; - - // Map the update package into memory. - ui->Print("Opening update package...\n"); - - if (path && needs_mount) { - if (path[0] == '@') { - ensure_path_mounted(path+1); - } else { - ensure_path_mounted(path); - } - } - - MemMapping map; - if (sysMapFile(path, &map) != 0) { - LOG(ERROR) << "failed to map file"; - return INSTALL_CORRUPT; - } - - // Verify package. - if (!verify_package(map.addr, map.length)) { - log_buffer.push_back(android::base::StringPrintf("error: %d", kZipVerificationFailure)); - sysReleaseMap(&map); - return INSTALL_CORRUPT; +static int really_install_package(const std::string& path, bool* wipe_cache, bool needs_mount, + std::vector* log_buffer, int retry_count, + int* max_temperature) { + ui->SetBackground(RecoveryUI::INSTALLING_UPDATE); + ui->Print("Finding update package...\n"); + // Give verification half the progress bar... + ui->SetProgressType(RecoveryUI::DETERMINATE); + ui->ShowProgress(VERIFICATION_PROGRESS_FRACTION, VERIFICATION_PROGRESS_TIME); + LOG(INFO) << "Update location: " << path; + + // Map the update package into memory. + ui->Print("Opening update package...\n"); + + if (needs_mount) { + if (path[0] == '@') { + ensure_path_mounted(path.substr(1).c_str()); + } else { + ensure_path_mounted(path.c_str()); } + } - // Try to open the package. - ZipArchiveHandle zip; - int err = OpenArchiveFromMemory(map.addr, map.length, path, &zip); - if (err != 0) { - LOG(ERROR) << "Can't open " << path << " : " << ErrorCodeString(err); - log_buffer.push_back(android::base::StringPrintf("error: %d", kZipOpenFailure)); + MemMapping map; + if (sysMapFile(path.c_str(), &map) != 0) { + LOG(ERROR) << "failed to map file"; + return INSTALL_CORRUPT; + } - sysReleaseMap(&map); - CloseArchive(zip); - return INSTALL_CORRUPT; - } + // Verify package. + if (!verify_package(map.addr, map.length)) { + log_buffer->push_back(android::base::StringPrintf("error: %d", kZipVerificationFailure)); + sysReleaseMap(&map); + return INSTALL_CORRUPT; + } - // 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; - } + // Try to open the package. + ZipArchiveHandle zip; + int err = OpenArchiveFromMemory(map.addr, map.length, path.c_str(), &zip); + if (err != 0) { + LOG(ERROR) << "Can't open " << path << " : " << ErrorCodeString(err); + log_buffer->push_back(android::base::StringPrintf("error: %d", kZipOpenFailure)); - // Verify and install the contents of the package. - ui->Print("Installing update...\n"); - if (retry_count > 0) { - ui->Print("Retry attempt: %d\n", retry_count); - } - ui->SetEnableReboot(false); - int result = try_update_binary(path, zip, wipe_cache, log_buffer, retry_count, max_temperature); - ui->SetEnableReboot(true); - ui->Print("\n"); + sysReleaseMap(&map); + CloseArchive(zip); + return INSTALL_CORRUPT; + } + // 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 result; + return INSTALL_CORRUPT; + } + + // Verify and install the contents of the package. + ui->Print("Installing update...\n"); + if (retry_count > 0) { + ui->Print("Retry attempt: %d\n", retry_count); + } + ui->SetEnableReboot(false); + int result = try_update_binary(path, zip, wipe_cache, log_buffer, retry_count, max_temperature); + ui->SetEnableReboot(true); + ui->Print("\n"); + + sysReleaseMap(&map); + CloseArchive(zip); + return result; } -int install_package(const char* path, bool* wipe_cache, const char* install_file, bool needs_mount, - int retry_count) { +int install_package(const std::string& path, bool* wipe_cache, const std::string& install_file, + bool needs_mount, int retry_count) { + CHECK(!path.empty()); + CHECK(!install_file.empty()); + CHECK(wipe_cache != nullptr); + modified_flash = true; auto start = std::chrono::system_clock::now(); @@ -631,7 +634,7 @@ int install_package(const char* path, bool* wipe_cache, const char* install_file LOG(ERROR) << "failed to set up expected mounts for install; aborting"; result = INSTALL_ERROR; } else { - result = really_install_package(path, wipe_cache, needs_mount, log_buffer, retry_count, + result = really_install_package(path, wipe_cache, needs_mount, &log_buffer, retry_count, &max_temperature); } diff --git a/install.h b/install.h index 68f0a8d47..f3fda3051 100644 --- a/install.h +++ b/install.h @@ -23,10 +23,9 @@ enum { INSTALL_SUCCESS, INSTALL_ERROR, INSTALL_CORRUPT, INSTALL_NONE, INSTALL_SKIPPED, INSTALL_RETRY }; -// Install the package specified by root_path. If INSTALL_SUCCESS is -// returned and *wipe_cache is true on exit, caller should wipe the -// cache partition. -int install_package(const char* root_path, bool* wipe_cache, const char* install_file, +// Installs the given update package. If INSTALL_SUCCESS is returned and *wipe_cache is true on +// exit, caller should wipe the cache partition. +int install_package(const std::string& package, bool* wipe_cache, const std::string& install_file, bool needs_mount, int retry_count); // Verify the package by ota keys. Return true if the package is verified successfully, @@ -35,9 +34,9 @@ bool verify_package(const unsigned char* package_data, size_t package_size); // Read meta data file of the package, write its content in the string pointed by meta_data. // Return true if succeed, otherwise return false. -bool read_metadata_from_package(ZipArchiveHandle zip, std::string* meta_data); +bool read_metadata_from_package(ZipArchiveHandle zip, std::string* metadata); -// Verifes the compatibility info in a Treble-compatible package. Returns true directly if the +// Verifies the compatibility info in a Treble-compatible package. Returns true directly if the // entry doesn't exist. bool verify_package_compatibility(ZipArchiveHandle package_zip); -- cgit v1.2.3 From b656a154ea497c1a179079f082b0a0701453bec5 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Tue, 18 Apr 2017 23:54:29 -0700 Subject: 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 | 50 ++++++++------- 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, 114 insertions(+), 142 deletions(-) diff --git a/install.cpp b/install.cpp index d86f46caa..689f4a0c6 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 a2133b953..dfa215073 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 6a79bf31f..52f6d20a7 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 */ - - std::vector ranges; +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(); + }; + + 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 dc4b5d724..35e87fd56 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 2cfb6d301..5338f05c6 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 f4699664b..434ee25bf 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 4a1a49b97..df4e38cae 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 c09e267a5..749c86a1f 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; } -- cgit v1.2.3 From ed13819a0d197d6851dca623707344ef2cad850b Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 1 May 2017 22:30:39 -0700 Subject: fuse_sideload: Change the minimal block size to 4096. run_fuse_sideload() is passing the block size as the max_read option, so it will only handle a request that involves at most two blocks at a time. However, the minimal allowed value was set to 1024 prior to this CL, which is inconsistent with the kernel code (fs/fuse/inode.c) that sets it to the greater of 4096 and the passed-in max_read option. This would fail the calls with a block size / max_read less than 4096 due to the wrongly computed block indices. Note that we didn't observe real issue in practice, because we have been using 64 KiB block sizes for both of adb and sdcard sideload calls. The issue only shows up in my local CL (to come later) that uses 1024 block size in run_fuse_sideload() tests. Test: recovery_component_test Test: adb sideload with the new recovery image on angler Change-Id: Id9f0cfea13d0d193dcb7cd41a1553a23739545f2 --- Android.mk | 4 +- fuse_sideload.cpp | 278 +++++++++++++++++++------------------- tests/Android.mk | 1 + tests/component/sideload_test.cpp | 19 ++- 4 files changed, 161 insertions(+), 141 deletions(-) diff --git a/Android.mk b/Android.mk index 5ce5cd7dc..fbb0c665d 100644 --- a/Android.mk +++ b/Android.mk @@ -25,7 +25,9 @@ LOCAL_SRC_FILES := fuse_sideload.cpp LOCAL_CFLAGS := -Wall -Werror LOCAL_CFLAGS += -D_XOPEN_SOURCE -D_GNU_SOURCE LOCAL_MODULE := libfusesideload -LOCAL_STATIC_LIBRARIES := libcrypto +LOCAL_STATIC_LIBRARIES := \ + libcrypto \ + libbase include $(BUILD_STATIC_LIBRARY) # libmounts (static library) diff --git a/fuse_sideload.cpp b/fuse_sideload.cpp index 279a976ad..219374fdb 100644 --- a/fuse_sideload.cpp +++ b/fuse_sideload.cpp @@ -61,6 +61,9 @@ #include #include +#include + +#include #include #include "fuse_sideload.h" @@ -364,164 +367,163 @@ static int handle_read(void* data, struct fuse_data* fd, const struct fuse_in_he return NO_STATUS; } -int run_fuse_sideload(struct provider_vtab* vtab, void* cookie, - uint64_t file_size, uint32_t block_size) -{ - int result; - - // If something's already mounted on our mountpoint, try to remove - // it. (Mostly in case of a previous abnormal exit.) - umount2(FUSE_SIDELOAD_HOST_MOUNTPOINT, MNT_FORCE); - - if (block_size < 1024) { - fprintf(stderr, "block size (%u) is too small\n", block_size); - return -1; - } - if (block_size > (1<<22)) { // 4 MiB - fprintf(stderr, "block size (%u) is too large\n", block_size); - return -1; - } - - struct fuse_data fd; - memset(&fd, 0, sizeof(fd)); - fd.vtab = vtab; - fd.cookie = cookie; - fd.file_size = file_size; - fd.block_size = block_size; - fd.file_blocks = (file_size == 0) ? 0 : (((file_size-1) / block_size) + 1); - - if (fd.file_blocks > (1<<18)) { - fprintf(stderr, "file has too many blocks (%u)\n", fd.file_blocks); - result = -1; - goto done; - } - - fd.hashes = (uint8_t*)calloc(fd.file_blocks, SHA256_DIGEST_LENGTH); - if (fd.hashes == NULL) { - fprintf(stderr, "failed to allocate %d bites for hashes\n", - fd.file_blocks * SHA256_DIGEST_LENGTH); - result = -1; - goto done; - } - - fd.uid = getuid(); - fd.gid = getgid(); - - fd.curr_block = -1; - fd.block_data = (uint8_t*)malloc(block_size); - if (fd.block_data == NULL) { - fprintf(stderr, "failed to allocate %d bites for block_data\n", block_size); - result = -1; - goto done; +int run_fuse_sideload(struct provider_vtab* vtab, void* cookie, uint64_t file_size, + uint32_t block_size) { + // If something's already mounted on our mountpoint, try to remove it. (Mostly in case of a + // previous abnormal exit.) + umount2(FUSE_SIDELOAD_HOST_MOUNTPOINT, MNT_FORCE); + + // fs/fuse/inode.c in kernel code uses the greater of 4096 and the passed-in max_read. + if (block_size < 4096) { + fprintf(stderr, "block size (%u) is too small\n", block_size); + return -1; + } + if (block_size > (1 << 22)) { // 4 MiB + fprintf(stderr, "block size (%u) is too large\n", block_size); + return -1; + } + + struct fuse_data fd = {}; + fd.vtab = vtab; + fd.cookie = cookie; + fd.file_size = file_size; + fd.block_size = block_size; + fd.file_blocks = (file_size == 0) ? 0 : (((file_size - 1) / block_size) + 1); + + int result; + if (fd.file_blocks > (1 << 18)) { + fprintf(stderr, "file has too many blocks (%u)\n", fd.file_blocks); + result = -1; + goto done; + } + + fd.hashes = (uint8_t*)calloc(fd.file_blocks, SHA256_DIGEST_LENGTH); + if (fd.hashes == NULL) { + fprintf(stderr, "failed to allocate %d bites for hashes\n", + fd.file_blocks * SHA256_DIGEST_LENGTH); + result = -1; + goto done; + } + + fd.uid = getuid(); + fd.gid = getgid(); + + fd.curr_block = -1; + fd.block_data = (uint8_t*)malloc(block_size); + if (fd.block_data == NULL) { + fprintf(stderr, "failed to allocate %d bites for block_data\n", block_size); + result = -1; + goto done; + } + fd.extra_block = (uint8_t*)malloc(block_size); + if (fd.extra_block == NULL) { + fprintf(stderr, "failed to allocate %d bites for extra_block\n", block_size); + result = -1; + goto done; + } + + fd.ffd = open("/dev/fuse", O_RDWR); + if (fd.ffd < 0) { + perror("open /dev/fuse"); + result = -1; + goto done; + } + + { + std::string opts = android::base::StringPrintf( + "fd=%d,user_id=%d,group_id=%d,max_read=%u,allow_other,rootmode=040000", fd.ffd, fd.uid, + fd.gid, block_size); + + result = mount("/dev/fuse", FUSE_SIDELOAD_HOST_MOUNTPOINT, "fuse", + MS_NOSUID | MS_NODEV | MS_RDONLY | MS_NOEXEC, opts.c_str()); + if (result < 0) { + perror("mount"); + goto done; } - fd.extra_block = (uint8_t*)malloc(block_size); - if (fd.extra_block == NULL) { - fprintf(stderr, "failed to allocate %d bites for extra_block\n", block_size); + } + + uint8_t request_buffer[sizeof(struct fuse_in_header) + PATH_MAX * 8]; + for (;;) { + ssize_t len = TEMP_FAILURE_RETRY(read(fd.ffd, request_buffer, sizeof(request_buffer))); + if (len == -1) { + perror("read request"); + if (errno == ENODEV) { result = -1; - goto done; + break; + } + continue; } - fd.ffd = open("/dev/fuse", O_RDWR); - if (fd.ffd < 0) { - perror("open /dev/fuse"); - result = -1; - goto done; + if (static_cast(len) < sizeof(struct fuse_in_header)) { + fprintf(stderr, "request too short: len=%zd\n", len); + continue; } - char opts[256]; - snprintf(opts, sizeof(opts), - ("fd=%d,user_id=%d,group_id=%d,max_read=%u," - "allow_other,rootmode=040000"), - fd.ffd, fd.uid, fd.gid, block_size); - - result = mount("/dev/fuse", FUSE_SIDELOAD_HOST_MOUNTPOINT, - "fuse", MS_NOSUID | MS_NODEV | MS_RDONLY | MS_NOEXEC, opts); - if (result < 0) { - perror("mount"); - goto done; - } - uint8_t request_buffer[sizeof(struct fuse_in_header) + PATH_MAX*8]; - for (;;) { - ssize_t len = TEMP_FAILURE_RETRY(read(fd.ffd, request_buffer, sizeof(request_buffer))); - if (len == -1) { - perror("read request"); - if (errno == ENODEV) { - result = -1; - break; - } - continue; - } - - if ((size_t)len < sizeof(struct fuse_in_header)) { - fprintf(stderr, "request too short: len=%zu\n", (size_t)len); - continue; - } + struct fuse_in_header* hdr = reinterpret_cast(request_buffer); + void* data = request_buffer + sizeof(struct fuse_in_header); - struct fuse_in_header* hdr = (struct fuse_in_header*) request_buffer; - void* data = request_buffer + sizeof(struct fuse_in_header); + result = -ENOSYS; - result = -ENOSYS; + switch (hdr->opcode) { + case FUSE_INIT: + result = handle_init(data, &fd, hdr); + break; - switch (hdr->opcode) { - case FUSE_INIT: - result = handle_init(data, &fd, hdr); - break; + case FUSE_LOOKUP: + result = handle_lookup(data, &fd, hdr); + break; - case FUSE_LOOKUP: - result = handle_lookup(data, &fd, hdr); - break; + case FUSE_GETATTR: + result = handle_getattr(data, &fd, hdr); + break; - case FUSE_GETATTR: - result = handle_getattr(data, &fd, hdr); - break; + case FUSE_OPEN: + result = handle_open(data, &fd, hdr); + break; - case FUSE_OPEN: - result = handle_open(data, &fd, hdr); - break; + case FUSE_READ: + result = handle_read(data, &fd, hdr); + break; - case FUSE_READ: - result = handle_read(data, &fd, hdr); - break; + case FUSE_FLUSH: + result = handle_flush(data, &fd, hdr); + break; - case FUSE_FLUSH: - result = handle_flush(data, &fd, hdr); - break; + case FUSE_RELEASE: + result = handle_release(data, &fd, hdr); + break; - case FUSE_RELEASE: - result = handle_release(data, &fd, hdr); - break; - - default: - fprintf(stderr, "unknown fuse request opcode %d\n", hdr->opcode); - break; - } + default: + fprintf(stderr, "unknown fuse request opcode %d\n", hdr->opcode); + break; + } - if (result == NO_STATUS_EXIT) { - result = 0; - break; - } + if (result == NO_STATUS_EXIT) { + result = 0; + break; + } - if (result != NO_STATUS) { - struct fuse_out_header outhdr; - outhdr.len = sizeof(outhdr); - outhdr.error = result; - outhdr.unique = hdr->unique; - TEMP_FAILURE_RETRY(write(fd.ffd, &outhdr, sizeof(outhdr))); - } + if (result != NO_STATUS) { + struct fuse_out_header outhdr; + outhdr.len = sizeof(outhdr); + outhdr.error = result; + outhdr.unique = hdr->unique; + TEMP_FAILURE_RETRY(write(fd.ffd, &outhdr, sizeof(outhdr))); } + } - done: - fd.vtab->close(fd.cookie); +done: + fd.vtab->close(fd.cookie); - result = umount2(FUSE_SIDELOAD_HOST_MOUNTPOINT, MNT_DETACH); - if (result < 0) { - printf("fuse_sideload umount failed: %s\n", strerror(errno)); - } + result = umount2(FUSE_SIDELOAD_HOST_MOUNTPOINT, MNT_DETACH); + if (result < 0) { + printf("fuse_sideload umount failed: %s\n", strerror(errno)); + } - if (fd.ffd) close(fd.ffd); - free(fd.hashes); - free(fd.block_data); - free(fd.extra_block); + if (fd.ffd) close(fd.ffd); + free(fd.hashes); + free(fd.block_data); + free(fd.extra_block); - return result; + return result; } diff --git a/tests/Android.mk b/tests/Android.mk index 4e125ccce..02a240127 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -126,6 +126,7 @@ LOCAL_STATIC_LIBRARIES := \ libimgpatch \ libbsdiff \ libbspatch \ + libfusesideload \ libotafault \ librecovery \ libupdater \ diff --git a/tests/component/sideload_test.cpp b/tests/component/sideload_test.cpp index ea93e9b84..40cfc6975 100644 --- a/tests/component/sideload_test.cpp +++ b/tests/component/sideload_test.cpp @@ -13,9 +13,24 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + #include + #include -TEST(SideloadTest, fusedevice) { - ASSERT_NE(-1, access("/dev/fuse", R_OK | W_OK)); +#include "fuse_sideload.h" + +TEST(SideloadTest, fuse_device) { + ASSERT_EQ(0, access("/dev/fuse", R_OK | W_OK)); +} + +TEST(SideloadTest, run_fuse_sideload_wrong_parameters) { + provider_vtab vtab; + vtab.close = [](void*) {}; + + ASSERT_EQ(-1, run_fuse_sideload(&vtab, nullptr, 4096, 4095)); + ASSERT_EQ(-1, run_fuse_sideload(&vtab, nullptr, 4096, (1 << 22) + 1)); + + // Too many blocks. + ASSERT_EQ(-1, run_fuse_sideload(&vtab, nullptr, ((1 << 18) + 1) * 4096, 4096)); } -- cgit v1.2.3 From e0c88793d184648a8d08b5d925b21cc6c9623ffc Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Tue, 2 May 2017 16:07:18 -0700 Subject: Add a default error code when updater script aborts We didn't report error/cause codes unless there's an explict "Abort()" call inside the updater script. As a result, some cause codes set by ErrorAbort() didn't show up in last_install. To fix the issue, add a default error code when the script terminates abnormally (i.e. with non zero status). Bug: 37912405 Test: error/cause code shows up in last_install when argument parsing fails Change-Id: Ic6d3bd1855b853aeaa0760071e593a00cf6f0209 --- error_code.h | 1 + updater/updater.cpp | 16 +++++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/error_code.h b/error_code.h index cde4ee6de..0e79c87ca 100644 --- a/error_code.h +++ b/error_code.h @@ -24,6 +24,7 @@ enum ErrorCode { kZipOpenFailure, kBootreasonInBlacklist, kPackageCompatibilityFailure, + kScriptExecutionFailure, }; enum CauseCode { diff --git a/updater/updater.cpp b/updater/updater.cpp index c09e267a5..37e003e66 100644 --- a/updater/updater.cpp +++ b/updater/updater.cpp @@ -193,13 +193,15 @@ int main(int argc, char** argv) { } } - if (state.error_code != kNoError) { - fprintf(cmd_pipe, "log error: %d\n", state.error_code); - // Cause code should provide additional information about the abort; - // report only when an error exists. - if (state.cause_code != kNoCause) { - fprintf(cmd_pipe, "log cause: %d\n", state.cause_code); - } + // Installation has been aborted. Set the error code to kScriptExecutionFailure unless + // a more specific code has been set in errmsg. + if (state.error_code == kNoError) { + state.error_code = kScriptExecutionFailure; + } + fprintf(cmd_pipe, "log error: %d\n", state.error_code); + // Cause code should provide additional information about the abort. + if (state.cause_code != kNoCause) { + fprintf(cmd_pipe, "log cause: %d\n", state.cause_code); } if (updater_info.package_zip) { -- cgit v1.2.3 From ec9706738f35a859f66fd0758b73381055804f63 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 3 May 2017 11:00:48 -0700 Subject: Remove EXPAND/STRINGIFY macros. They are error-prone by putting anything into a string (e.g. EXPAND(RECOVERY_API_VERSION) would become "RECOVER_API_VERSION" if we forgot to pass -DRECOVERY_API_VERSION=3). RECOVERY_API_VERSION is the only user (in bootable/recovery) that gets stringified. Assign it to a typed var and sanity check the value. Don't see other reference to the macros from device-specific recovery directories (they can still define that locally if really needed). Test: recovery_component_test Test: Sideload an OTA on angler and marlin respectively. Change-Id: I358bbdf8f0a99db5ce4c7bc2fdcafe8013501b64 --- common.h | 4 ++-- install.cpp | 2 +- recovery.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common.h b/common.h index 62fb1324b..87b84772d 100644 --- a/common.h +++ b/common.h @@ -22,8 +22,8 @@ #include -#define STRINGIFY(x) #x -#define EXPAND(x) STRINGIFY(x) +static constexpr int kRecoveryApiVersion = RECOVERY_API_VERSION; // Defined in Android.mk. +static_assert(kRecoveryApiVersion >= 3, "Invalid recovery API version."); class RecoveryUI; diff --git a/install.cpp b/install.cpp index 689f4a0c6..2cc06603b 100644 --- a/install.cpp +++ b/install.cpp @@ -287,7 +287,7 @@ int update_binary_command(const std::string& path, ZipArchiveHandle zip, int ret *cmd = { binary, - EXPAND(RECOVERY_API_VERSION), // defined in Android.mk + std::to_string(kRecoveryApiVersion), std::to_string(status_fd), path, }; diff --git a/recovery.cpp b/recovery.cpp index 944c24086..6dd985831 100644 --- a/recovery.cpp +++ b/recovery.cpp @@ -1501,7 +1501,7 @@ int main(int argc, char **argv) { property_list(print_property, NULL); printf("\n"); - ui->Print("Supported API: %d\n", RECOVERY_API_VERSION); + ui->Print("Supported API: %d\n", kRecoveryApiVersion); int status = INSTALL_SUCCESS; -- cgit v1.2.3 From f38401a27c8e34f6ddb42760dc977dcd62a62d85 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Wed, 3 May 2017 15:31:12 -0700 Subject: Update the comment for obsolete symlink handling . Symlink is a filebased OTA feature, and the corresponding updater function has been removed in https://android-review.googlesource.com/#/c/350357/. Also the only place where we call "package_extract_dir()" is to unpack some bootloader dir in vendor's code. We plan to remove it also in a separate bug. Bug: 31917448 Test: mma Change-Id: I3986d60958e64e0d5d8fa5f5bd508c579fb7fa2c --- otautil/ZipUtil.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/otautil/ZipUtil.cpp b/otautil/ZipUtil.cpp index 714c956ed..9cc97e499 100644 --- a/otautil/ZipUtil.cpp +++ b/otautil/ZipUtil.cpp @@ -74,7 +74,6 @@ bool ExtractPackageRecursive(ZipArchiveHandle zip, const std::string& zip_path, if (path.back() == '/') { continue; } - //TODO(b/31917448) handle the symlink. if (dirCreateHierarchy(path.c_str(), UNZIP_DIRMODE, timestamp, true, sehnd) != 0) { LOG(ERROR) << "failed to create dir for " << path; -- cgit v1.2.3 From 8be0f39fec7f26164fd0791ff6d15bde65fc849c Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Thu, 4 May 2017 00:29:31 +0000 Subject: Revert "Remove EXPAND/STRINGIFY macros." This reverts commit ec9706738f35a859f66fd0758b73381055804f63. Reason for revert: It's not a good idea to put RECOVERY_API_VERSION in common.h, which might be included by device-specific codes (but with RECOVERY_API_VERSION undefined). Change-Id: I9feb9c64a5af3e9165164622a59b043aa28a8b8c --- common.h | 4 ++-- install.cpp | 2 +- recovery.cpp | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/common.h b/common.h index 87b84772d..62fb1324b 100644 --- a/common.h +++ b/common.h @@ -22,8 +22,8 @@ #include -static constexpr int kRecoveryApiVersion = RECOVERY_API_VERSION; // Defined in Android.mk. -static_assert(kRecoveryApiVersion >= 3, "Invalid recovery API version."); +#define STRINGIFY(x) #x +#define EXPAND(x) STRINGIFY(x) class RecoveryUI; diff --git a/install.cpp b/install.cpp index 2cc06603b..689f4a0c6 100644 --- a/install.cpp +++ b/install.cpp @@ -287,7 +287,7 @@ int update_binary_command(const std::string& path, ZipArchiveHandle zip, int ret *cmd = { binary, - std::to_string(kRecoveryApiVersion), + EXPAND(RECOVERY_API_VERSION), // defined in Android.mk std::to_string(status_fd), path, }; diff --git a/recovery.cpp b/recovery.cpp index 6dd985831..944c24086 100644 --- a/recovery.cpp +++ b/recovery.cpp @@ -1501,7 +1501,7 @@ int main(int argc, char **argv) { property_list(print_property, NULL); printf("\n"); - ui->Print("Supported API: %d\n", kRecoveryApiVersion); + ui->Print("Supported API: %d\n", RECOVERY_API_VERSION); int status = INSTALL_SUCCESS; -- cgit v1.2.3 From adeb41a8c0da3122a2907acb4aafd7ff9bce26af Mon Sep 17 00:00:00 2001 From: Jin Qian Date: Fri, 28 Apr 2017 16:15:13 -0700 Subject: recovery: update mkfs.f2fs args to match f2fs-tools 1.8.0 mkfs.f2fs in 1.8.0 returns error if number of sectors is 0. Skip this argument to let mkfs detect device size. 0 sector is also not necessary for 1.4.1. Test: format userdata to f2fs and boot Bug: 37758867 Change-Id: If120988dfb678596c973d183572f870eb0b72a27 --- roots.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/roots.cpp b/roots.cpp index 6e5ef9810..727736b70 100644 --- a/roots.cpp +++ b/roots.cpp @@ -232,14 +232,14 @@ int format_volume(const char* volume, const char* directory) { << ") not supported on " << v->fs_type; return -1; } - char *num_sectors; - if (asprintf(&num_sectors, "%zd", length / 512) <= 0) { + char *num_sectors = nullptr; + if (length >= 512 && asprintf(&num_sectors, "%zd", length / 512) <= 0) { LOG(ERROR) << "format_volume: failed to create " << v->fs_type << " command for " << v->blk_device; return -1; } const char *f2fs_path = "/sbin/mkfs.f2fs"; - const char* const f2fs_argv[] = {"mkfs.f2fs", "-t", "-d1", v->blk_device, num_sectors, NULL}; + const char* const f2fs_argv[] = {"mkfs.f2fs", "-t", "-d1", v->blk_device, num_sectors, nullptr}; result = exec_cmd(f2fs_path, (char* const*)f2fs_argv); free(num_sectors); -- cgit v1.2.3 From b168f5f8576b32b9c1bd6c5823dc56b09d497d11 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Sun, 16 Apr 2017 15:22:13 -0700 Subject: recovery: Use libverifier instead of rebuilding the sources. Test: mmma bootable/recovery Change-Id: Ie8cec009b00c121948179518ba9cbc26a82352bf --- Android.mk | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Android.mk b/Android.mk index fbb0c665d..0c3d4fdf2 100644 --- a/Android.mk +++ b/Android.mk @@ -69,7 +69,6 @@ include $(CLEAR_VARS) LOCAL_SRC_FILES := \ adb_install.cpp \ - asn1_decoder.cpp \ device.cpp \ fuse_sdcard_provider.cpp \ recovery.cpp \ @@ -77,7 +76,6 @@ LOCAL_SRC_FILES := \ rotate_logs.cpp \ screen_ui.cpp \ ui.cpp \ - verifier.cpp \ wear_ui.cpp \ wear_touch.cpp \ @@ -99,6 +97,7 @@ LOCAL_C_INCLUDES += \ LOCAL_STATIC_LIBRARIES := \ librecovery \ + libverifier \ libbatterymonitor \ libbootloader_message \ libext4_utils \ @@ -173,7 +172,6 @@ include $(BUILD_EXECUTABLE) # =============================== include $(CLEAR_VARS) LOCAL_MODULE := libverifier -LOCAL_MODULE_TAGS := tests LOCAL_SRC_FILES := \ asn1_decoder.cpp \ verifier.cpp -- cgit v1.2.3 From 00d5757186c279ba5e8a52a6f5209be3e7152025 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Tue, 2 May 2017 15:48:54 -0700 Subject: Add a binary path param to update_binary_command(). This allows writing native tests for non-A/B update_binary_command(). Prior to this CL, it was extracting the updater to a hard-coded location (/tmp/update_binary) that's not available under the test environment. Test: recovery_component_test on angler and marlin respectively. Test: Sideload OTA packages on angler and marlin respectively. Change-Id: I78b9cc211d90c0a16a84e94e339b65759300e2a8 --- install.cpp | 44 +++++++++++---------- private/install.h | 8 +++- tests/component/install_test.cpp | 84 +++++++++++++++++++++++++++++++++++----- 3 files changed, 103 insertions(+), 33 deletions(-) diff --git a/install.cpp b/install.cpp index 689f4a0c6..a1f2e4fbd 100644 --- a/install.cpp +++ b/install.cpp @@ -51,6 +51,7 @@ #include "error_code.h" #include "otautil/SysUtil.h" #include "otautil/ThermalUtil.h" +#include "private/install.h" #include "roots.h" #include "ui.h" #include "verifier.h" @@ -125,12 +126,6 @@ static void read_source_target_build(ZipArchiveHandle zip, std::vector* cmd); - #ifdef AB_OTA_UPDATER // Parses the metadata of the OTA package in |zip| and checks whether we are @@ -211,8 +206,9 @@ static int check_newer_ab_build(ZipArchiveHandle zip) { return 0; } -int update_binary_command(const std::string& path, ZipArchiveHandle zip, int /* retry_count */, - int status_fd, std::vector* cmd) { +int update_binary_command(const std::string& package, ZipArchiveHandle zip, + const std::string& binary_path, int /* retry_count */, int status_fd, + std::vector* cmd) { CHECK(cmd != nullptr); int ret = check_newer_ab_build(zip); if (ret != 0) { @@ -246,8 +242,8 @@ int update_binary_command(const std::string& path, ZipArchiveHandle zip, int /* } long payload_offset = payload_entry.offset; *cmd = { - "/sbin/update_engine_sideload", - "--payload=file://" + path, + binary_path, + "--payload=file://" + package, android::base::StringPrintf("--offset=%ld", payload_offset), "--headers=" + std::string(payload_properties.begin(), payload_properties.end()), android::base::StringPrintf("--status_fd=%d", status_fd), @@ -257,8 +253,9 @@ int update_binary_command(const std::string& path, ZipArchiveHandle zip, int /* #else // !AB_OTA_UPDATER -int update_binary_command(const std::string& path, ZipArchiveHandle zip, int retry_count, - int status_fd, std::vector* cmd) { +int update_binary_command(const std::string& package, ZipArchiveHandle zip, + const std::string& binary_path, int retry_count, int status_fd, + std::vector* cmd) { CHECK(cmd != nullptr); // On traditional updates we extract the update binary from the package. @@ -270,11 +267,10 @@ int update_binary_command(const std::string& path, ZipArchiveHandle zip, int ret return INSTALL_CORRUPT; } - const char* binary = "/tmp/update_binary"; - unlink(binary); - int fd = creat(binary, 0755); + unlink(binary_path.c_str()); + int fd = creat(binary_path.c_str(), 0755); if (fd == -1) { - PLOG(ERROR) << "Failed to create " << binary; + PLOG(ERROR) << "Failed to create " << binary_path; return INSTALL_ERROR; } @@ -286,10 +282,10 @@ int update_binary_command(const std::string& path, ZipArchiveHandle zip, int ret } *cmd = { - binary, + binary_path, EXPAND(RECOVERY_API_VERSION), // defined in Android.mk std::to_string(status_fd), - path, + package, }; if (retry_count > 0) { cmd->push_back("retry"); @@ -308,7 +304,7 @@ static void log_max_temperature(int* max_temperature) { } // If the package contains an update binary, extract it and run it. -static int try_update_binary(const std::string& path, ZipArchiveHandle zip, bool* wipe_cache, +static int try_update_binary(const std::string& package, ZipArchiveHandle zip, bool* wipe_cache, std::vector* log_buffer, int retry_count, int* max_temperature) { read_source_target_build(zip, log_buffer); @@ -317,7 +313,13 @@ static int try_update_binary(const std::string& path, ZipArchiveHandle zip, bool pipe(pipefd); std::vector args; - int ret = update_binary_command(path, zip, retry_count, pipefd[1], &args); +#ifdef AB_OTA_UPDATER + int ret = update_binary_command(package, zip, "/sbin/update_engine_sideload", retry_count, + pipefd[1], &args); +#else + int ret = update_binary_command(package, zip, "/tmp/update-binary", retry_count, pipefd[1], + &args); +#endif if (ret) { close(pipefd[0]); close(pipefd[1]); @@ -472,7 +474,7 @@ static int try_update_binary(const std::string& path, ZipArchiveHandle zip, bool return INSTALL_RETRY; } if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { - LOG(ERROR) << "Error in " << path << " (Status " << WEXITSTATUS(status) << ")"; + LOG(ERROR) << "Error in " << package << " (Status " << WEXITSTATUS(status) << ")"; return INSTALL_ERROR; } diff --git a/private/install.h b/private/install.h index 12d303b01..ef64bd41d 100644 --- a/private/install.h +++ b/private/install.h @@ -23,5 +23,9 @@ #include -int update_binary_command(const std::string& path, ZipArchiveHandle zip, int retry_count, - int status_fd, std::vector* cmd); +// Extract the update binary from the open zip archive |zip| located at |package| to |binary_path|. +// Store the command line that should be called into |cmd|. The |status_fd| is the file descriptor +// the child process should use to report back the progress of the update. +int update_binary_command(const std::string& package, ZipArchiveHandle zip, + const std::string& binary_path, int retry_count, int status_fd, + std::vector* cmd); diff --git a/tests/component/install_test.cpp b/tests/component/install_test.cpp index 40201d76f..968196fc0 100644 --- a/tests/component/install_test.cpp +++ b/tests/component/install_test.cpp @@ -15,6 +15,8 @@ */ #include +#include +#include #include #include @@ -225,18 +227,62 @@ TEST(InstallTest, update_binary_command_smoke) { ZipArchiveHandle zip; ASSERT_EQ(0, OpenArchive(temp_file.path, &zip)); + ZipString payload_name("payload.bin"); + ZipEntry payload_entry; + ASSERT_EQ(0, FindEntry(zip, payload_name, &payload_entry)); int status_fd = 10; - std::string path = "/path/to/update.zip"; + std::string package = "/path/to/update.zip"; + std::string binary_path = "/sbin/update_engine_sideload"; std::vector cmd; - ASSERT_EQ(0, update_binary_command(path, zip, 0, status_fd, &cmd)); - ASSERT_EQ("/sbin/update_engine_sideload", cmd[0]); - ASSERT_EQ("--payload=file://" + path, cmd[1]); + ASSERT_EQ(0, update_binary_command(package, zip, binary_path, 0, status_fd, &cmd)); + ASSERT_EQ(5U, cmd.size()); + ASSERT_EQ(binary_path, cmd[0]); + ASSERT_EQ("--payload=file://" + package, cmd[1]); + ASSERT_EQ("--offset=" + std::to_string(payload_entry.offset), cmd[2]); ASSERT_EQ("--headers=" + properties, cmd[3]); ASSERT_EQ("--status_fd=" + std::to_string(status_fd), cmd[4]); CloseArchive(zip); #else - // Cannot test update_binary_command() because it tries to extract update-binary to /tmp. - GTEST_LOG_(INFO) << "Test skipped on non-A/B device."; + TemporaryFile temp_file; + FILE* zip_file = fdopen(temp_file.fd, "w"); + ZipWriter writer(zip_file); + static constexpr const char* UPDATE_BINARY_NAME = "META-INF/com/google/android/update-binary"; + ASSERT_EQ(0, writer.StartEntry(UPDATE_BINARY_NAME, kCompressStored)); + ASSERT_EQ(0, writer.FinishEntry()); + ASSERT_EQ(0, writer.Finish()); + ASSERT_EQ(0, fclose(zip_file)); + + ZipArchiveHandle zip; + ASSERT_EQ(0, OpenArchive(temp_file.path, &zip)); + int status_fd = 10; + std::string package = "/path/to/update.zip"; + TemporaryDir td; + std::string binary_path = std::string(td.path) + "/update_binary"; + std::vector cmd; + ASSERT_EQ(0, update_binary_command(package, zip, binary_path, 0, status_fd, &cmd)); + ASSERT_EQ(4U, cmd.size()); + ASSERT_EQ(binary_path, cmd[0]); + ASSERT_EQ("3", cmd[1]); // RECOVERY_API_VERSION + ASSERT_EQ(std::to_string(status_fd), cmd[2]); + ASSERT_EQ(package, cmd[3]); + struct stat sb; + ASSERT_EQ(0, stat(binary_path.c_str(), &sb)); + ASSERT_EQ(static_cast(0755), sb.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO)); + + // With non-zero retry count. update_binary will be removed automatically. + cmd.clear(); + ASSERT_EQ(0, update_binary_command(package, zip, binary_path, 2, status_fd, &cmd)); + ASSERT_EQ(5U, cmd.size()); + ASSERT_EQ(binary_path, cmd[0]); + ASSERT_EQ("3", cmd[1]); // RECOVERY_API_VERSION + ASSERT_EQ(std::to_string(status_fd), cmd[2]); + ASSERT_EQ(package, cmd[3]); + ASSERT_EQ("retry", cmd[4]); + sb = {}; + ASSERT_EQ(0, stat(binary_path.c_str(), &sb)); + ASSERT_EQ(static_cast(0755), sb.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO)); + + CloseArchive(zip); #endif // AB_OTA_UPDATER } @@ -267,12 +313,30 @@ TEST(InstallTest, update_binary_command_invalid) { ZipArchiveHandle zip; ASSERT_EQ(0, OpenArchive(temp_file.path, &zip)); int status_fd = 10; - std::string path = "/path/to/update.zip"; + std::string package = "/path/to/update.zip"; + std::string binary_path = "/sbin/update_engine_sideload"; std::vector cmd; - ASSERT_EQ(INSTALL_CORRUPT, update_binary_command(path, zip, 0, status_fd, &cmd)); + ASSERT_EQ(INSTALL_CORRUPT, update_binary_command(package, zip, binary_path, 0, status_fd, &cmd)); CloseArchive(zip); #else - // Cannot test update_binary_command() because it tries to extract update-binary to /tmp. - GTEST_LOG_(INFO) << "Test skipped on non-A/B device."; + TemporaryFile temp_file; + FILE* zip_file = fdopen(temp_file.fd, "w"); + ZipWriter writer(zip_file); + // The archive must have something to be opened correctly. + ASSERT_EQ(0, writer.StartEntry("dummy_entry", 0)); + ASSERT_EQ(0, writer.FinishEntry()); + ASSERT_EQ(0, writer.Finish()); + ASSERT_EQ(0, fclose(zip_file)); + + // Missing update binary. + ZipArchiveHandle zip; + ASSERT_EQ(0, OpenArchive(temp_file.path, &zip)); + int status_fd = 10; + std::string package = "/path/to/update.zip"; + TemporaryDir td; + std::string binary_path = std::string(td.path) + "/update_binary"; + std::vector cmd; + ASSERT_EQ(INSTALL_CORRUPT, update_binary_command(package, zip, binary_path, 0, status_fd, &cmd)); + CloseArchive(zip); #endif // AB_OTA_UPDATER } -- cgit v1.2.3 From f3ae55a167892dc486379a0444bfb8a38ce1f7f2 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Tue, 9 May 2017 15:49:36 -0700 Subject: otautil: Android.mk -> Android.bp Test: flash and boot recovery on internal angler Change-Id: Id8845b4b422d0078b251333eb6d30ce14771ef10 --- Android.bp | 3 +++ Android.mk | 1 - otautil/Android.bp | 34 ++++++++++++++++++++++++++++++++++ otautil/Android.mk | 33 --------------------------------- 4 files changed, 37 insertions(+), 34 deletions(-) create mode 100644 Android.bp create mode 100644 otautil/Android.bp delete mode 100644 otautil/Android.mk diff --git a/Android.bp b/Android.bp new file mode 100644 index 000000000..f919ebc83 --- /dev/null +++ b/Android.bp @@ -0,0 +1,3 @@ +subdirs = [ + "otautil", +] diff --git a/Android.mk b/Android.mk index fbb0c665d..bab9aa316 100644 --- a/Android.mk +++ b/Android.mk @@ -192,7 +192,6 @@ include \ $(LOCAL_PATH)/minadbd/Android.mk \ $(LOCAL_PATH)/minui/Android.mk \ $(LOCAL_PATH)/otafault/Android.mk \ - $(LOCAL_PATH)/otautil/Android.mk \ $(LOCAL_PATH)/tests/Android.mk \ $(LOCAL_PATH)/tools/Android.mk \ $(LOCAL_PATH)/uncrypt/Android.mk \ diff --git a/otautil/Android.bp b/otautil/Android.bp new file mode 100644 index 000000000..0b2314374 --- /dev/null +++ b/otautil/Android.bp @@ -0,0 +1,34 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +cc_library_static { + name: "libotautil", + + srcs: [ + "SysUtil.cpp", + "DirUtil.cpp", + "ZipUtil.cpp", + "ThermalUtil.cpp", + ], + + static_libs: [ + "libselinux", + "libbase", + ], + + cflags: [ + "-Werror", + "-Wall", + ], +} diff --git a/otautil/Android.mk b/otautil/Android.mk deleted file mode 100644 index f7ca9a9ee..000000000 --- a/otautil/Android.mk +++ /dev/null @@ -1,33 +0,0 @@ -# Copyright (C) 2016 The Android Open Source Project -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -LOCAL_PATH := $(call my-dir) -include $(CLEAR_VARS) - -LOCAL_SRC_FILES := \ - SysUtil.cpp \ - DirUtil.cpp \ - ZipUtil.cpp \ - ThermalUtil.cpp - -LOCAL_STATIC_LIBRARIES := \ - libselinux \ - libbase - -LOCAL_MODULE := libotautil -LOCAL_CFLAGS := \ - -Werror \ - -Wall - -include $(BUILD_STATIC_LIBRARY) -- cgit v1.2.3 From 57130c45f85d68b4420e4dccb9c972c7f8946031 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 10 May 2017 12:11:21 -0700 Subject: recovery: Skip "/" in setup_install_mounts(). We don't want to do anything for "/" when preparing for an install. Bug: 36686818 Test: adb sideload on angler/marlin respectively. Change-Id: Id854dd0a743a0e163a8f13baf2514105091ddc67 --- roots.cpp | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/roots.cpp b/roots.cpp index 727736b70..9b4270256 100644 --- a/roots.cpp +++ b/roots.cpp @@ -260,26 +260,29 @@ int format_volume(const char* volume) { } int setup_install_mounts() { - if (fstab == NULL) { - LOG(ERROR) << "can't set up install mounts: no fstab loaded"; - return -1; - } - for (int i = 0; i < fstab->num_entries; ++i) { - Volume* v = fstab->recs + i; + if (fstab == nullptr) { + LOG(ERROR) << "can't set up install mounts: no fstab loaded"; + return -1; + } + for (int i = 0; i < fstab->num_entries; ++i) { + const Volume* v = fstab->recs + i; - if (strcmp(v->mount_point, "/tmp") == 0 || - strcmp(v->mount_point, "/cache") == 0) { - if (ensure_path_mounted(v->mount_point) != 0) { - LOG(ERROR) << "failed to mount " << v->mount_point; - return -1; - } + // We don't want to do anything with "/". + if (strcmp(v->mount_point, "/") == 0) { + continue; + } - } else { - if (ensure_path_unmounted(v->mount_point) != 0) { - LOG(ERROR) << "failed to unmount " << v->mount_point; - return -1; - } - } + if (strcmp(v->mount_point, "/tmp") == 0 || strcmp(v->mount_point, "/cache") == 0) { + if (ensure_path_mounted(v->mount_point) != 0) { + LOG(ERROR) << "failed to mount " << v->mount_point; + return -1; + } + } else { + if (ensure_path_unmounted(v->mount_point) != 0) { + LOG(ERROR) << "failed to unmount " << v->mount_point; + return -1; + } } - return 0; + } + return 0; } -- cgit v1.2.3 From 0167d4c382a697a81638af9b84f93ca50a78217f Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Thu, 11 May 2017 14:44:15 -0700 Subject: Don't write to /sys/class/android_usb/android0/enable with configfs. USB configfs doesn't use /s/c/a/a/enable. Trying to set that gives confusing message on screen when sideloading. Bug: 37713851 Test: adb sideload on device using with configfs. Test: adb sideload on marlin. Change-Id: Ifa55f90c2a5fe6bf9e7cee95882de9f6de686d73 --- adb_install.cpp | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/adb_install.cpp b/adb_install.cpp index e7d7758f9..ac0130651 100644 --- a/adb_install.cpp +++ b/adb_install.cpp @@ -26,7 +26,10 @@ #include #include +#include +#include #include +#include #include "common.h" #include "fuse_sideload.h" @@ -34,16 +37,22 @@ #include "ui.h" static void set_usb_driver(bool enabled) { - int fd = open("/sys/class/android_usb/android0/enable", O_WRONLY); - if (fd < 0) { - ui->Print("failed to open driver control: %s\n", strerror(errno)); + // USB configfs doesn't use /s/c/a/a/enable. + if (android::base::GetBoolProperty("sys.usb.configfs", false)) { return; } - if (TEMP_FAILURE_RETRY(write(fd, enabled ? "1" : "0", 1)) == -1) { - ui->Print("failed to set driver control: %s\n", strerror(errno)); + + static constexpr const char* USB_DRIVER_CONTROL = "/sys/class/android_usb/android0/enable"; + android::base::unique_fd fd(open(USB_DRIVER_CONTROL, O_WRONLY)); + if (fd == -1) { + PLOG(ERROR) << "Failed to open driver control"; + return; } - if (close(fd) < 0) { - ui->Print("failed to close driver control: %s\n", strerror(errno)); + // Not using android::base::WriteStringToFile since that will open with O_CREAT and give EPERM + // when USB_DRIVER_CONTROL doesn't exist. When it gives EPERM, we don't know whether that's due + // to non-existent USB_DRIVER_CONTROL or indeed a permission issue. + if (!android::base::WriteStringToFd(enabled ? "1" : "0", fd)) { + PLOG(ERROR) << "Failed to set driver control"; } } -- cgit v1.2.3 From 397a8137a0074ade60eee5efb32cc5a46c8af73c Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Fri, 12 May 2017 11:02:27 -0700 Subject: updater: Update the mkfs.f2fs argument to match f2fs-tools 1.8.0. Commit adeb41a8c0da3122a2907acb4aafd7ff9bce26af has switched the argument for recovery. This CL handles the case for updater. Note that there's a chance the updater may run against the old recovery (and f2fs 1.4.1 binary). Not sending a 0-sector argument to f2fs 1.4.1 also works. Bug: 37758867 Test: Make an OTA package that calls format f2fs, with mkfs.f2fs 1.8.0 and 1.4.1 binaries respectively. Change-Id: I4d4bbe8c57544d1c514b7aa37fbf22a0aab14e2c --- updater/install.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/updater/install.cpp b/updater/install.cpp index 888239c83..c5f9a89bb 100644 --- a/updater/install.cpp +++ b/updater/install.cpp @@ -317,9 +317,11 @@ Value* FormatFn(const char* name, State* state, const std::vector(f2fs_argv)); + const char* f2fs_argv[] = { + "mkfs.f2fs", "-t", "-d1", location.c_str(), (size < 512) ? nullptr : num_sectors.c_str(), + nullptr + }; + int status = exec_cmd(f2fs_path, const_cast(f2fs_argv)); if (status != 0) { LOG(ERROR) << name << ": mkfs.f2fs failed (" << status << ") on " << location; return StringValue(""); -- cgit v1.2.3