Merge "libziparchive: report errors on over-long names." am: d62b9ca271
am: d43d5baed5
Change-Id: Ida08cff20b20f960057ee4018217555e2f16d2cd
diff --git a/fastboot/fastboot.cpp b/fastboot/fastboot.cpp
index f8f7eb3..25df451 100644
--- a/fastboot/fastboot.cpp
+++ b/fastboot/fastboot.cpp
@@ -502,9 +502,8 @@
static bool UnzipToMemory(ZipArchiveHandle zip, const std::string& entry_name,
std::vector<char>* 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 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 ab38dfd..32c6ea8 100644
--- a/libziparchive/include/ziparchive/zip_archive.h
+++ b/libziparchive/include/ziparchive/zip_archive.h
@@ -25,6 +25,8 @@
#include <sys/cdefs.h>
#include <sys/types.h>
+#include <string_view>
+
#include "android-base/off64_t.h"
/* Zip compression methods we support */
@@ -39,10 +41,7 @@
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 @@
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 @@
* 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 @@
*
* 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 596786a..bc7103b 100644
--- a/libziparchive/zip_archive.cc
+++ b/libziparchive/zip_archive.cc
@@ -690,8 +690,7 @@
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 @@
delete reinterpret_cast<IterationHandle*>(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 @@
return FindEntry(archive, static_cast<uint32_t>(ent), data);
}
+int32_t FindEntry(const ZipArchiveHandle archive, const std::string_view entryName,
+ ZipEntry* data) {
+ if (entryName.empty() || entryName.size() > static_cast<size_t>(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<int>(entryName.size()), entryName.data());
+ return static_cast<int32_t>(ent); // kEntryNotFound is safe to truncate.
+ }
+ // We know there are at most hast_table_size entries, safe to truncate.
+ return FindEntry(archive, static_cast<uint32_t>(ent), data);
+}
+
int32_t Next(void* cookie, ZipEntry* data, ZipString* name) {
IterationHandle* handle = reinterpret_cast<IterationHandle*>(cookie);
if (handle == NULL) {
@@ -1152,8 +1169,9 @@
return archive->mapped_zip.GetFileDescriptor();
}
-ZipString::ZipString(const char* entry_name) : name(reinterpret_cast<const uint8_t*>(entry_name)) {
- size_t len = strlen(entry_name);
+ZipString::ZipString(std::string_view entry_name)
+ : name(reinterpret_cast<const uint8_t*>(entry_name.data())) {
+ size_t len = entry_name.size();
CHECK_LE(len, static_cast<size_t>(UINT16_MAX));
name_length = static_cast<uint16_t>(len);
}
diff --git a/libziparchive/zip_archive_benchmark.cpp b/libziparchive/zip_archive_benchmark.cpp
index 46aa5a6..52166a4 100644
--- a/libziparchive/zip_archive_benchmark.cpp
+++ b/libziparchive/zip_archive_benchmark.cpp
@@ -55,7 +55,7 @@
// 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 e471d5e..cfbce1c 100644
--- a/libziparchive/zip_archive_test.cc
+++ b/libziparchive/zip_archive_test.cc
@@ -64,12 +64,6 @@
return OpenArchive(abs_path.c_str(), handle);
}
-static void SetZipString(ZipString* zip_str, const std::string& str) {
- zip_str->name = reinterpret_cast<const uint8_t*>(str.c_str());
- CHECK_LE(str.size(), std::numeric_limits<uint16_t>::max());
- zip_str->name_length = static_cast<uint16_t>(str.size());
-}
-
TEST(ziparchive, Open) {
ZipArchiveHandle handle;
ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle));
@@ -192,9 +186,7 @@
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 @@
ASSERT_EQ(static_cast<uint32_t>(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 @@
// 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 @@
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 @@
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<uint32_t>(0), entry.uncompressed_length);
uint8_t buffer[1];
ASSERT_EQ(0, ExtractToMemory(handle, &entry, buffer, 1));
@@ -327,9 +332,7 @@
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 @@
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 @@
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 @@
static void ZipArchiveStreamTest(ZipArchiveHandle& handle, const std::string& entry_name, bool raw,
bool verified, ZipEntry* entry, std::vector<uint8_t>* 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<ZipArchiveStreamEntry> stream;
if (raw) {
stream.reset(ZipArchiveStreamEntry::CreateRaw(handle, *entry));
@@ -589,11 +586,7 @@
// 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<uint32_t>(12), entry.uncompressed_length);
entry_out->resize(12);
diff --git a/libziparchive/zip_writer_test.cc b/libziparchive/zip_writer_test.cc
index 63adbbc..c3da23c 100644
--- a/libziparchive/zip_writer_test.cc
+++ b/libziparchive/zip_writer_test.cc
@@ -62,7 +62,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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);