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