From 91a649ab62c9cf60ae9d02dd38c23fc23f98a1f3 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 21 May 2018 16:05:56 -0700 Subject: [PATCH 1/2] updater: Add ABORT command. This will be used for testing purpose only, replacing the previously used "fail", to intentionally abort an update. As we're separating the logic between commands parsing and execution, "abort" needs to be considered as a valid command during the parsing. Test: recovery_unit_test and recovery_component_test on marlin. Change-Id: I47c41c423e62c41cc8515fd92f3c5959be08da02 --- tests/component/updater_test.cpp | 13 ++++++++++--- tests/unit/commands_test.cpp | 24 ++++++++++++++++++++++++ updater/blockimg.cpp | 7 +++++++ updater/commands.cpp | 17 ++++++++++++++++- updater/include/private/commands.h | 12 ++++++++++++ 5 files changed, 69 insertions(+), 4 deletions(-) diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp index fe4f45e1..e8abb535 100644 --- a/tests/component/updater_test.cpp +++ b/tests/component/updater_test.cpp @@ -46,6 +46,7 @@ #include "otautil/paths.h" #include "otautil/print_sha1.h" #include "otautil/sysutil.h" +#include "private/commands.h" #include "updater/blockimg.h" #include "updater/install.h" #include "updater/updater.h" @@ -150,6 +151,9 @@ class UpdaterTest : public ::testing::Test { Paths::Get().set_last_command_file(temp_last_command_.path); Paths::Get().set_stash_directory_base(temp_stash_base_.path); + // Enable a special command "abort" to simulate interruption. + Command::abort_allowed_ = true; + last_command_file_ = temp_last_command_.path; image_file_ = image_temp_file_.path; } @@ -580,7 +584,7 @@ TEST_F(UpdaterTest, block_image_update_fail) { "2", "stash " + src_hash + " 2,0,2", "free " + src_hash, - "fail", + "abort", // clang-format on }; @@ -714,7 +718,7 @@ TEST_F(UpdaterTest, last_command_update) { "stash " + block1_hash + " 2,0,1", "move " + block1_hash + " 2,1,2 1 2,0,1", "stash " + block3_hash + " 2,2,3", - "fail", + "abort", // clang-format on }; @@ -859,6 +863,9 @@ class ResumableUpdaterTest : public testing::TestWithParam { Paths::Get().set_last_command_file(temp_last_command_.path); Paths::Get().set_stash_directory_base(temp_stash_base_.path); + // Enable a special command "abort" to simulate interruption. + Command::abort_allowed_ = true; + index_ = GetParam(); image_file_ = image_temp_file_.path; last_command_file_ = temp_last_command_.path; @@ -1030,7 +1037,7 @@ TEST_P(ResumableUpdaterTest, InterruptVerifyResume) { << g_transfer_list[kTransferListHeaderLines + index_] << ")"; std::vector transfer_list_copy{ g_transfer_list }; - transfer_list_copy[kTransferListHeaderLines + index_] = "fail"; + transfer_list_copy[kTransferListHeaderLines + index_] = "abort"; g_entries["transfer_list"] = android::base::Join(transfer_list_copy, '\n'); diff --git a/tests/unit/commands_test.cpp b/tests/unit/commands_test.cpp index cb2be917..3daa58f3 100644 --- a/tests/unit/commands_test.cpp +++ b/tests/unit/commands_test.cpp @@ -159,6 +159,27 @@ TEST(CommandsTest, Parse_EmptyInput) { ASSERT_EQ("invalid type", err); } +TEST(CommandsTest, Parse_ABORT_Allowed) { + Command::abort_allowed_ = true; + + const std::string input{ "abort" }; + std::string err; + Command command = Command::Parse(input, 0, &err); + ASSERT_TRUE(command); + + ASSERT_EQ(TargetInfo(), command.target()); + ASSERT_EQ(SourceInfo(), command.source()); + ASSERT_EQ(StashInfo(), command.stash()); + ASSERT_EQ(PatchInfo(), command.patch()); +} + +TEST(CommandsTest, Parse_ABORT_NotAllowed) { + const std::string input{ "abort" }; + std::string err; + Command command = Command::Parse(input, 0, &err); + ASSERT_FALSE(command); +} + TEST(CommandsTest, Parse_BSDIFF) { const std::string input{ "bsdiff 0 148 " @@ -312,9 +333,12 @@ TEST(CommandsTest, Parse_ZERO) { } TEST(CommandsTest, Parse_InvalidNumberOfArgs) { + Command::abort_allowed_ = true; + // Note that the case of having excess args in BSDIFF, IMGDIFF and MOVE is covered by // ParseTargetInfoAndSourceInfo_InvalidInput. std::vector inputs{ + "abort foo", "bsdiff", "erase", "erase 4,3,5,10,12 hash1", diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp index 937c5e14..fc713859 100644 --- a/updater/blockimg.cpp +++ b/updater/blockimg.cpp @@ -1489,6 +1489,11 @@ static int PerformCommandErase(CommandParameters& params) { return 0; } +static int PerformCommandAbort(CommandParameters&) { + LOG(INFO) << "Aborting as instructed"; + return -1; +} + using CommandFunction = std::function; using CommandMap = std::unordered_map; @@ -1888,6 +1893,7 @@ Value* BlockImageVerifyFn(const char* name, State* state, // Commands which are not allowed are set to nullptr to skip them completely. const CommandMap command_map{ // clang-format off + { Command::Type::ABORT, PerformCommandAbort }, { Command::Type::BSDIFF, PerformCommandDiff }, { Command::Type::ERASE, nullptr }, { Command::Type::FREE, PerformCommandFree }, @@ -1908,6 +1914,7 @@ Value* BlockImageUpdateFn(const char* name, State* state, const std::vector>& argv) { const CommandMap command_map{ // clang-format off + { Command::Type::ABORT, PerformCommandAbort }, { Command::Type::BSDIFF, PerformCommandDiff }, { Command::Type::ERASE, PerformCommandErase }, { Command::Type::FREE, PerformCommandFree }, diff --git a/updater/commands.cpp b/updater/commands.cpp index fb19ebc9..e8814967 100644 --- a/updater/commands.cpp +++ b/updater/commands.cpp @@ -29,8 +29,16 @@ using namespace std::string_literals; +bool Command::abort_allowed_ = false; + Command::Type Command::ParseType(const std::string& type_str) { - if (type_str == "bsdiff") { + if (type_str == "abort") { + if (!abort_allowed_) { + LOG(ERROR) << "ABORT disallowed"; + return Type::LAST; + } + return Type::ABORT; + } else if (type_str == "bsdiff") { return Type::BSDIFF; } else if (type_str == "erase") { return Type::ERASE; @@ -237,6 +245,13 @@ Command Command::Parse(const std::string& line, size_t index, std::string* err) src_hash, &source_info, err)) { return {}; } + } else if (op == Type::ABORT) { + // No-op, other than sanity checking the input args. + if (pos != tokens.size()) { + *err = android::base::StringPrintf("invalid number of args: %zu (expected 0)", + tokens.size() - pos); + return {}; + } } else { *err = "invalid op"; return {}; diff --git a/updater/include/private/commands.h b/updater/include/private/commands.h index 784892fb..087d7cfb 100644 --- a/updater/include/private/commands.h +++ b/updater/include/private/commands.h @@ -213,9 +213,13 @@ class PatchInfo { // - Free the given stash data. // - Meaningful args: StashInfo // +// abort +// - Abort the current update. Allowed for testing code only. +// class Command { public: enum class Type { + ABORT, BSDIFF, ERASE, FREE, @@ -280,6 +284,11 @@ class Command { } private: + friend class ResumableUpdaterTest; + friend class UpdaterTest; + + FRIEND_TEST(CommandsTest, Parse_ABORT_Allowed); + FRIEND_TEST(CommandsTest, Parse_InvalidNumberOfArgs); FRIEND_TEST(CommandsTest, ParseTargetInfoAndSourceInfo_InvalidInput); FRIEND_TEST(CommandsTest, ParseTargetInfoAndSourceInfo_StashesOnly); FRIEND_TEST(CommandsTest, ParseTargetInfoAndSourceInfo_SourceBlocksAndStashes); @@ -293,6 +302,9 @@ class Command { const std::string& src_hash, SourceInfo* source, std::string* err); + // Allows parsing ABORT command, which should be used for testing purpose only. + static bool abort_allowed_; + // The type of the command. Type type_{ Type::LAST }; // The index of the Command object, which is specified by the caller. From 0b58e9a011cac548f7e5d6477bbe99f2bbe72f81 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Fri, 6 Jul 2018 15:01:05 -0700 Subject: [PATCH 2/2] updater: Remove the support for sha1_check(). The matching edify function has been removed from EdifyGenerator [1]. In theory device-specific releasetools script may still use this function, but it no longer looks useful. Because a) we should use range_sha1() when asserting the SHA-1 hash of a block device; b) we should look into the contents when asserting a text file. [1] https://android-review.googlesource.com/c/platform/build/+/714104 Test: Run recovery_component_test on marlin. Test: Code search shows no active user. Change-Id: Id39439101534fb89cf8c5cea80a4b758c8a1a60d --- tests/component/updater_test.cpp | 61 +++++++++++++++----------------- updater/install.cpp | 50 +------------------------- 2 files changed, 29 insertions(+), 82 deletions(-) diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp index e8abb535..0b6b96f7 100644 --- a/tests/component/updater_test.cpp +++ b/tests/component/updater_test.cpp @@ -72,7 +72,7 @@ static void expect(const char* expected, const char* expr_str, CauseCode cause_c if (expected == nullptr) { ASSERT_FALSE(status); } else { - ASSERT_TRUE(status); + ASSERT_TRUE(status) << "Evaluate() finished with error message: " << state.errmsg; ASSERT_STREQ(expected, result.c_str()); } @@ -138,6 +138,25 @@ static std::string get_sha1(const std::string& content) { return print_sha1(digest); } +static Value* BlobToString(const char* name, State* state, + const std::vector>& argv) { + if (argv.size() != 1) { + return ErrorAbort(state, kArgsParsingFailure, "%s() expects 1 arg, got %zu", name, argv.size()); + } + + std::vector> args; + if (!ReadValueArgs(state, argv, &args)) { + return nullptr; + } + + if (args[0]->type != VAL_BLOB) { + return ErrorAbort(state, kArgsParsingFailure, "%s() expects a BLOB argument", name); + } + + args[0]->type = VAL_STRING; + return args[0].release(); +} + class UpdaterTest : public ::testing::Test { protected: void SetUp() override { @@ -145,6 +164,8 @@ class UpdaterTest : public ::testing::Test { RegisterInstallFunctions(); RegisterBlockImageFunctions(); + RegisterFunction("blob_to_string", BlobToString); + // Each test is run in a separate process (isolated mode). Shared temporary files won't cause // conflicts. Paths::Get().set_cache_temp_source(temp_saved_source_.path); @@ -192,33 +213,6 @@ TEST_F(UpdaterTest, getprop) { expect(nullptr, "getprop(\"arg1\", \"arg2\")", kArgsParsingFailure); } -TEST_F(UpdaterTest, sha1_check) { - // sha1_check(data) returns the SHA-1 of the data. - expect("81fe8bfe87576c3ecb22426f8e57847382917acf", "sha1_check(\"abcd\")", kNoCause); - expect("da39a3ee5e6b4b0d3255bfef95601890afd80709", "sha1_check(\"\")", kNoCause); - - // sha1_check(data, sha1_hex, [sha1_hex, ...]) returns the matched SHA-1. - expect("81fe8bfe87576c3ecb22426f8e57847382917acf", - "sha1_check(\"abcd\", \"81fe8bfe87576c3ecb22426f8e57847382917acf\")", - kNoCause); - - expect("81fe8bfe87576c3ecb22426f8e57847382917acf", - "sha1_check(\"abcd\", \"wrong_sha1\", \"81fe8bfe87576c3ecb22426f8e57847382917acf\")", - kNoCause); - - // Or "" if there's no match. - expect("", - "sha1_check(\"abcd\", \"wrong_sha1\")", - kNoCause); - - expect("", - "sha1_check(\"abcd\", \"wrong_sha1\", \"wrong_sha2\")", - kNoCause); - - // sha1_check() expects at least one argument. - expect(nullptr, "sha1_check()", kArgsParsingFailure); -} - TEST_F(UpdaterTest, apply_patch_check) { // Zero-argument is not valid. expect(nullptr, "apply_patch_check()", kArgsParsingFailure); @@ -355,12 +349,13 @@ TEST_F(UpdaterTest, package_extract_file) { script = "package_extract_file(\"a.txt\", \"/dev/full\")"; expect("", script.c_str(), kNoCause, &updater_info); - // One-argument version. - script = "sha1_check(package_extract_file(\"a.txt\"))"; - expect(kATxtSha1Sum.c_str(), script.c_str(), kNoCause, &updater_info); + // One-argument version. package_extract_file() gives a VAL_BLOB, which needs to be converted to + // VAL_STRING for equality test. + script = "blob_to_string(package_extract_file(\"a.txt\")) == \"" + kATxtContents + "\""; + expect("t", script.c_str(), kNoCause, &updater_info); - script = "sha1_check(package_extract_file(\"b.txt\"))"; - expect(kBTxtSha1Sum.c_str(), script.c_str(), kNoCause, &updater_info); + script = "blob_to_string(package_extract_file(\"b.txt\")) == \"" + kBTxtContents + "\""; + expect("t", script.c_str(), kNoCause, &updater_info); // Missing entry. The one-argument version aborts the evaluation. script = "package_extract_file(\"doesntexist\")"; diff --git a/updater/install.cpp b/updater/install.cpp index 1ac19ce0..9d108e0e 100644 --- a/updater/install.cpp +++ b/updater/install.cpp @@ -262,9 +262,7 @@ Value* ApplyPatchFn(const char* name, State* state, // apply_patch_check(filename, [sha1, ...]) // Returns true if the contents of filename or the temporary copy in the cache partition (if // present) have a SHA-1 checksum equal to one of the given sha1 values. sha1 values are -// specified as 40 hex digits. This function differs from sha1_check(read_file(filename), -// sha1 [, ...]) in that it knows to check the cache partition copy, so apply_patch_check() will -// succeed even if the file was corrupted by an interrupted apply_patch() update. +// specified as 40 hex digits. Value* ApplyPatchCheckFn(const char* name, State* state, const std::vector>& argv) { if (argv.size() < 1) { @@ -287,51 +285,6 @@ Value* ApplyPatchCheckFn(const char* name, State* state, return StringValue(result == 0 ? "t" : ""); } -// sha1_check(data) -// to return the sha1 of the data (given in the format returned by -// read_file). -// -// sha1_check(data, sha1_hex, [sha1_hex, ...]) -// returns the sha1 of the file if it matches any of the hex -// strings passed, or "" if it does not equal any of them. -// -Value* Sha1CheckFn(const char* name, State* state, const std::vector>& argv) { - if (argv.size() < 1) { - return ErrorAbort(state, kArgsParsingFailure, "%s() expects at least 1 arg", name); - } - - std::vector> args; - if (!ReadValueArgs(state, argv, &args)) { - return nullptr; - } - - if (args[0]->type == VAL_INVALID) { - return StringValue(""); - } - uint8_t digest[SHA_DIGEST_LENGTH]; - SHA1(reinterpret_cast(args[0]->data.c_str()), args[0]->data.size(), digest); - - if (argv.size() == 1) { - return StringValue(print_sha1(digest)); - } - - for (size_t i = 1; i < argv.size(); ++i) { - uint8_t arg_digest[SHA_DIGEST_LENGTH]; - if (args[i]->type != VAL_STRING) { - LOG(ERROR) << name << "(): arg " << i << " is not a string; skipping"; - } else if (ParseSha1(args[i]->data, arg_digest) != 0) { - // Warn about bad args and skip them. - LOG(ERROR) << name << "(): error parsing \"" << args[i]->data << "\" as sha-1; skipping"; - } else if (memcmp(digest, arg_digest, SHA_DIGEST_LENGTH) == 0) { - // Found a match. - return args[i].release(); - } - } - - // Didn't match any of the hex strings; return false. - return StringValue(""); -} - // mount(fs_type, partition_type, location, mount_point) // mount(fs_type, partition_type, location, mount_point, mount_options) @@ -1027,7 +980,6 @@ void RegisterInstallFunctions() { RegisterFunction("wipe_block_device", WipeBlockDeviceFn); RegisterFunction("read_file", ReadFileFn); - RegisterFunction("sha1_check", Sha1CheckFn); RegisterFunction("write_value", WriteValueFn); RegisterFunction("wipe_cache", WipeCacheFn);