From fa188268e43ab75732a480d6b2ec748d9d0dbfae Mon Sep 17 00:00:00 2001 From: Alex Deymo Date: Tue, 10 Oct 2017 17:56:17 +0200 Subject: Use SuffixArrayIndexInterface opaque type instead of the underlying data pointer. bsdiff interface is changing such that it hides the suffix array pointer from the public interface. This allows to use a different suffix array data size depending on the input size, running much faster in the normal case. Bug: 34220646 Test: `make checkbuild`; Ran an incremental update generation on a non-A/B device. Change-Id: I78e766da56cf28bc7774b8c8e58527bc11d919fb --- applypatch/imgdiff.cpp | 12 +++++++----- applypatch/include/applypatch/imgdiff_image.h | 5 +++-- tests/component/updater_test.cpp | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/applypatch/imgdiff.cpp b/applypatch/imgdiff.cpp index c887a854d..ccd68dc3e 100644 --- a/applypatch/imgdiff.cpp +++ b/applypatch/imgdiff.cpp @@ -161,7 +161,7 @@ #include #include #include -#include +#include #include #include @@ -322,7 +322,8 @@ void ImageChunk::MergeAdjacentNormal(const ImageChunk& other) { } bool ImageChunk::MakePatch(const ImageChunk& tgt, const ImageChunk& src, - std::vector* patch_data, saidx_t** bsdiff_cache) { + std::vector* patch_data, + bsdiff::SuffixArrayIndexInterface** bsdiff_cache) { #if defined(__ANDROID__) char ptemp[] = "/data/local/tmp/imgdiff-patch-XXXXXX"; #else @@ -1081,7 +1082,7 @@ bool ZipModeImage::GeneratePatchesInternal(const ZipModeImage& tgt_image, printf("Construct patches for %zu chunks...\n", tgt_image.NumOfChunks()); patch_chunks->clear(); - saidx_t* bsdiff_cache = nullptr; + bsdiff::SuffixArrayIndexInterface* bsdiff_cache = nullptr; for (size_t i = 0; i < tgt_image.NumOfChunks(); i++) { const auto& tgt_chunk = tgt_image[i]; @@ -1095,7 +1096,8 @@ bool ZipModeImage::GeneratePatchesInternal(const ZipModeImage& tgt_image, : src_image.FindChunkByName(tgt_chunk.GetEntryName()); const auto& src_ref = (src_chunk == nullptr) ? src_image.PseudoSource() : *src_chunk; - saidx_t** bsdiff_cache_ptr = (src_chunk == nullptr) ? &bsdiff_cache : nullptr; + bsdiff::SuffixArrayIndexInterface** bsdiff_cache_ptr = + (src_chunk == nullptr) ? &bsdiff_cache : nullptr; std::vector patch_data; if (!ImageChunk::MakePatch(tgt_chunk, src_ref, &patch_data, bsdiff_cache_ptr)) { @@ -1112,7 +1114,7 @@ bool ZipModeImage::GeneratePatchesInternal(const ZipModeImage& tgt_image, patch_chunks->emplace_back(tgt_chunk, src_ref, std::move(patch_data)); } } - free(bsdiff_cache); + delete bsdiff_cache; CHECK_EQ(patch_chunks->size(), tgt_image.NumOfChunks()); return true; diff --git a/applypatch/include/applypatch/imgdiff_image.h b/applypatch/include/applypatch/imgdiff_image.h index 491043dc1..4e915e5e7 100644 --- a/applypatch/include/applypatch/imgdiff_image.h +++ b/applypatch/include/applypatch/imgdiff_image.h @@ -24,7 +24,7 @@ #include #include -#include +#include #include #include @@ -98,7 +98,8 @@ class ImageChunk { * repeatedly, pass nullptr if not needed. */ static bool MakePatch(const ImageChunk& tgt, const ImageChunk& src, - std::vector* patch_data, saidx_t** bsdiff_cache); + std::vector* patch_data, + bsdiff::SuffixArrayIndexInterface** bsdiff_cache); private: const uint8_t* GetRawData() const; diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp index 2a0575a31..19653ec50 100644 --- a/tests/component/updater_test.cpp +++ b/tests/component/updater_test.cpp @@ -32,7 +32,7 @@ #include #include #include -#include +#include #include #include #include -- cgit v1.2.3 From fdec10333591a885bdc1667088811cdc8bceffb7 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Tue, 24 Oct 2017 12:25:41 -0700 Subject: applypatch: Fix a memory leak in ApplyImagePatch(). $ valgrind --leak-check=full out/host/linux-x86/nativetest64/recovery_host_test/recovery_host_test ==36755== 112 bytes in 1 blocks are definitely lost in loss record 4 of 16 ==36755== at 0x40307C4: malloc (valgrind/coregrind/m_replacemalloc/vg_replace_malloc.c:270) ==36755== by 0x40C1669: operator new(unsigned long) (external/libcxxabi/src/cxa_new_delete.cpp:46) ==36755== by 0x18D6A8: ApplyImagePatch(unsigned char const*, unsigned long, Value const*, std::__1::function, sha_state_st*, Value const*) (bootable/recovery/applypatch/imgpatch.cpp:62) ==36755== by 0x18D02B: ApplyImagePatch(unsigned char const*, unsigned long, unsigned char const*, unsigned long, std::__1::function) (bootable/recovery/applypatch/imgpatch.cpp:134) ==36755== by 0x160D15: GenerateTarget(std::__1::basic_string, std::__1::allocator > const&, std::__1::basic_string, std::__1::allocator > const&, std::__1::basic_string, std::__1::allocator >*) (bootable/recovery/tests/component/imgdiff_test.cpp:85) ==36755== by 0x11FA7D: verify_patched_image(std::__1::basic_string, std::__1::allocator > const&, std::__1::basic_string, std::__1::allocator > const&, std::__1::basic_string, std::__1::allocator > const&) (bootable/recovery/tests/component/imgdiff_test.cpp:96) ==36755== by 0x12966C: ImgdiffTest_zip_mode_smoke_trailer_zeros_Test::TestBody() (bootable/recovery/tests/component/imgdiff_test.cpp:295) ==36755== by 0x235EF9: testing::Test::Run() (external/googletest/googletest/src/gtest.cc:2455) ==36755== by 0x236CBF: testing::TestInfo::Run() (external/googletest/googletest/src/gtest.cc:2653) ==36755== by 0x2372D6: testing::TestCase::Run() (external/googletest/googletest/src/gtest.cc:2771) ==36755== by 0x23EEE6: testing::internal::UnitTestImpl::RunAllTests() (external/googletest/googletest/src/gtest.cc:4648) ==36755== by 0x23EB45: testing::UnitTest::Run() (external/googletest/googletest/src/gtest.cc:2455) std::unique_ptr strm(new z_stream(), deflateEnd); Only the internally allocated buffers inside 'strm' would be free'd by deflateEnd(), but not 'strm' itself. This CL fixes the issue by moving 'strm' to stack variable. Note that we only need to call deflateEnd() on successful return of deflateInit2(). Test: recovery_host_test && recovery_component_test Change-Id: I39b9bdf62376b8029f95cab82c8542bfcb874009 --- applypatch/imgpatch.cpp | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/applypatch/imgpatch.cpp b/applypatch/imgpatch.cpp index 7a43ddbef..25ba0a182 100644 --- a/applypatch/imgpatch.cpp +++ b/applypatch/imgpatch.cpp @@ -59,13 +59,13 @@ static bool ApplyBSDiffPatchAndStreamOutput(const uint8_t* src_data, size_t src_ int mem_level = Read4(deflate_header + 52); int strategy = Read4(deflate_header + 56); - std::unique_ptr strm(new z_stream(), deflateEnd); - strm->zalloc = Z_NULL; - strm->zfree = Z_NULL; - strm->opaque = Z_NULL; - strm->avail_in = 0; - strm->next_in = nullptr; - int ret = deflateInit2(strm.get(), level, method, window_bits, mem_level, strategy); + z_stream strm; + strm.zalloc = Z_NULL; + strm.zfree = Z_NULL; + strm.opaque = Z_NULL; + strm.avail_in = 0; + strm.next_in = nullptr; + int ret = deflateInit2(&strm, level, method, window_bits, mem_level, strategy); if (ret != Z_OK) { LOG(ERROR) << "Failed to init uncompressed data deflation: " << ret; return false; @@ -76,18 +76,19 @@ static bool ApplyBSDiffPatchAndStreamOutput(const uint8_t* src_data, size_t src_ size_t actual_target_length = 0; size_t total_written = 0; static constexpr size_t buffer_size = 32768; - auto compression_sink = [&](const uint8_t* data, size_t len) -> size_t { + auto compression_sink = [&strm, &actual_target_length, &expected_target_length, &total_written, + &ret, &ctx, &sink](const uint8_t* data, size_t len) -> size_t { // The input patch length for an update never exceeds INT_MAX. - strm->avail_in = len; - strm->next_in = data; + strm.avail_in = len; + strm.next_in = data; do { std::vector buffer(buffer_size); - strm->avail_out = buffer_size; - strm->next_out = buffer.data(); + strm.avail_out = buffer_size; + strm.next_out = buffer.data(); if (actual_target_length + len < expected_target_length) { - ret = deflate(strm.get(), Z_NO_FLUSH); + ret = deflate(&strm, Z_NO_FLUSH); } else { - ret = deflate(strm.get(), Z_FINISH); + ret = deflate(&strm, Z_FINISH); } if (ret != Z_OK && ret != Z_STREAM_END) { LOG(ERROR) << "Failed to deflate stream: " << ret; @@ -95,20 +96,24 @@ static bool ApplyBSDiffPatchAndStreamOutput(const uint8_t* src_data, size_t src_ return 0; } - size_t have = buffer_size - strm->avail_out; + size_t have = buffer_size - strm.avail_out; total_written += have; if (sink(buffer.data(), have) != have) { LOG(ERROR) << "Failed to write " << have << " compressed bytes to output."; return 0; } if (ctx) SHA1_Update(ctx, buffer.data(), have); - } while ((strm->avail_in != 0 || strm->avail_out == 0) && ret != Z_STREAM_END); + } while ((strm.avail_in != 0 || strm.avail_out == 0) && ret != Z_STREAM_END); actual_target_length += len; return len; }; - if (ApplyBSDiffPatch(src_data, src_len, patch, patch_offset, compression_sink, nullptr) != 0) { + int bspatch_result = + ApplyBSDiffPatch(src_data, src_len, patch, patch_offset, compression_sink, nullptr); + deflateEnd(&strm); + + if (bspatch_result != 0) { return false; } -- cgit v1.2.3