diff options
| -rw-r--r-- | applypatch/imgdiff.cpp | 45 | ||||
| -rw-r--r-- | applypatch/include/applypatch/imgdiff_image.h | 5 | ||||
| -rw-r--r-- | roots.cpp | 2 | ||||
| -rw-r--r-- | tests/component/imgdiff_test.cpp | 101 |
4 files changed, 132 insertions, 21 deletions
diff --git a/applypatch/imgdiff.cpp b/applypatch/imgdiff.cpp index 3dae63d4b..674cc2b16 100644 --- a/applypatch/imgdiff.cpp +++ b/applypatch/imgdiff.cpp @@ -955,14 +955,17 @@ bool ZipModeImage::SplitZipModeImageWithLimit(const ZipModeImage& tgt_image, tgt->GetRawDataLength()); } } else { - ZipModeImage::AddSplitImageFromChunkList(tgt_image, src_image, src_ranges, split_tgt_chunks, - split_src_chunks, split_tgt_images, - split_src_images); + bool added_image = ZipModeImage::AddSplitImageFromChunkList( + tgt_image, src_image, src_ranges, split_tgt_chunks, split_src_chunks, split_tgt_images, + split_src_images); split_tgt_chunks.clear(); split_src_chunks.clear(); - used_src_ranges.Insert(src_ranges); - split_src_ranges->push_back(std::move(src_ranges)); + // No need to update the split_src_ranges if we don't update the split source images. + if (added_image) { + used_src_ranges.Insert(src_ranges); + split_src_ranges->push_back(std::move(src_ranges)); + } src_ranges.Clear(); // We don't have enough space for the current chunk; start a new split image and handle @@ -973,9 +976,12 @@ bool ZipModeImage::SplitZipModeImageWithLimit(const ZipModeImage& tgt_image, // TODO Trim it in case the CD exceeds limit too much. src_ranges.Insert(central_directory->GetStartOffset(), central_directory->DataLengthForPatch()); - ZipModeImage::AddSplitImageFromChunkList(tgt_image, src_image, src_ranges, split_tgt_chunks, - split_src_chunks, split_tgt_images, split_src_images); - split_src_ranges->push_back(std::move(src_ranges)); + bool added_image = ZipModeImage::AddSplitImageFromChunkList(tgt_image, src_image, src_ranges, + split_tgt_chunks, split_src_chunks, + split_tgt_images, split_src_images); + if (added_image) { + split_src_ranges->push_back(std::move(src_ranges)); + } ValidateSplitImages(*split_tgt_images, *split_src_images, *split_src_ranges, tgt_image.file_content_.size()); @@ -983,7 +989,7 @@ bool ZipModeImage::SplitZipModeImageWithLimit(const ZipModeImage& tgt_image, return true; } -void ZipModeImage::AddSplitImageFromChunkList(const ZipModeImage& tgt_image, +bool ZipModeImage::AddSplitImageFromChunkList(const ZipModeImage& tgt_image, const ZipModeImage& src_image, const SortedRangeSet& split_src_ranges, const std::vector<ImageChunk>& split_tgt_chunks, @@ -991,12 +997,6 @@ void ZipModeImage::AddSplitImageFromChunkList(const ZipModeImage& tgt_image, std::vector<ZipModeImage>* split_tgt_images, std::vector<ZipModeImage>* split_src_images) { CHECK(!split_tgt_chunks.empty()); - // Target chunks should occupy at least one block. - // TODO put a warning and change the type to raw if it happens in extremely rare cases. - size_t tgt_size = split_tgt_chunks.back().GetStartOffset() + - split_tgt_chunks.back().DataLengthForPatch() - - split_tgt_chunks.front().GetStartOffset(); - CHECK_GE(tgt_size, BLOCK_SIZE); std::vector<ImageChunk> aligned_tgt_chunks; @@ -1015,7 +1015,12 @@ void ZipModeImage::AddSplitImageFromChunkList(const ZipModeImage& tgt_image, i++; } - CHECK_LT(i, split_tgt_chunks.size()); + + // Nothing left after alignment in the current split tgt chunks; skip adding the split_tgt_image. + if (i == split_tgt_chunks.size()) { + return false; + } + aligned_tgt_chunks.insert(aligned_tgt_chunks.end(), split_tgt_chunks.begin() + i + 1, split_tgt_chunks.end()); CHECK(!aligned_tgt_chunks.empty()); @@ -1024,8 +1029,10 @@ void ZipModeImage::AddSplitImageFromChunkList(const ZipModeImage& tgt_image, size_t end_offset = aligned_tgt_chunks.back().GetStartOffset() + aligned_tgt_chunks.back().GetRawDataLength(); if (end_offset % BLOCK_SIZE != 0 && end_offset < tgt_image.file_content_.size()) { + size_t tail_block_length = std::min<size_t>(tgt_image.file_content_.size() - end_offset, + BLOCK_SIZE - (end_offset % BLOCK_SIZE)); aligned_tgt_chunks.emplace_back(CHUNK_NORMAL, end_offset, &tgt_image.file_content_, - BLOCK_SIZE - (end_offset % BLOCK_SIZE)); + tail_block_length); } ZipModeImage split_tgt_image(false); @@ -1049,6 +1056,8 @@ void ZipModeImage::AddSplitImageFromChunkList(const ZipModeImage& tgt_image, split_tgt_images->push_back(std::move(split_tgt_image)); split_src_images->push_back(std::move(split_src_image)); + + return true; } void ZipModeImage::ValidateSplitImages(const std::vector<ZipModeImage>& split_tgt_images, @@ -1536,7 +1545,7 @@ int imgdiff(int argc, const char** argv) { " patches together and output them into <patch-file>.\n" " --split-info, Output the split information (patch_size, tgt_size, src_ranges);\n" " zip mode with block-limit only.\n" - " --debug_dir, Debug directory to put the split srcs and patches, zip mode only.\n" + " --debug-dir, Debug directory to put the split srcs and patches, zip mode only.\n" " -v, --verbose, Enable verbose logging."; return 2; } diff --git a/applypatch/include/applypatch/imgdiff_image.h b/applypatch/include/applypatch/imgdiff_image.h index 0f74420f0..084807237 100644 --- a/applypatch/include/applypatch/imgdiff_image.h +++ b/applypatch/include/applypatch/imgdiff_image.h @@ -265,8 +265,9 @@ class ZipModeImage : public Image { std::vector<SortedRangeSet>& split_src_ranges, size_t total_tgt_size); // Construct the dummy split images based on the chunks info and source ranges; and move them into - // the given vectors. - static void AddSplitImageFromChunkList(const ZipModeImage& tgt_image, + // the given vectors. Return true if we add a new split image into |split_tgt_images|, and + // false otherwise. + static bool AddSplitImageFromChunkList(const ZipModeImage& tgt_image, const ZipModeImage& src_image, const SortedRangeSet& split_src_ranges, const std::vector<ImageChunk>& split_tgt_chunks, @@ -189,7 +189,7 @@ static int exec_cmd(const std::vector<std::string>& args) { argv.push_back(nullptr); pid_t child; - if ((child = vfork()) == 0) { + if ((child = fork()) == 0) { execv(argv[0], argv.data()); _exit(EXIT_FAILURE); } diff --git a/tests/component/imgdiff_test.cpp b/tests/component/imgdiff_test.cpp index 728b6cc76..6c23def01 100644 --- a/tests/component/imgdiff_test.cpp +++ b/tests/component/imgdiff_test.cpp @@ -969,3 +969,104 @@ TEST(ImgdiffTest, zip_mode_large_enough_limit) { // Expect 1 piece of patch since limit is larger than the zip file size. GenerateAndCheckSplitTarget(debug_dir.path, 1, tgt); } + +TEST(ImgdiffTest, zip_mode_large_apk_small_target_chunk) { + TemporaryFile tgt_file; + FILE* tgt_file_ptr = fdopen(tgt_file.release(), "wb"); + ZipWriter tgt_writer(tgt_file_ptr); + + // The first entry is less than 4096 bytes, followed immediately by an entry that has a very + // large counterpart in the source file. Therefore the first entry will be patched separately. + std::string small_chunk("a", 2000); + ASSERT_EQ(0, tgt_writer.StartEntry("a", 0)); + ASSERT_EQ(0, tgt_writer.WriteBytes(small_chunk.data(), small_chunk.size())); + ASSERT_EQ(0, tgt_writer.FinishEntry()); + construct_store_entry( + { + { "b", 12, 'b' }, { "c", 3, 'c' }, + }, + &tgt_writer); + ASSERT_EQ(0, tgt_writer.Finish()); + ASSERT_EQ(0, fclose(tgt_file_ptr)); + + TemporaryFile src_file; + FILE* src_file_ptr = fdopen(src_file.release(), "wb"); + ZipWriter src_writer(src_file_ptr); + construct_store_entry({ { "a", 1, 'a' }, { "b", 13, 'b' }, { "c", 1, 'c' } }, &src_writer); + ASSERT_EQ(0, src_writer.Finish()); + ASSERT_EQ(0, fclose(src_file_ptr)); + + // Compute patch. + TemporaryFile patch_file; + TemporaryFile split_info_file; + TemporaryDir debug_dir; + std::string split_info_arg = android::base::StringPrintf("--split-info=%s", split_info_file.path); + std::string debug_dir_arg = android::base::StringPrintf("--debug-dir=%s", debug_dir.path); + std::vector<const char*> args = { + "imgdiff", "-z", "--block-limit=10", split_info_arg.c_str(), debug_dir_arg.c_str(), + src_file.path, tgt_file.path, patch_file.path, + }; + ASSERT_EQ(0, imgdiff(args.size(), args.data())); + + std::string tgt; + ASSERT_TRUE(android::base::ReadFileToString(tgt_file.path, &tgt)); + + // Expect three split src images: + // src_piece 0: a 1 blocks + // src_piece 1: b-0 10 blocks + // src_piece 2: b-1 3 blocks, c 1 blocks, CD + GenerateAndCheckSplitTarget(debug_dir.path, 3, tgt); +} + +TEST(ImgdiffTest, zip_mode_large_apk_skipped_small_target_chunk) { + TemporaryFile tgt_file; + FILE* tgt_file_ptr = fdopen(tgt_file.release(), "wb"); + ZipWriter tgt_writer(tgt_file_ptr); + + construct_store_entry( + { + { "a", 11, 'a' }, + }, + &tgt_writer); + + // Construct a tiny target entry of 1 byte, which will be skipped due to the tail alignment of + // the previous entry. + std::string small_chunk("b", 1); + ASSERT_EQ(0, tgt_writer.StartEntry("b", 0)); + ASSERT_EQ(0, tgt_writer.WriteBytes(small_chunk.data(), small_chunk.size())); + ASSERT_EQ(0, tgt_writer.FinishEntry()); + + ASSERT_EQ(0, tgt_writer.Finish()); + ASSERT_EQ(0, fclose(tgt_file_ptr)); + + TemporaryFile src_file; + FILE* src_file_ptr = fdopen(src_file.release(), "wb"); + ZipWriter src_writer(src_file_ptr); + construct_store_entry( + { + { "a", 11, 'a' }, { "b", 11, 'b' }, + }, + &src_writer); + ASSERT_EQ(0, src_writer.Finish()); + ASSERT_EQ(0, fclose(src_file_ptr)); + + // Compute patch. + TemporaryFile patch_file; + TemporaryFile split_info_file; + TemporaryDir debug_dir; + std::string split_info_arg = android::base::StringPrintf("--split-info=%s", split_info_file.path); + std::string debug_dir_arg = android::base::StringPrintf("--debug-dir=%s", debug_dir.path); + std::vector<const char*> args = { + "imgdiff", "-z", "--block-limit=10", split_info_arg.c_str(), debug_dir_arg.c_str(), + src_file.path, tgt_file.path, patch_file.path, + }; + ASSERT_EQ(0, imgdiff(args.size(), args.data())); + + std::string tgt; + ASSERT_TRUE(android::base::ReadFileToString(tgt_file.path, &tgt)); + + // Expect two split src images: + // src_piece 0: a-0 10 blocks + // src_piece 1: a-0 1 block, CD + GenerateAndCheckSplitTarget(debug_dir.path, 2, tgt); +} |