From 5450c84ba4cc62b887e69ec11a922194b36756a9 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Wed, 18 Oct 2017 13:15:21 -0700 Subject: Finish the new data receiver when update fails The thread to receive new data may still be alive after we exit PerformBlockImageUpdate() upon failures. This caused memory corruption when we run the unittest repeatedly. Set the receiver_available flag to false and make sure the receiver exits when the update fails. Bug: 65430057 Test: unittests passed with tsan Change-Id: Icb232d13fb96c78262249ffbd29cdbe5b77f1fce --- tests/component/updater_test.cpp | 220 +++++++++++++++++++++++---------------- updater/blockimg.cpp | 57 +++++++--- 2 files changed, 174 insertions(+), 103 deletions(-) diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp index e6aec4ad6..266657d31 100644 --- a/tests/component/updater_test.cpp +++ b/tests/component/updater_test.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -74,6 +75,23 @@ static void expect(const char* expected, const char* expr_str, CauseCode cause_c ASSERT_EQ(cause_code, state.cause_code); } +static void BuildUpdatePackage(const std::unordered_map& entries, + int fd) { + FILE* zip_file_ptr = fdopen(fd, "wb"); + ZipWriter zip_writer(zip_file_ptr); + + for (const auto& entry : entries) { + ASSERT_EQ(0, zip_writer.StartEntry(entry.first.c_str(), 0)); + if (!entry.second.empty()) { + ASSERT_EQ(0, zip_writer.WriteBytes(entry.second.data(), entry.second.size())); + } + ASSERT_EQ(0, zip_writer.FinishEntry()); + } + + ASSERT_EQ(0, zip_writer.Finish()); + ASSERT_EQ(0, fclose(zip_file_ptr)); +} + static std::string get_sha1(const std::string& content) { uint8_t digest[SHA_DIGEST_LENGTH]; SHA1(reinterpret_cast(content.c_str()), content.size(), digest); @@ -420,30 +438,19 @@ TEST_F(UpdaterTest, show_progress) { ASSERT_EQ(0, fclose(updater_info.cmd_pipe)); } -TEST_F(UpdaterTest, block_image_update) { - // Create a zip file with new_data and patch_data. - TemporaryFile zip_file; - FILE* zip_file_ptr = fdopen(zip_file.release(), "wb"); - ZipWriter zip_writer(zip_file_ptr); - - // Add a dummy new data. - ASSERT_EQ(0, zip_writer.StartEntry("new_data", 0)); - ASSERT_EQ(0, zip_writer.FinishEntry()); - - // Generate and add the patch data. +TEST_F(UpdaterTest, block_image_update_patch_data) { std::string src_content = std::string(4096, 'a') + std::string(4096, 'c'); std::string tgt_content = std::string(4096, 'b') + std::string(4096, 'd'); + + // Generate the patch data. TemporaryFile patch_file; ASSERT_EQ(0, bsdiff::bsdiff(reinterpret_cast(src_content.data()), src_content.size(), reinterpret_cast(tgt_content.data()), tgt_content.size(), patch_file.path, nullptr)); std::string patch_content; ASSERT_TRUE(android::base::ReadFileToString(patch_file.path, &patch_content)); - ASSERT_EQ(0, zip_writer.StartEntry("patch_data", 0)); - ASSERT_EQ(0, zip_writer.WriteBytes(patch_content.data(), patch_content.size())); - ASSERT_EQ(0, zip_writer.FinishEntry()); - // Add two transfer lists. The first one contains a bsdiff; and we expect the update to succeed. + // Create the transfer list that contains a bsdiff. std::string src_hash = get_sha1(src_content); std::string tgt_hash = get_sha1(tgt_content); std::vector transfer_list = { @@ -456,27 +463,16 @@ TEST_F(UpdaterTest, block_image_update) { src_hash.c_str(), tgt_hash.c_str(), src_hash.c_str()), "free " + src_hash, }; - ASSERT_EQ(0, zip_writer.StartEntry("transfer_list", 0)); - std::string commands = android::base::Join(transfer_list, '\n'); - ASSERT_EQ(0, zip_writer.WriteBytes(commands.data(), commands.size())); - ASSERT_EQ(0, zip_writer.FinishEntry()); - // Stash and free some blocks, then fail the 2nd update intentionally. - std::vector fail_transfer_list = { - "4", - "2", - "0", - "2", - "stash " + tgt_hash + " 2,0,2", - "free " + tgt_hash, - "fail", + std::unordered_map entries = { + { "new_data", "" }, + { "patch_data", patch_content }, + { "transfer_list", android::base::Join(transfer_list, '\n') }, }; - ASSERT_EQ(0, zip_writer.StartEntry("fail_transfer_list", 0)); - std::string fail_commands = android::base::Join(fail_transfer_list, '\n'); - ASSERT_EQ(0, zip_writer.WriteBytes(fail_commands.data(), fail_commands.size())); - ASSERT_EQ(0, zip_writer.FinishEntry()); - ASSERT_EQ(0, zip_writer.Finish()); - ASSERT_EQ(0, fclose(zip_file_ptr)); + + // Build the update package. + TemporaryFile zip_file; + BuildUpdatePackage(entries, zip_file.release()); MemMapping map; ASSERT_TRUE(map.MapFile(zip_file.path)); @@ -491,7 +487,7 @@ TEST_F(UpdaterTest, block_image_update) { updater_info.package_zip_addr = map.addr; updater_info.package_zip_len = map.length; - // Execute the commands in the 1st transfer list. + // Execute the commands in the transfer list. TemporaryFile update_file; ASSERT_TRUE(android::base::WriteStringToFile(src_content, update_file.path)); std::string script = "block_image_update(\"" + std::string(update_file.path) + @@ -502,44 +498,98 @@ TEST_F(UpdaterTest, block_image_update) { ASSERT_TRUE(android::base::ReadFileToString(update_file.path, &updated_content)); ASSERT_EQ(tgt_hash, get_sha1(updated_content)); - // Expect the 2nd update to fail, but expect the stashed blocks to be freed. - script = "block_image_update(\"" + std::string(update_file.path) + - R"(", package_extract_file("fail_transfer_list"), "new_data", "patch_data"))"; + ASSERT_EQ(0, fclose(updater_info.cmd_pipe)); + CloseArchive(handle); +} + +TEST_F(UpdaterTest, block_image_update_fail) { + std::string src_content(4096 * 2, 'e'); + std::string src_hash = get_sha1(src_content); + // Stash and free some blocks, then fail the update intentionally. + std::vector transfer_list = { + "4", "2", "0", "2", "stash " + src_hash + " 2,0,2", "free " + src_hash, "fail", + }; + + // Add a new data of 10 bytes to test the deadlock. + std::unordered_map entries = { + { "new_data", std::string(10, 0) }, + { "patch_data", "" }, + { "transfer_list", android::base::Join(transfer_list, '\n') }, + }; + + // Build the update package. + TemporaryFile zip_file; + BuildUpdatePackage(entries, zip_file.release()); + + MemMapping map; + ASSERT_TRUE(map.MapFile(zip_file.path)); + ZipArchiveHandle handle; + ASSERT_EQ(0, OpenArchiveFromMemory(map.addr, map.length, zip_file.path, &handle)); + + // Set up the handler, command_pipe, patch offset & length. + UpdaterInfo updater_info; + updater_info.package_zip = handle; + TemporaryFile temp_pipe; + updater_info.cmd_pipe = fdopen(temp_pipe.release(), "wbe"); + updater_info.package_zip_addr = map.addr; + updater_info.package_zip_len = map.length; + + TemporaryFile update_file; + ASSERT_TRUE(android::base::WriteStringToFile(src_content, update_file.path)); + // Expect the stashed blocks to be freed. + std::string script = "block_image_update(\"" + std::string(update_file.path) + + R"(", package_extract_file("transfer_list"), "new_data", "patch_data"))"; expect("", script.c_str(), kNoCause, &updater_info); // Updater generates the stash name based on the input file name. std::string name_digest = get_sha1(update_file.path); std::string stash_base = "/cache/recovery/" + name_digest; ASSERT_EQ(0, access(stash_base.c_str(), F_OK)); - ASSERT_EQ(-1, access((stash_base + tgt_hash).c_str(), F_OK)); + ASSERT_EQ(-1, access((stash_base + src_hash).c_str(), F_OK)); ASSERT_EQ(0, rmdir(stash_base.c_str())); ASSERT_EQ(0, fclose(updater_info.cmd_pipe)); CloseArchive(handle); } -TEST_F(UpdaterTest, new_data_short_write) { - // Create a zip file with new_data. +TEST_F(UpdaterTest, new_data_over_write) { + std::vector transfer_list = { + "4", "1", "0", "0", "new 2,0,1", + }; + + // Write 4096 + 100 bytes of new data. + std::unordered_map entries = { + { "new_data", std::string(4196, 0) }, + { "patch_data", "" }, + { "transfer_list", android::base::Join(transfer_list, '\n') }, + }; + + // Build the update package. TemporaryFile zip_file; - FILE* zip_file_ptr = fdopen(zip_file.release(), "wb"); - ZipWriter zip_writer(zip_file_ptr); + BuildUpdatePackage(entries, zip_file.release()); + + MemMapping map; + ASSERT_TRUE(map.MapFile(zip_file.path)); + ZipArchiveHandle handle; + ASSERT_EQ(0, OpenArchiveFromMemory(map.addr, map.length, zip_file.path, &handle)); - // Add the empty new data. - ASSERT_EQ(0, zip_writer.StartEntry("empty_new_data", 0)); - ASSERT_EQ(0, zip_writer.FinishEntry()); - // Add the short written new data. - ASSERT_EQ(0, zip_writer.StartEntry("short_new_data", 0)); - std::string new_data_short = std::string(10, 'a'); - ASSERT_EQ(0, zip_writer.WriteBytes(new_data_short.data(), new_data_short.size())); - ASSERT_EQ(0, zip_writer.FinishEntry()); - // Add the data of exactly one block. - ASSERT_EQ(0, zip_writer.StartEntry("exact_new_data", 0)); - std::string new_data_exact = std::string(4096, 'a'); - ASSERT_EQ(0, zip_writer.WriteBytes(new_data_exact.data(), new_data_exact.size())); - ASSERT_EQ(0, zip_writer.FinishEntry()); - // Add a dummy patch data. - ASSERT_EQ(0, zip_writer.StartEntry("patch_data", 0)); - ASSERT_EQ(0, zip_writer.FinishEntry()); + // Set up the handler, command_pipe, patch offset & length. + UpdaterInfo updater_info; + updater_info.package_zip = handle; + TemporaryFile temp_pipe; + updater_info.cmd_pipe = fdopen(temp_pipe.release(), "wbe"); + updater_info.package_zip_addr = map.addr; + updater_info.package_zip_len = map.length; + TemporaryFile update_file; + std::string script = "block_image_update(\"" + std::string(update_file.path) + + R"(", package_extract_file("transfer_list"), "new_data", "patch_data"))"; + expect("t", script.c_str(), kNoCause, &updater_info); + + ASSERT_EQ(0, fclose(updater_info.cmd_pipe)); + CloseArchive(handle); +} + +TEST_F(UpdaterTest, new_data_short_write) { std::vector transfer_list = { "4", "1", @@ -547,12 +597,17 @@ TEST_F(UpdaterTest, new_data_short_write) { "0", "new 2,0,1", }; - ASSERT_EQ(0, zip_writer.StartEntry("transfer_list", 0)); - std::string commands = android::base::Join(transfer_list, '\n'); - ASSERT_EQ(0, zip_writer.WriteBytes(commands.data(), commands.size())); - ASSERT_EQ(0, zip_writer.FinishEntry()); - ASSERT_EQ(0, zip_writer.Finish()); - ASSERT_EQ(0, fclose(zip_file_ptr)); + + std::unordered_map entries = { + { "empty_new_data", "" }, + { "short_new_data", std::string(10, 'a') }, + { "exact_new_data", std::string(4096, 'a') }, + { "patch_data", "" }, + { "transfer_list", android::base::Join(transfer_list, '\n') }, + }; + + TemporaryFile zip_file; + BuildUpdatePackage(entries, zip_file.release()); MemMapping map; ASSERT_TRUE(map.MapFile(zip_file.path)); @@ -587,14 +642,6 @@ TEST_F(UpdaterTest, new_data_short_write) { } TEST_F(UpdaterTest, brotli_new_data) { - // Create a zip file with new_data. - TemporaryFile zip_file; - FILE* zip_file_ptr = fdopen(zip_file.release(), "wb"); - ZipWriter zip_writer(zip_file_ptr); - - // Add a brotli compressed new data entry. - ASSERT_EQ(0, zip_writer.StartEntry("new.dat.br", 0)); - auto generator = []() { return rand() % 128; }; // Generate 100 blocks of random data. std::string brotli_new_data; @@ -602,16 +649,12 @@ TEST_F(UpdaterTest, brotli_new_data) { generate_n(back_inserter(brotli_new_data), 4096 * 100, generator); size_t encoded_size = BrotliEncoderMaxCompressedSize(brotli_new_data.size()); - std::vector encoded_data(encoded_size); + std::string encoded_data(encoded_size, 0); ASSERT_TRUE(BrotliEncoderCompress( BROTLI_DEFAULT_QUALITY, BROTLI_DEFAULT_WINDOW, BROTLI_DEFAULT_MODE, brotli_new_data.size(), - reinterpret_cast(brotli_new_data.data()), &encoded_size, encoded_data.data())); - - ASSERT_EQ(0, zip_writer.WriteBytes(encoded_data.data(), encoded_size)); - ASSERT_EQ(0, zip_writer.FinishEntry()); - // Add a dummy patch data. - ASSERT_EQ(0, zip_writer.StartEntry("patch_data", 0)); - ASSERT_EQ(0, zip_writer.FinishEntry()); + reinterpret_cast(brotli_new_data.data()), &encoded_size, + reinterpret_cast(const_cast(encoded_data.data())))); + encoded_data.resize(encoded_size); // Write a few small chunks of new data, then a large chunk, and finally a few small chunks. // This helps us to catch potential short writes. @@ -627,12 +670,15 @@ TEST_F(UpdaterTest, brotli_new_data) { "new 2,98,99", "new 2,99,100", }; - ASSERT_EQ(0, zip_writer.StartEntry("transfer_list", 0)); - std::string commands = android::base::Join(transfer_list, '\n'); - ASSERT_EQ(0, zip_writer.WriteBytes(commands.data(), commands.size())); - ASSERT_EQ(0, zip_writer.FinishEntry()); - ASSERT_EQ(0, zip_writer.Finish()); - ASSERT_EQ(0, fclose(zip_file_ptr)); + + std::unordered_map entries = { + { "new.dat.br", std::move(encoded_data) }, + { "patch_data", "" }, + { "transfer_list", android::base::Join(transfer_list, '\n') }, + }; + + TemporaryFile zip_file; + BuildUpdatePackage(entries, zip_file.release()); MemMapping map; ASSERT_TRUE(map.MapFile(zip_file.path)); diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp index ce3cea4f3..6c7b3efcf 100644 --- a/updater/blockimg.cpp +++ b/updater/blockimg.cpp @@ -281,6 +281,11 @@ static bool receive_new_data(const uint8_t* data, size_t size, void* cookie) { // Wait for nti->writer to be non-null, indicating some of this data is wanted. pthread_mutex_lock(&nti->mu); while (nti->writer == nullptr) { + // End the new data receiver if we encounter an error when performing block image update. + if (!nti->receiver_available) { + pthread_mutex_unlock(&nti->mu); + return false; + } pthread_cond_wait(&nti->cv, &nti->mu); } pthread_mutex_unlock(&nti->mu); @@ -316,6 +321,11 @@ static bool receive_brotli_new_data(const uint8_t* data, size_t size, void* cook // Wait for nti->writer to be non-null, indicating some of this data is wanted. pthread_mutex_lock(&nti->mu); while (nti->writer == nullptr) { + // End the receiver if we encounter an error when performing block image update. + if (!nti->receiver_available) { + pthread_mutex_unlock(&nti->mu); + return false; + } pthread_cond_wait(&nti->cv, &nti->mu); } pthread_mutex_unlock(&nti->mu); @@ -1591,29 +1601,44 @@ static Value* PerformBlockImageUpdate(const char* name, State* state, } } + rc = 0; + +pbiudone: if (params.canwrite) { - pthread_join(params.thread, nullptr); + pthread_mutex_lock(¶ms.nti.mu); + if (params.nti.receiver_available) { + LOG(WARNING) << "new data receiver is still available after executing all commands."; + } + params.nti.receiver_available = false; + pthread_cond_broadcast(¶ms.nti.cv); + pthread_mutex_unlock(¶ms.nti.mu); + int ret = pthread_join(params.thread, nullptr); + if (ret != 0) { + LOG(WARNING) << "pthread join returned with " << strerror(ret); + } - LOG(INFO) << "wrote " << params.written << " blocks; expected " << total_blocks; - LOG(INFO) << "stashed " << params.stashed << " blocks"; - LOG(INFO) << "max alloc needed was " << params.buffer.size(); + if (rc == 0) { + LOG(INFO) << "wrote " << params.written << " blocks; expected " << total_blocks; + LOG(INFO) << "stashed " << params.stashed << " blocks"; + LOG(INFO) << "max alloc needed was " << params.buffer.size(); - const char* partition = strrchr(blockdev_filename->data.c_str(), '/'); - if (partition != nullptr && *(partition + 1) != 0) { - fprintf(cmd_pipe, "log bytes_written_%s: %zu\n", partition + 1, params.written * BLOCKSIZE); - fprintf(cmd_pipe, "log bytes_stashed_%s: %zu\n", partition + 1, params.stashed * BLOCKSIZE); - fflush(cmd_pipe); + const char* partition = strrchr(blockdev_filename->data.c_str(), '/'); + if (partition != nullptr && *(partition + 1) != 0) { + fprintf(cmd_pipe, "log bytes_written_%s: %zu\n", partition + 1, params.written * BLOCKSIZE); + fprintf(cmd_pipe, "log bytes_stashed_%s: %zu\n", partition + 1, params.stashed * BLOCKSIZE); + fflush(cmd_pipe); + } + // Delete stash only after successfully completing the update, as it may contain blocks needed + // to complete the update later. + DeleteStash(params.stashbase); } - // Delete stash only after successfully completing the update, as it may contain blocks needed - // to complete the update later. - DeleteStash(params.stashbase); - } else { + + pthread_mutex_destroy(¶ms.nti.mu); + pthread_cond_destroy(¶ms.nti.cv); + } else if (rc == 0) { LOG(INFO) << "verified partition contents; update may be resumed"; } - rc = 0; - -pbiudone: if (ota_fsync(params.fd) == -1) { failure_type = kFsyncFailure; PLOG(ERROR) << "fsync failed"; -- cgit v1.2.3