From 3f638ee8340dec0e95ecdaca7b3f6b3440f85e50 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Thu, 26 Apr 2018 17:23:23 -0700 Subject: [PATCH 1/2] Save the target file when applypatch tests fail Save the target file to tempfile upon unittest failures so that we can try to decompress the deflate chunks in the flaky unittests. And print the zlib version in case that gets changed. Also the SHA1 of the uncompressed data seems correct; so only keep the final SHA1 to double confirm. Bug: 67849209 Test: recovery_component_test Change-Id: Ic6447c2b75c29379d6844cd23a0ff1c4305694a0 --- applypatch/applypatch.cpp | 8 ++++++++ applypatch/imgpatch.cpp | 24 ++++++++++-------------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/applypatch/applypatch.cpp b/applypatch/applypatch.cpp index 39b8030d..14137de9 100644 --- a/applypatch/applypatch.cpp +++ b/applypatch/applypatch.cpp @@ -672,6 +672,14 @@ static int GenerateTarget(const FileContents& source_file, const std::unique_ptr bonus_digest); printf("bonus size %zu sha1 %s\n", bonus_data->data.size(), short_sha1(bonus_digest).c_str()); + // TODO(b/67849209) Remove after debugging the unit test flakiness. + if (android::base::GetMinimumLogSeverity() <= android::base::LogSeverity::DEBUG) { + if (WriteToPartition(reinterpret_cast(memory_sink_str.c_str()), + memory_sink_str.size(), target_filename) != 0) { + LOG(DEBUG) << "Failed to write patched data " << target_filename; + } + } + return 1; } else { printf("now %s\n", short_sha1(target_sha1).c_str()); diff --git a/applypatch/imgpatch.cpp b/applypatch/imgpatch.cpp index b06a64f2..c4c2707f 100644 --- a/applypatch/imgpatch.cpp +++ b/applypatch/imgpatch.cpp @@ -60,6 +60,11 @@ static bool ApplyBSDiffPatchAndStreamOutput(const uint8_t* src_data, size_t src_ int mem_level = Read4(deflate_header + 52); int strategy = Read4(deflate_header + 56); + // TODO(b/67849209) Remove after debugging the unit test flakiness. + if (android::base::GetMinimumLogSeverity() <= android::base::LogSeverity::DEBUG) { + LOG(DEBUG) << "zlib version " << zlibVersion(); + } + z_stream strm; strm.zalloc = Z_NULL; strm.zfree = Z_NULL; @@ -101,26 +106,17 @@ static bool ApplyBSDiffPatchAndStreamOutput(const uint8_t* src_data, size_t src_ size_t have = buffer_size - strm.avail_out; total_written += have; - - // TODO(b/67849209) Remove after debugging the unit test flakiness. - if (android::base::GetMinimumLogSeverity() <= android::base::LogSeverity::DEBUG && - have != 0) { - SHA1_Update(&sha_ctx, data, len - strm.avail_in); - SHA_CTX temp_ctx; - memcpy(&temp_ctx, &sha_ctx, sizeof(SHA_CTX)); - uint8_t digest_so_far[SHA_DIGEST_LENGTH]; - SHA1_Final(digest_so_far, &temp_ctx); - LOG(DEBUG) << "Processed " << actual_target_length + len - strm.avail_in - << " bytes input data in the sink function, sha1 so far: " - << short_sha1(digest_so_far); - } - if (sink(buffer.data(), have) != have) { LOG(ERROR) << "Failed to write " << have << " compressed bytes to output."; return 0; } } while ((strm.avail_in != 0 || strm.avail_out == 0) && ret != Z_STREAM_END); + // TODO(b/67849209) Remove after debugging the unit test flakiness. + if (android::base::GetMinimumLogSeverity() <= android::base::LogSeverity::DEBUG) { + SHA1_Update(&sha_ctx, data, len); + } + actual_target_length += len; return len; }; From f9f17347c8c94c759363c9495fcd5e32a82bf984 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Fri, 27 Apr 2018 10:44:04 -0700 Subject: [PATCH 2/2] recovery: Revoke (most of) short options. `recovery` is not a command line tool, and these short options don't have the common meanings that are used elsewhere. In the platform code, we're not actively using the short forms. - bootable/recovery/uncrypt/uncrypt.cpp - bootable/recovery/updater/install.cpp - frameworks/base/core/java/android/os/RecoverySystem.java - system/core/adb/services.cpp - system/core/init/builtins.cpp - system/update_engine/hardware_android.cc - system/vold/cryptfs.cpp (Callers must have used libbootloader_message.) '--show_text' and '--just_exit' are not that obvious from the initial commit messages. They appear to be used by vendor code (e.g. '--show_text' is optionally used by bootloader, as in the noted bug). So this CL keeps them as is for now. Test: `mmma -j bootable/recovery` Test: Check the code search for possible callers to recovery. Change-Id: I8a87f5fb50131d647dfc8290381ca47a60f543fa --- recovery.cpp | 103 +++++++++++++++++++++++---------------------------- 1 file changed, 47 insertions(+), 56 deletions(-) diff --git a/recovery.cpp b/recovery.cpp index d3af357c..d7bbb13d 100644 --- a/recovery.cpp +++ b/recovery.cpp @@ -75,25 +75,6 @@ #include "stub_ui.h" #include "ui.h" -static const struct option OPTIONS[] = { - { "update_package", required_argument, NULL, 'u' }, - { "retry_count", required_argument, NULL, 'n' }, - { "wipe_data", no_argument, NULL, 'w' }, - { "wipe_cache", no_argument, NULL, 'c' }, - { "show_text", no_argument, NULL, 't' }, - { "sideload", no_argument, NULL, 's' }, - { "sideload_auto_reboot", no_argument, NULL, 'a' }, - { "just_exit", no_argument, NULL, 'x' }, - { "locale", required_argument, NULL, 'l' }, - { "shutdown_after", no_argument, NULL, 'p' }, - { "reason", required_argument, NULL, 'r' }, - { "security", no_argument, NULL, 'e'}, - { "wipe_ab", no_argument, NULL, 0 }, - { "wipe_package_size", required_argument, NULL, 0 }, - { "prompt_and_wipe_data", no_argument, NULL, 0 }, - { NULL, 0, NULL, 0 }, -}; - // More bootreasons can be found in "system/core/bootstat/bootstat.cpp". static const std::vector bootreason_blacklist { "kernel_panic", @@ -140,9 +121,10 @@ struct selabel_handle* sehandle; * The arguments which may be supplied in the recovery.command file: * --update_package=path - verify install an OTA package file * --wipe_data - erase user data (and cache), then reboot - * --prompt_and_wipe_data - prompt the user that data is corrupt, - * with their consent erase user data (and cache), then reboot + * --prompt_and_wipe_data - prompt the user that data is corrupt, with their consent erase user + * data (and cache), then reboot * --wipe_cache - wipe cache (but not user data), then reboot + * --show_text - show the recovery text menu, used by some bootloader (e.g. http://b/36872519). * --set_encrypted_filesystem=on|off - enables / diasables encrypted fs * --just_exit - do nothing; exit and reboot * @@ -1407,6 +1389,25 @@ int main(int argc, char **argv) { std::transform(args.cbegin(), args.cend(), args_to_parse.begin(), [](const std::string& arg) { return const_cast(arg.c_str()); }); + static constexpr struct option OPTIONS[] = { + { "just_exit", no_argument, nullptr, 'x' }, + { "locale", required_argument, nullptr, 0 }, + { "prompt_and_wipe_data", no_argument, nullptr, 0 }, + { "reason", required_argument, nullptr, 0 }, + { "retry_count", required_argument, nullptr, 0 }, + { "security", no_argument, nullptr, 0 }, + { "show_text", no_argument, nullptr, 't' }, + { "shutdown_after", no_argument, nullptr, 0 }, + { "sideload", no_argument, nullptr, 0 }, + { "sideload_auto_reboot", no_argument, nullptr, 0 }, + { "update_package", required_argument, nullptr, 0 }, + { "wipe_ab", no_argument, nullptr, 0 }, + { "wipe_cache", no_argument, nullptr, 0 }, + { "wipe_data", no_argument, nullptr, 0 }, + { "wipe_package_size", required_argument, nullptr, 0 }, + { nullptr, 0, nullptr, 0 }, + }; + const char* update_package = nullptr; bool should_wipe_data = false; bool should_prompt_and_wipe_data = false; @@ -1426,51 +1427,41 @@ int main(int argc, char **argv) { while ((arg = getopt_long(args_to_parse.size(), args_to_parse.data(), "", OPTIONS, &option_index)) != -1) { switch (arg) { - case 'n': - android::base::ParseInt(optarg, &retry_count, 0); - break; - case 'u': - update_package = optarg; - break; - case 'w': - should_wipe_data = true; - break; - case 'c': - should_wipe_cache = true; - break; case 't': show_text = true; break; - case 's': - sideload = true; - break; - case 'a': - sideload = true; - sideload_auto_reboot = true; - break; case 'x': just_exit = true; break; - case 'l': - locale = optarg; - break; - case 'p': - shutdown_after = true; - break; - case 'r': - reason = optarg; - break; - case 'e': - security_update = true; - break; case 0: { std::string option = OPTIONS[option_index].name; - if (option == "wipe_ab") { - should_wipe_ab = true; - } else if (option == "wipe_package_size") { - android::base::ParseUint(optarg, &wipe_package_size); + if (option == "locale") { + locale = optarg; } else if (option == "prompt_and_wipe_data") { should_prompt_and_wipe_data = true; + } else if (option == "reason") { + reason = optarg; + } else if (option == "retry_count") { + android::base::ParseInt(optarg, &retry_count, 0); + } else if (option == "security") { + security_update = true; + } else if (option == "sideload") { + sideload = true; + } else if (option == "sideload_auto_reboot") { + sideload = true; + sideload_auto_reboot = true; + } else if (option == "shutdown_after") { + shutdown_after = true; + } else if (option == "update_package") { + update_package = optarg; + } else if (option == "wipe_ab") { + should_wipe_ab = true; + } else if (option == "wipe_cache") { + should_wipe_cache = true; + } else if (option == "wipe_data") { + should_wipe_data = true; + } else if (option == "wipe_package_size") { + android::base::ParseUint(optarg, &wipe_package_size); } break; }