From d37ce8f0821758edc33ad9c42b0bf78ff29b365f Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Sat, 17 Dec 2016 17:10:04 -0800 Subject: imgdiff: Fix an edge case that leads to infinite loop. When the input image ends with the magic value sequence of 0x1f, 0x8b, 0x0b (optionally with 0x00), the image parsing code will be stuck in an infinite loop. Test: recovery_component_test passes. Change-Id: Ie3629dfdc41360387b19cc3e0359c95ae4fb998e --- applypatch/Android.mk | 1 + applypatch/imgdiff.cpp | 25 +++++-------- tests/component/imgdiff_test.cpp | 80 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 15 deletions(-) diff --git a/applypatch/Android.mk b/applypatch/Android.mk index 61e110617..ec3c6ee38 100644 --- a/applypatch/Android.mk +++ b/applypatch/Android.mk @@ -124,6 +124,7 @@ libimgdiff_cflags := \ libimgdiff_static_libraries := \ libbsdiff \ + libbase \ libz # libimgdiff (static library) diff --git a/applypatch/imgdiff.cpp b/applypatch/imgdiff.cpp index 18a15a164..62de726a4 100644 --- a/applypatch/imgdiff.cpp +++ b/applypatch/imgdiff.cpp @@ -124,6 +124,7 @@ #include "applypatch/imgdiff.h" #include +#include #include #include #include @@ -131,6 +132,9 @@ #include #include +#include +#include + #include #include @@ -382,19 +386,12 @@ unsigned char* ReadImage(const char* filename, int* num_chunks, ImageChunk** chu } size_t sz = static_cast(st.st_size); - unsigned char* img = static_cast(malloc(sz + 4)); - FILE* f = fopen(filename, "rb"); - if (fread(img, 1, sz, f) != sz) { + unsigned char* img = static_cast(malloc(sz)); + android::base::unique_fd fd(open(filename, O_RDONLY)); + if (!android::base::ReadFully(fd, img, sz)) { printf("failed to read \"%s\" %s\n", filename, strerror(errno)); - fclose(f); - return NULL; + return nullptr; } - fclose(f); - - // append 4 zero bytes to the data so we can always search for the - // four-byte string 1f8b0800 starting at any point in the actual - // file data, without special-casing the end of the data. - memset(img+sz, 0, 4); size_t pos = 0; @@ -518,10 +515,8 @@ unsigned char* ReadImage(const char* filename, int* num_chunks, ImageChunk** chu curr->data = p; for (curr->len = 0; curr->len < (sz - pos); ++curr->len) { - if (p[curr->len] == 0x1f && - p[curr->len+1] == 0x8b && - p[curr->len+2] == 0x08 && - p[curr->len+3] == 0x00) { + if (sz - pos >= 4 && p[curr->len] == 0x1f && p[curr->len + 1] == 0x8b && + p[curr->len + 2] == 0x08 && p[curr->len + 3] == 0x00) { break; } } diff --git a/tests/component/imgdiff_test.cpp b/tests/component/imgdiff_test.cpp index 3711859de..7ad330783 100644 --- a/tests/component/imgdiff_test.cpp +++ b/tests/component/imgdiff_test.cpp @@ -404,6 +404,86 @@ TEST(ImgdiffTest, image_mode_spurious_magic) { ASSERT_EQ(tgt, patched); } +TEST(ImgdiffTest, image_mode_short_input1) { + // src: "abcdefgh" + '0x1f8b0b'. + const std::vector src_data = { 'a', 'b', 'c', 'd', 'e', 'f', + 'g', 'h', '\x1f', '\x8b', '\x08' }; + const std::string src(src_data.cbegin(), src_data.cend()); + TemporaryFile src_file; + ASSERT_TRUE(android::base::WriteStringToFile(src, src_file.path)); + + // tgt: "abcdefgxyz". + const std::vector tgt_data = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'x', 'y', 'z' }; + const std::string tgt(tgt_data.cbegin(), tgt_data.cend()); + TemporaryFile tgt_file; + ASSERT_TRUE(android::base::WriteStringToFile(tgt, tgt_file.path)); + + TemporaryFile patch_file; + std::vector args = { + "imgdiff", src_file.path, tgt_file.path, patch_file.path, + }; + ASSERT_EQ(0, imgdiff(args.size(), args.data())); + + // Verify. + std::string patch; + ASSERT_TRUE(android::base::ReadFileToString(patch_file.path, &patch)); + + // Expect one CHUNK_RAW (header) entry. + size_t num_normal; + size_t num_raw; + size_t num_deflate; + verify_patch_header(patch, &num_normal, &num_raw, &num_deflate); + ASSERT_EQ(0U, num_normal); + ASSERT_EQ(0U, num_deflate); + ASSERT_EQ(1U, num_raw); + + std::string patched; + ASSERT_EQ(0, ApplyImagePatch(reinterpret_cast(src.data()), src.size(), + reinterpret_cast(patch.data()), patch.size(), + MemorySink, &patched)); + ASSERT_EQ(tgt, patched); +} + +TEST(ImgdiffTest, image_mode_short_input2) { + // src: "abcdefgh" + '0x1f8b0b00'. + const std::vector src_data = { 'a', 'b', 'c', 'd', 'e', 'f', + 'g', 'h', '\x1f', '\x8b', '\x08', '\x00' }; + const std::string src(src_data.cbegin(), src_data.cend()); + TemporaryFile src_file; + ASSERT_TRUE(android::base::WriteStringToFile(src, src_file.path)); + + // tgt: "abcdefgxyz". + const std::vector tgt_data = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'x', 'y', 'z' }; + const std::string tgt(tgt_data.cbegin(), tgt_data.cend()); + TemporaryFile tgt_file; + ASSERT_TRUE(android::base::WriteStringToFile(tgt, tgt_file.path)); + + TemporaryFile patch_file; + std::vector args = { + "imgdiff", src_file.path, tgt_file.path, patch_file.path, + }; + ASSERT_EQ(0, imgdiff(args.size(), args.data())); + + // Verify. + std::string patch; + ASSERT_TRUE(android::base::ReadFileToString(patch_file.path, &patch)); + + // Expect one CHUNK_RAW (header) entry. + size_t num_normal; + size_t num_raw; + size_t num_deflate; + verify_patch_header(patch, &num_normal, &num_raw, &num_deflate); + ASSERT_EQ(0U, num_normal); + ASSERT_EQ(0U, num_deflate); + ASSERT_EQ(1U, num_raw); + + std::string patched; + ASSERT_EQ(0, ApplyImagePatch(reinterpret_cast(src.data()), src.size(), + reinterpret_cast(patch.data()), patch.size(), + MemorySink, &patched)); + ASSERT_EQ(tgt, patched); +} + TEST(ImgdiffTest, image_mode_single_entry_long) { // src: "abcdefgh" + '0x1f8b0b00' + some bytes. const std::vector src_data = { 'a', 'b', 'c', 'd', 'e', 'f', 'g', -- cgit v1.2.3