libziparchive: start moving to a non-ZipString StartIteration API.
Same issue as with FindEntry: using ZipString in the API forces all
callers to make sure they don't hit the ZipString length limits. Switch
to std::string_view and uniformly use the empty string as a way to
signal no prefix/suffix rather than nullptr.
Also use default arguments to make the common case of no prefix and no
suffix more convenient.
Also just use std::string to increase the lifetime of the provided
prefix/suffix rather than manual memory management.
Bug: http://b/129068177
Test: treehugger
Change-Id: I6675e39ce62fadd766386d77d27423013c17d6f7
diff --git a/libziparchive/include/ziparchive/zip_archive.h b/libziparchive/include/ziparchive/zip_archive.h
index 6e837a4..7eead7e 100644
--- a/libziparchive/include/ziparchive/zip_archive.h
+++ b/libziparchive/include/ziparchive/zip_archive.h
@@ -177,7 +177,10 @@
*
* 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 std::string_view optional_prefix = "",
+ const std::string_view optional_suffix = "");
+// TODO: remove this.
int32_t StartIteration(ZipArchiveHandle archive, void** cookie_ptr,
const ZipString* optional_prefix, const ZipString* optional_suffix);
diff --git a/libziparchive/unzip.cpp b/libziparchive/unzip.cpp
index c6def73..3a3a694 100644
--- a/libziparchive/unzip.cpp
+++ b/libziparchive/unzip.cpp
@@ -249,7 +249,7 @@
// libziparchive iteration order doesn't match the central directory.
// We could sort, but that would cost extra and wouldn't match either.
void* cookie;
- int err = StartIteration(zah, &cookie, nullptr, nullptr);
+ int err = StartIteration(zah, &cookie);
if (err != 0) {
error(1, 0, "couldn't iterate %s: %s", archive_name, ErrorCodeString(err));
}
diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc
index 2bd8fb9..ac3e236 100644
--- a/libziparchive/zip_archive.cc
+++ b/libziparchive/zip_archive.cc
@@ -689,54 +689,59 @@
}
struct IterationHandle {
- uint32_t position;
- // TODO: switch these to std::string now that Windows uses libc++ too.
- ZipString prefix;
- ZipString suffix;
ZipArchive* archive;
- IterationHandle(const ZipString* in_prefix, const ZipString* in_suffix) {
- if (in_prefix) {
- uint8_t* name_copy = new uint8_t[in_prefix->name_length];
- memcpy(name_copy, in_prefix->name, in_prefix->name_length);
- prefix.name = name_copy;
- prefix.name_length = in_prefix->name_length;
- } else {
- prefix.name = NULL;
- prefix.name_length = 0;
- }
- if (in_suffix) {
- uint8_t* name_copy = new uint8_t[in_suffix->name_length];
- memcpy(name_copy, in_suffix->name, in_suffix->name_length);
- suffix.name = name_copy;
- suffix.name_length = in_suffix->name_length;
- } else {
- suffix.name = NULL;
- suffix.name_length = 0;
- }
- }
+ std::string prefix_holder;
+ ZipString prefix;
- ~IterationHandle() {
- delete[] prefix.name;
- delete[] suffix.name;
- }
+ std::string suffix_holder;
+ ZipString suffix;
+
+ uint32_t position = 0;
+
+ IterationHandle(ZipArchive* archive, const std::string_view in_prefix,
+ const std::string_view in_suffix)
+ : archive(archive),
+ prefix_holder(in_prefix),
+ prefix(prefix_holder),
+ suffix_holder(in_suffix),
+ suffix(suffix_holder) {}
};
int32_t StartIteration(ZipArchiveHandle archive, void** cookie_ptr,
- const ZipString* optional_prefix, const ZipString* optional_suffix) {
+ const std::string_view optional_prefix,
+ const std::string_view optional_suffix) {
if (archive == NULL || archive->hash_table == NULL) {
ALOGW("Zip: Invalid ZipArchiveHandle");
return kInvalidHandle;
}
- IterationHandle* cookie = new IterationHandle(optional_prefix, optional_suffix);
- cookie->position = 0;
- cookie->archive = archive;
+ if (optional_prefix.size() > static_cast<size_t>(UINT16_MAX) ||
+ optional_suffix.size() > static_cast<size_t>(UINT16_MAX)) {
+ ALOGW("Zip: prefix/suffix too long");
+ return kInvalidEntryName;
+ }
- *cookie_ptr = cookie;
+ *cookie_ptr = new IterationHandle(archive, optional_prefix, optional_suffix);
return 0;
}
+// TODO: remove this.
+int32_t StartIteration(ZipArchiveHandle archive, void** cookie_ptr,
+ const ZipString* optional_prefix, const ZipString* optional_suffix) {
+ std::string prefix;
+ if (optional_prefix) {
+ prefix = std::string(reinterpret_cast<const char*>(optional_prefix->name),
+ optional_prefix->name_length);
+ }
+ std::string suffix;
+ if (optional_suffix) {
+ suffix = std::string(reinterpret_cast<const char*>(optional_suffix->name),
+ optional_suffix->name_length);
+ }
+ return StartIteration(archive, cookie_ptr, prefix.c_str(), suffix.c_str());
+}
+
void EndIteration(void* cookie) {
delete reinterpret_cast<IterationHandle*>(cookie);
}
diff --git a/libziparchive/zip_archive_benchmark.cpp b/libziparchive/zip_archive_benchmark.cpp
index 52166a4..434f2e1 100644
--- a/libziparchive/zip_archive_benchmark.cpp
+++ b/libziparchive/zip_archive_benchmark.cpp
@@ -75,7 +75,7 @@
while (state.KeepRunning()) {
OpenArchive(temp_file->path, &handle);
- StartIteration(handle, &iteration_cookie, nullptr, nullptr);
+ StartIteration(handle, &iteration_cookie);
while (Next(iteration_cookie, &data, &name) == 0) {
}
EndIteration(iteration_cookie);
diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc
index cfbce1c..993c975 100644
--- a/libziparchive/zip_archive_test.cc
+++ b/libziparchive/zip_archive_test.cc
@@ -107,7 +107,7 @@
close(fd);
}
-static void AssertIterationOrder(const ZipString* prefix, const ZipString* suffix,
+static void AssertIterationOrder(const std::string_view prefix, const std::string_view suffix,
const std::vector<std::string>& expected_names_sorted) {
ZipArchiveHandle handle;
ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle));
@@ -137,30 +137,26 @@
static const std::vector<std::string> kExpectedMatchesSorted = {"a.txt", "b.txt", "b/", "b/c.txt",
"b/d.txt"};
- AssertIterationOrder(nullptr, nullptr, kExpectedMatchesSorted);
+ AssertIterationOrder("", "", kExpectedMatchesSorted);
}
TEST(ziparchive, IterationWithPrefix) {
- ZipString prefix("b/");
static const std::vector<std::string> kExpectedMatchesSorted = {"b/", "b/c.txt", "b/d.txt"};
- AssertIterationOrder(&prefix, nullptr, kExpectedMatchesSorted);
+ AssertIterationOrder("b/", "", kExpectedMatchesSorted);
}
TEST(ziparchive, IterationWithSuffix) {
- ZipString suffix(".txt");
static const std::vector<std::string> kExpectedMatchesSorted = {"a.txt", "b.txt", "b/c.txt",
"b/d.txt"};
- AssertIterationOrder(nullptr, &suffix, kExpectedMatchesSorted);
+ AssertIterationOrder("", ".txt", kExpectedMatchesSorted);
}
TEST(ziparchive, IterationWithPrefixAndSuffix) {
- ZipString prefix("b");
- ZipString suffix(".txt");
static const std::vector<std::string> kExpectedMatchesSorted = {"b.txt", "b/c.txt", "b/d.txt"};
- AssertIterationOrder(&prefix, &suffix, kExpectedMatchesSorted);
+ AssertIterationOrder("b", ".txt", kExpectedMatchesSorted);
}
TEST(ziparchive, IterationWithBadPrefixAndSuffix) {
@@ -168,9 +164,7 @@
ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle));
void* iteration_cookie;
- ZipString prefix("x");
- ZipString suffix("y");
- ASSERT_EQ(0, StartIteration(handle, &iteration_cookie, &prefix, &suffix));
+ ASSERT_EQ(0, StartIteration(handle, &iteration_cookie, "x", "y"));
ZipEntry data;
ZipString name;
@@ -228,7 +222,7 @@
ASSERT_EQ(0, OpenArchiveWrapper("declaredlength.zip", &handle));
void* iteration_cookie;
- ASSERT_EQ(0, StartIteration(handle, &iteration_cookie, nullptr, nullptr));
+ ASSERT_EQ(0, StartIteration(handle, &iteration_cookie));
ZipString name;
ZipEntry data;