From b17bf521d5c8925c82d9242c331256c95148620b Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 3 May 2019 22:38:44 -0700 Subject: [PATCH] libziparchive: report errors on over-long names. Switch FindEntry and the ZipString constructor to std::string_view. This lets us accept an over-long name so that we can reject it as too long. Also fastboot changes to track the API change. Bug: http://b/129068177 Test: treehugger Change-Id: I7df7acd1fe2c46380b789c25f8909e0553e2d55e --- fastboot/fastboot.cpp | 6 +- .../include/ziparchive/zip_archive.h | 13 ++-- libziparchive/zip_archive.cc | 26 +++++-- libziparchive/zip_archive_benchmark.cpp | 2 +- libziparchive/zip_archive_test.cc | 69 +++++++++---------- libziparchive/zip_writer_test.cc | 26 +++---- 6 files changed, 76 insertions(+), 66 deletions(-) diff --git a/fastboot/fastboot.cpp b/fastboot/fastboot.cpp index f8f7eb36e..25df4516f 100644 --- a/fastboot/fastboot.cpp +++ b/fastboot/fastboot.cpp @@ -502,9 +502,8 @@ static std::vector LoadBootableImage(const std::string& kernel, const std: static bool UnzipToMemory(ZipArchiveHandle zip, const std::string& entry_name, std::vector* out) { - ZipString zip_entry_name(entry_name.c_str()); ZipEntry zip_entry; - if (FindEntry(zip, zip_entry_name, &zip_entry) != 0) { + if (FindEntry(zip, entry_name, &zip_entry) != 0) { fprintf(stderr, "archive does not contain '%s'\n", entry_name.c_str()); return false; } @@ -614,9 +613,8 @@ static void delete_fbemarker_tmpdir(const std::string& dir) { static int unzip_to_file(ZipArchiveHandle zip, const char* entry_name) { unique_fd fd(make_temporary_fd(entry_name)); - ZipString zip_entry_name(entry_name); ZipEntry zip_entry; - if (FindEntry(zip, zip_entry_name, &zip_entry) != 0) { + if (FindEntry(zip, entry_name, &zip_entry) != 0) { fprintf(stderr, "archive does not contain '%s'\n", entry_name); errno = ENOENT; return -1; diff --git a/libziparchive/include/ziparchive/zip_archive.h b/libziparchive/include/ziparchive/zip_archive.h index ab38dfd2c..32c6ea830 100644 --- a/libziparchive/include/ziparchive/zip_archive.h +++ b/libziparchive/include/ziparchive/zip_archive.h @@ -25,6 +25,8 @@ #include #include +#include + #include "android-base/off64_t.h" /* Zip compression methods we support */ @@ -39,10 +41,7 @@ struct ZipString { ZipString() {} - /* - * entry_name has to be an c-style string with only ASCII characters. - */ - explicit ZipString(const char* entry_name); + explicit ZipString(std::string_view entry_name); bool operator==(const ZipString& rhs) const { return name && (name_length == rhs.name_length) && (memcmp(name, rhs.name, name_length) == 0); @@ -149,8 +148,7 @@ int32_t OpenArchiveFromMemory(void* address, size_t length, const char* debugFil void CloseArchive(ZipArchiveHandle archive); /* - * Find an entry in the Zip archive, by name. |entryName| must be a null - * terminated string, and |data| must point to a writeable memory location. + * Find an entry in the Zip archive, by name. |data| must be non-null. * * Returns 0 if an entry is found, and populates |data| with information * about this entry. Returns negative values otherwise. @@ -164,6 +162,8 @@ void CloseArchive(ZipArchiveHandle archive); * On non-Windows platforms this method does not modify internal state and * can be called concurrently. */ +int32_t FindEntry(const ZipArchiveHandle archive, const std::string_view entryName, ZipEntry* data); +// TODO: remove this internally, where there is a new user. int32_t FindEntry(const ZipArchiveHandle archive, const ZipString& entryName, ZipEntry* data); /* @@ -179,6 +179,7 @@ int32_t FindEntry(const ZipArchiveHandle archive, const ZipString& entryName, Zi * * Returns 0 on success and negative values on failure. */ +// TODO: switch these ZipStrings to std::string_view. int32_t StartIteration(ZipArchiveHandle archive, void** cookie_ptr, const ZipString* optional_prefix, const ZipString* optional_suffix); diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc index 596786a3a..bc7103b96 100644 --- a/libziparchive/zip_archive.cc +++ b/libziparchive/zip_archive.cc @@ -690,8 +690,7 @@ static int32_t FindEntry(const ZipArchive* archive, const int32_t ent, ZipEntry* struct IterationHandle { uint32_t position; - // We're not using vector here because this code is used in the Windows SDK - // where the STL is not available. + // TODO: switch these to std::string now that Windows uses libc++ too. ZipString prefix; ZipString suffix; ZipArchive* archive; @@ -742,6 +741,7 @@ void EndIteration(void* cookie) { delete reinterpret_cast(cookie); } +// TODO: remove this internally. int32_t FindEntry(const ZipArchiveHandle archive, const ZipString& entryName, ZipEntry* data) { if (entryName.name_length == 0) { ALOGW("Zip: Invalid filename %.*s", entryName.name_length, entryName.name); @@ -758,6 +758,23 @@ int32_t FindEntry(const ZipArchiveHandle archive, const ZipString& entryName, Zi return FindEntry(archive, static_cast(ent), data); } +int32_t FindEntry(const ZipArchiveHandle archive, const std::string_view entryName, + ZipEntry* data) { + if (entryName.empty() || entryName.size() > static_cast(UINT16_MAX)) { + ALOGW("Zip: Invalid filename of length %zu", entryName.size()); + return kInvalidEntryName; + } + + const int64_t ent = EntryToIndex(archive->hash_table, archive->hash_table_size, + ZipString(entryName), archive->central_directory.GetBasePtr()); + if (ent < 0) { + ALOGV("Zip: Could not find entry %.*s", static_cast(entryName.size()), entryName.data()); + return static_cast(ent); // kEntryNotFound is safe to truncate. + } + // We know there are at most hast_table_size entries, safe to truncate. + return FindEntry(archive, static_cast(ent), data); +} + int32_t Next(void* cookie, ZipEntry* data, ZipString* name) { IterationHandle* handle = reinterpret_cast(cookie); if (handle == NULL) { @@ -1152,8 +1169,9 @@ int GetFileDescriptor(const ZipArchiveHandle archive) { return archive->mapped_zip.GetFileDescriptor(); } -ZipString::ZipString(const char* entry_name) : name(reinterpret_cast(entry_name)) { - size_t len = strlen(entry_name); +ZipString::ZipString(std::string_view entry_name) + : name(reinterpret_cast(entry_name.data())) { + size_t len = entry_name.size(); CHECK_LE(len, static_cast(UINT16_MAX)); name_length = static_cast(len); } diff --git a/libziparchive/zip_archive_benchmark.cpp b/libziparchive/zip_archive_benchmark.cpp index 46aa5a68f..52166a479 100644 --- a/libziparchive/zip_archive_benchmark.cpp +++ b/libziparchive/zip_archive_benchmark.cpp @@ -55,7 +55,7 @@ static void FindEntry_no_match(benchmark::State& state) { // In order to walk through all file names in the archive, look for a name // that does not exist in the archive. - ZipString name("thisFileNameDoesNotExist"); + std::string_view name("thisFileNameDoesNotExist"); // Start the benchmark. while (state.KeepRunning()) { diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc index e471d5ea4..cfbce1cb7 100644 --- a/libziparchive/zip_archive_test.cc +++ b/libziparchive/zip_archive_test.cc @@ -64,12 +64,6 @@ static int32_t OpenArchiveWrapper(const std::string& name, ZipArchiveHandle* han return OpenArchive(abs_path.c_str(), handle); } -static void SetZipString(ZipString* zip_str, const std::string& str) { - zip_str->name = reinterpret_cast(str.c_str()); - CHECK_LE(str.size(), std::numeric_limits::max()); - zip_str->name_length = static_cast(str.size()); -} - TEST(ziparchive, Open) { ZipArchiveHandle handle; ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); @@ -192,9 +186,7 @@ TEST(ziparchive, FindEntry) { ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); ZipEntry data; - ZipString name; - SetZipString(&name, kATxtName); - ASSERT_EQ(0, FindEntry(handle, name, &data)); + ASSERT_EQ(0, FindEntry(handle, kATxtName, &data)); // Known facts about a.txt, from zipinfo -v. ASSERT_EQ(63, data.offset); @@ -205,9 +197,28 @@ TEST(ziparchive, FindEntry) { ASSERT_EQ(static_cast(0x438a8005), data.mod_time); // An entry that doesn't exist. Should be a negative return code. - ZipString absent_name; - SetZipString(&absent_name, kNonexistentTxtName); - ASSERT_LT(FindEntry(handle, absent_name, &data), 0); + ASSERT_LT(FindEntry(handle, kNonexistentTxtName, &data), 0); + + CloseArchive(handle); +} + +TEST(ziparchive, FindEntry_empty) { + ZipArchiveHandle handle; + ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); + + ZipEntry data; + ASSERT_EQ(kInvalidEntryName, FindEntry(handle, "", &data)); + + CloseArchive(handle); +} + +TEST(ziparchive, FindEntry_too_long) { + ZipArchiveHandle handle; + ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); + + std::string very_long_name(65536, 'x'); + ZipEntry data; + ASSERT_EQ(kInvalidEntryName, FindEntry(handle, very_long_name, &data)); CloseArchive(handle); } @@ -234,9 +245,7 @@ TEST(ziparchive, ExtractToMemory) { // An entry that's deflated. ZipEntry data; - ZipString a_name; - SetZipString(&a_name, kATxtName); - ASSERT_EQ(0, FindEntry(handle, a_name, &data)); + ASSERT_EQ(0, FindEntry(handle, kATxtName, &data)); const uint32_t a_size = data.uncompressed_length; ASSERT_EQ(a_size, kATxtContents.size()); uint8_t* buffer = new uint8_t[a_size]; @@ -245,9 +254,7 @@ TEST(ziparchive, ExtractToMemory) { delete[] buffer; // An entry that's stored. - ZipString b_name; - SetZipString(&b_name, kBTxtName); - ASSERT_EQ(0, FindEntry(handle, b_name, &data)); + ASSERT_EQ(0, FindEntry(handle, kBTxtName, &data)); const uint32_t b_size = data.uncompressed_length; ASSERT_EQ(b_size, kBTxtContents.size()); buffer = new uint8_t[b_size]; @@ -302,9 +309,7 @@ TEST(ziparchive, EmptyEntries) { ASSERT_EQ(0, OpenArchiveFd(tmp_file.fd, "EmptyEntriesTest", &handle, false)); ZipEntry entry; - ZipString empty_name; - SetZipString(&empty_name, kEmptyTxtName); - ASSERT_EQ(0, FindEntry(handle, empty_name, &entry)); + ASSERT_EQ(0, FindEntry(handle, kEmptyTxtName, &entry)); ASSERT_EQ(static_cast(0), entry.uncompressed_length); uint8_t buffer[1]; ASSERT_EQ(0, ExtractToMemory(handle, &entry, buffer, 1)); @@ -327,9 +332,7 @@ TEST(ziparchive, EntryLargerThan32K) { ASSERT_EQ(0, OpenArchiveFd(tmp_file.fd, "EntryLargerThan32KTest", &handle, false)); ZipEntry entry; - ZipString ab_name; - SetZipString(&ab_name, kAbTxtName); - ASSERT_EQ(0, FindEntry(handle, ab_name, &entry)); + ASSERT_EQ(0, FindEntry(handle, kAbTxtName, &entry)); ASSERT_EQ(kAbUncompressedSize, entry.uncompressed_length); // Extract the entry to memory. @@ -386,9 +389,7 @@ TEST(ziparchive, ExtractToFile) { ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); ZipEntry entry; - ZipString name; - SetZipString(&name, kATxtName); - ASSERT_EQ(0, FindEntry(handle, name, &entry)); + ASSERT_EQ(0, FindEntry(handle, kATxtName, &entry)); ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, tmp_file.fd)); // Assert that the first 8 bytes of the file haven't been clobbered. @@ -424,10 +425,8 @@ TEST(ziparchive, OpenFromMemory) { OpenArchiveFromMemory(file_map->data(), file_map->size(), zip_path.c_str(), &handle)); // Assert one entry can be found and extracted correctly. - std::string BINARY_PATH("META-INF/com/google/android/update-binary"); - ZipString binary_path(BINARY_PATH.c_str()); ZipEntry binary_entry; - ASSERT_EQ(0, FindEntry(handle, binary_path, &binary_entry)); + ASSERT_EQ(0, FindEntry(handle, "META-INF/com/google/android/update-binary", &binary_entry)); TemporaryFile tmp_binary; ASSERT_NE(-1, tmp_binary.fd); ASSERT_EQ(0, ExtractEntryToFile(handle, &binary_entry, tmp_binary.fd)); @@ -436,9 +435,7 @@ TEST(ziparchive, OpenFromMemory) { static void ZipArchiveStreamTest(ZipArchiveHandle& handle, const std::string& entry_name, bool raw, bool verified, ZipEntry* entry, std::vector* read_data) { - ZipString name; - SetZipString(&name, entry_name); - ASSERT_EQ(0, FindEntry(handle, name, entry)); + ASSERT_EQ(0, FindEntry(handle, entry_name, entry)); std::unique_ptr stream; if (raw) { stream.reset(ZipArchiveStreamEntry::CreateRaw(handle, *entry)); @@ -589,11 +586,7 @@ static void ExtractEntryToMemory(const std::vector& zip_data, // an entry whose name is "name" and whose size is 12 (contents = // "abdcdefghijk"). ZipEntry entry; - ZipString name; - std::string name_str = "name"; - SetZipString(&name, name_str); - - ASSERT_EQ(0, FindEntry(handle, name, &entry)); + ASSERT_EQ(0, FindEntry(handle, "name", &entry)); ASSERT_EQ(static_cast(12), entry.uncompressed_length); entry_out->resize(12); diff --git a/libziparchive/zip_writer_test.cc b/libziparchive/zip_writer_test.cc index 63adbbce0..c3da23c79 100644 --- a/libziparchive/zip_writer_test.cc +++ b/libziparchive/zip_writer_test.cc @@ -62,7 +62,7 @@ TEST_F(zipwriter, WriteUncompressedZipWithOneFile) { ASSERT_EQ(0, OpenArchiveFd(fd_, "temp", &handle, false)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("file.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "file.txt", &data)); EXPECT_EQ(kCompressStored, data.method); EXPECT_EQ(0u, data.has_data_descriptor); EXPECT_EQ(strlen(expected), data.compressed_length); @@ -95,19 +95,19 @@ TEST_F(zipwriter, WriteUncompressedZipWithMultipleFiles) { ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("file.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "file.txt", &data)); EXPECT_EQ(kCompressStored, data.method); EXPECT_EQ(2u, data.compressed_length); ASSERT_EQ(2u, data.uncompressed_length); ASSERT_TRUE(AssertFileEntryContentsEq("he", handle, &data)); - ASSERT_EQ(0, FindEntry(handle, ZipString("file/file.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "file/file.txt", &data)); EXPECT_EQ(kCompressStored, data.method); EXPECT_EQ(3u, data.compressed_length); ASSERT_EQ(3u, data.uncompressed_length); ASSERT_TRUE(AssertFileEntryContentsEq("llo", handle, &data)); - ASSERT_EQ(0, FindEntry(handle, ZipString("file/file2.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "file/file2.txt", &data)); EXPECT_EQ(kCompressStored, data.method); EXPECT_EQ(0u, data.compressed_length); EXPECT_EQ(0u, data.uncompressed_length); @@ -129,7 +129,7 @@ TEST_F(zipwriter, WriteUncompressedZipFileWithAlignedFlag) { ASSERT_EQ(0, OpenArchiveFd(fd_, "temp", &handle, false)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("align.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "align.txt", &data)); EXPECT_EQ(0, data.offset & 0x03); CloseArchive(handle); @@ -163,7 +163,7 @@ TEST_F(zipwriter, WriteUncompressedZipFileWithAlignedFlagAndTime) { ASSERT_EQ(0, OpenArchiveFd(fd_, "temp", &handle, false)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("align.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "align.txt", &data)); EXPECT_EQ(0, data.offset & 0x03); struct tm mod = data.GetModificationTime(); @@ -191,7 +191,7 @@ TEST_F(zipwriter, WriteUncompressedZipFileWithAlignedValue) { ASSERT_EQ(0, OpenArchiveFd(fd_, "temp", &handle, false)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("align.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "align.txt", &data)); EXPECT_EQ(0, data.offset & 0xfff); CloseArchive(handle); @@ -213,7 +213,7 @@ TEST_F(zipwriter, WriteUncompressedZipFileWithAlignedValueAndTime) { ASSERT_EQ(0, OpenArchiveFd(fd_, "temp", &handle, false)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("align.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "align.txt", &data)); EXPECT_EQ(0, data.offset & 0xfff); struct tm mod = data.GetModificationTime(); @@ -241,7 +241,7 @@ TEST_F(zipwriter, WriteCompressedZipWithOneFile) { ASSERT_EQ(0, OpenArchiveFd(fd_, "temp", &handle, false)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("file.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "file.txt", &data)); EXPECT_EQ(kCompressDeflated, data.method); ASSERT_EQ(4u, data.uncompressed_length); ASSERT_TRUE(AssertFileEntryContentsEq("helo", handle, &data)); @@ -273,7 +273,7 @@ TEST_F(zipwriter, WriteCompressedZipFlushFull) { ASSERT_EQ(0, OpenArchiveFd(fd_, "temp", &handle, false)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("file.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "file.txt", &data)); EXPECT_EQ(kCompressDeflated, data.method); EXPECT_EQ(kBufSize, data.uncompressed_length); @@ -340,12 +340,12 @@ TEST_F(zipwriter, BackupRemovesTheLastFile) { ASSERT_EQ(0, OpenArchiveFd(fd_, "temp", &handle, false)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, ZipString("keep.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "keep.txt", &data)); ASSERT_TRUE(AssertFileEntryContentsEq(kKeepThis, handle, &data)); - ASSERT_NE(0, FindEntry(handle, ZipString("drop.txt"), &data)); + ASSERT_NE(0, FindEntry(handle, "drop.txt", &data)); - ASSERT_EQ(0, FindEntry(handle, ZipString("replace.txt"), &data)); + ASSERT_EQ(0, FindEntry(handle, "replace.txt", &data)); ASSERT_TRUE(AssertFileEntryContentsEq(kReplaceWithThis, handle, &data)); CloseArchive(handle);