From a720635be20f4fc3acd39c7b83b919140ced0f94 Mon Sep 17 00:00:00 2001 From: Fabien Sanglard Date: Tue, 20 Oct 2020 15:47:10 -0700 Subject: [PATCH] Fix zipalign alignment error Problem: Zipalign operates over several false assumptions. First it assumes that zip entries are in the same order in the body and in the Central Direcotry. Second, it assumes there are not space between entries. This makes alignment incorrect when these asserts are not true. Solution: Don't align entries by tracking bias based on input zip entry location. Calculate the expected alignment based on the out- put zip file and correct with extra padding. Fixes: 162117652 Test: Units Tests Change-Id: Ia179338f658cab18a377cba2c7c8e629089a2785 --- tools/zipalign/Android.bp | 2 ++ tools/zipalign/ZipAlign.cpp | 13 +------- tools/zipalign/ZipFile.cpp | 37 +++++++++++++++++++---- tools/zipalign/ZipFile.h | 10 +++--- tools/zipalign/tests/data/diffOrders.zip | Bin 0 -> 220 bytes tools/zipalign/tests/data/holes.zip | Bin 0 -> 137 bytes tools/zipalign/tests/src/align_test.cpp | 26 ++++++++++++++++ 7 files changed, 66 insertions(+), 22 deletions(-) create mode 100644 tools/zipalign/tests/data/diffOrders.zip create mode 100644 tools/zipalign/tests/data/holes.zip diff --git a/tools/zipalign/Android.bp b/tools/zipalign/Android.bp index d88ee740dc..135cd766ea 100644 --- a/tools/zipalign/Android.bp +++ b/tools/zipalign/Android.bp @@ -63,6 +63,8 @@ cc_test_host { "libgmock", ], data: [ + "tests/data/diffOrders.zip", + "tests/data/holes.zip", "tests/data/unaligned.zip", ], defaults: ["zipalign_defaults"], diff --git a/tools/zipalign/ZipAlign.cpp b/tools/zipalign/ZipAlign.cpp index 1851ac562c..08f67ff9d0 100644 --- a/tools/zipalign/ZipAlign.cpp +++ b/tools/zipalign/ZipAlign.cpp @@ -47,7 +47,6 @@ static int copyAndAlign(ZipFile* pZin, ZipFile* pZout, int alignment, bool zopfl { int numEntries = pZin->getNumEntries(); ZipEntry* pEntry; - int bias = 0; status_t status; for (int i = 0; i < numEntries; i++) { @@ -68,30 +67,20 @@ static int copyAndAlign(ZipFile* pZin, ZipFile* pZout, int alignment, bool zopfl if (zopfli) { status = pZout->addRecompress(pZin, pEntry, &pNewEntry); - bias += pNewEntry->getCompressedLen() - pEntry->getCompressedLen(); } else { status = pZout->add(pZin, pEntry, padding, &pNewEntry); } } else { const int alignTo = getAlignment(pageAlignSharedLibs, alignment, pEntry); - /* - * Copy the entry, adjusting as required. We assume that the - * file position in the new file will be equal to the file - * position in the original. - */ - off_t newOffset = pEntry->getFileOffset() + bias; - padding = (alignTo - (newOffset % alignTo)) % alignTo; - //printf("--- %s: orig at %ld(+%d) len=%ld, adding pad=%d\n", // pEntry->getFileName(), (long) pEntry->getFileOffset(), // bias, (long) pEntry->getUncompressedLen(), padding); - status = pZout->add(pZin, pEntry, padding, &pNewEntry); + status = pZout->add(pZin, pEntry, alignTo, &pNewEntry); } if (status != OK) return 1; - bias += padding; //printf(" added '%s' at %ld (pad=%d)\n", // pNewEntry->getFileName(), (long) pNewEntry->getFileOffset(), // padding); diff --git a/tools/zipalign/ZipFile.cpp b/tools/zipalign/ZipFile.cpp index 29d1bc6849..9938a06088 100644 --- a/tools/zipalign/ZipFile.cpp +++ b/tools/zipalign/ZipFile.cpp @@ -502,6 +502,32 @@ bail: return result; } +/* + * Based on the current position in the output zip, assess where the entry + * payload will end up if written as-is. If alignment is not satisfactory, + * add some padding in the extra field. + * + */ +status_t ZipFile::alignEntry(android::ZipEntry* pEntry, uint32_t alignTo){ + if (alignTo == 0 || alignTo == 1) + return OK; + + // Calculate where the entry payload offset will end up if we were to write + // it as-is. + uint64_t expectedPayloadOffset = ftell(mZipFp) + + android::ZipEntry::LocalFileHeader::kLFHLen + + pEntry->mLFH.mFileNameLength + + pEntry->mLFH.mExtraFieldLength; + + // If the alignment is not what was requested, add some padding in the extra + // so the payload ends up where is requested. + uint64_t alignDiff = alignTo - (expectedPayloadOffset % alignTo); + if (alignDiff == 0) + return OK; + + return pEntry->addPadding(alignDiff); +} + /* * Add an entry by copying it from another zip file. If "padding" is * nonzero, the specified number of bytes will be added to the "extra" @@ -510,7 +536,7 @@ bail: * If "ppEntry" is non-NULL, a pointer to the new entry will be returned. */ status_t ZipFile::add(const ZipFile* pSourceZip, const ZipEntry* pSourceEntry, - int padding, ZipEntry** ppEntry) + int alignTo, ZipEntry** ppEntry) { ZipEntry* pEntry = NULL; status_t result; @@ -537,11 +563,10 @@ status_t ZipFile::add(const ZipFile* pSourceZip, const ZipEntry* pSourceEntry, result = pEntry->initFromExternal(pSourceEntry); if (result != OK) goto bail; - if (padding != 0) { - result = pEntry->addPadding(padding); - if (result != OK) - goto bail; - } + + result = alignEntry(pEntry, alignTo); + if (result != OK) + goto bail; /* * From here on out, failures are more interesting. diff --git a/tools/zipalign/ZipFile.h b/tools/zipalign/ZipFile.h index 11d20c5a2a..854f981a32 100644 --- a/tools/zipalign/ZipFile.h +++ b/tools/zipalign/ZipFile.h @@ -102,14 +102,14 @@ public: } /* - * Add an entry by copying it from another zip file. If "padding" is - * nonzero, the specified number of bytes will be added to the "extra" - * field in the header. + * Add an entry by copying it from another zip file. If "alignment" is + * nonzero, an appropriate number of bytes will be added to the "extra" + * field in the header so the entry payload is aligned. * * If "ppEntry" is non-NULL, a pointer to the new entry will be returned. */ status_t add(const ZipFile* pSourceZip, const ZipEntry* pSourceEntry, - int padding, ZipEntry** ppEntry); + int alignment, ZipEntry** ppEntry); /* * Add an entry by copying it from another zip file, recompressing with @@ -163,6 +163,8 @@ private: ZipFile(const ZipFile& src); ZipFile& operator=(const ZipFile& src); + status_t alignEntry(android::ZipEntry* pEntry, uint32_t alignTo); + class EndOfCentralDir { public: EndOfCentralDir(void) : diff --git a/tools/zipalign/tests/data/diffOrders.zip b/tools/zipalign/tests/data/diffOrders.zip new file mode 100644 index 0000000000000000000000000000000000000000..8f512ed49b2e6f15d5f30e82b97cca5fb14dbbd5 GIT binary patch literal 220 zcmWIWW@cdk14Rx+CI|~gGJ_ZaI2C}TQ&Q7V74*Jmy2J>S0AY}V#H3`P5sXY=4v>#- t3)l{zI0$TOR0NV>BT+S=8wyqp*5CvrfmQ@~v$BB%nSd|}NT+}}3;@ts5<>t0 literal 0 HcmV?d00001 diff --git a/tools/zipalign/tests/data/holes.zip b/tools/zipalign/tests/data/holes.zip new file mode 100644 index 0000000000000000000000000000000000000000..c88f891c707afd242125682227e2c34b1e153882 GIT binary patch literal 137 zcmWIWW@cdk14Rx+CI|~gGJ_ZaI2C}TQ&Q6cycwAoz)Bey(9M7fgALr)s0h=*$_5f) N1VTd~?F8a50073M2c!T1 literal 0 HcmV?d00001 diff --git a/tools/zipalign/tests/src/align_test.cpp b/tools/zipalign/tests/src/align_test.cpp index 073a15678f..cbd92187f3 100644 --- a/tools/zipalign/tests/src/align_test.cpp +++ b/tools/zipalign/tests/src/align_test.cpp @@ -22,3 +22,29 @@ TEST(Align, Unaligned) { int result = process(src.c_str(), dst.c_str(), 4, true, false, 4096); ASSERT_EQ(0, result); } + +// Align a zip featuring a hole at the beginning. The +// hole in the archive is a delete entry in the Central +// Directory. +TEST(Align, Holes) { + const std::string src = GetTestPath("holes.zip"); + const std::string dst = GetTestPath("holes_out.zip"); + + int processed = process(src.c_str(), dst.c_str(), 4, true, false, 4096); + ASSERT_EQ(0, processed); + + int verified = verify(dst.c_str(), 4, false, true); + ASSERT_EQ(0, verified); +} + +// Align a zip where LFH order and CD entries differ. +TEST(Align, DifferenteOrders) { + const std::string src = GetTestPath("diffOrders.zip"); + const std::string dst = GetTestPath("diffOrders_out.zip"); + + int processed = process(src.c_str(), dst.c_str(), 4, true, false, 4096); + ASSERT_EQ(0, processed); + + int verified = verify(dst.c_str(), 4, false, true); + ASSERT_EQ(0, verified); +}