diff --git a/adb/fastdeploy/deploypatchgenerator/apk_archive.cpp b/adb/fastdeploy/deploypatchgenerator/apk_archive.cpp index 3dc5e506e..932d579b5 100644 --- a/adb/fastdeploy/deploypatchgenerator/apk_archive.cpp +++ b/adb/fastdeploy/deploypatchgenerator/apk_archive.cpp @@ -36,7 +36,7 @@ struct FileRegion { FileRegion(borrowed_fd fd, off64_t offset, size_t length) : mapped_(android::base::MappedFile::FromOsHandle(adb_get_os_handle(fd), offset, length, PROT_READ)) { - if (mapped_.data() != nullptr) { + if (mapped_ != nullptr) { return; } @@ -50,14 +50,14 @@ struct FileRegion { } } - const char* data() const { return mapped_.data() ? mapped_.data() : buffer_.data(); } - size_t size() const { return mapped_.data() ? mapped_.size() : buffer_.size(); } + const char* data() const { return mapped_ ? mapped_->data() : buffer_.data(); } + size_t size() const { return mapped_ ? mapped_->size() : buffer_.size(); } private: FileRegion() = default; DISALLOW_COPY_AND_ASSIGN(FileRegion); - android::base::MappedFile mapped_; + std::unique_ptr mapped_; std::string buffer_; }; } // namespace diff --git a/base/include/android-base/mapped_file.h b/base/include/android-base/mapped_file.h index 6a19f1b73..8c37f4305 100644 --- a/base/include/android-base/mapped_file.h +++ b/base/include/android-base/mapped_file.h @@ -53,7 +53,8 @@ class MappedFile { /** * Same thing, but using the raw OS file handle instead of a CRT wrapper. */ - static MappedFile FromOsHandle(os_handle h, off64_t offset, size_t length, int prot); + static std::unique_ptr FromOsHandle(os_handle h, off64_t offset, size_t length, + int prot); /** * Removes the mapping. @@ -69,10 +70,6 @@ class MappedFile { char* data() const { return base_ + offset_; } size_t size() const { return size_; } - bool isValid() const { return base_ != nullptr; } - - explicit operator bool() const { return isValid(); } - private: DISALLOW_IMPLICIT_CONSTRUCTORS(MappedFile); diff --git a/base/mapped_file.cpp b/base/mapped_file.cpp index 862b73bc2..fff345384 100644 --- a/base/mapped_file.cpp +++ b/base/mapped_file.cpp @@ -38,15 +38,14 @@ static off64_t InitPageSize() { std::unique_ptr MappedFile::FromFd(borrowed_fd fd, off64_t offset, size_t length, int prot) { #if defined(_WIN32) - auto file = - FromOsHandle(reinterpret_cast(_get_osfhandle(fd.get())), offset, length, prot); + return FromOsHandle(reinterpret_cast(_get_osfhandle(fd.get())), offset, length, prot); #else - auto file = FromOsHandle(fd.get(), offset, length, prot); + return FromOsHandle(fd.get(), offset, length, prot); #endif - return file ? std::make_unique(std::move(file)) : std::unique_ptr{}; } -MappedFile MappedFile::FromOsHandle(os_handle h, off64_t offset, size_t length, int prot) { +std::unique_ptr MappedFile::FromOsHandle(os_handle h, off64_t offset, size_t length, + int prot) { static const off64_t page_size = InitPageSize(); size_t slop = offset % page_size; off64_t file_offset = offset - slop; @@ -59,28 +58,30 @@ MappedFile MappedFile::FromOsHandle(os_handle h, off64_t offset, size_t length, // http://b/119818070 "app crashes when reading asset of zero length". // Return a MappedFile that's only valid for reading the size. if (length == 0 && ::GetLastError() == ERROR_FILE_INVALID) { - return MappedFile{const_cast(kEmptyBuffer), 0, 0, nullptr}; + return std::unique_ptr( + new MappedFile(const_cast(kEmptyBuffer), 0, 0, nullptr)); } - return MappedFile(nullptr, 0, 0, nullptr); + return nullptr; } void* base = MapViewOfFile(handle, (prot & PROT_WRITE) ? FILE_MAP_ALL_ACCESS : FILE_MAP_READ, 0, file_offset, file_length); if (base == nullptr) { CloseHandle(handle); - return MappedFile(nullptr, 0, 0, nullptr); + return nullptr; } - return MappedFile{static_cast(base), length, slop, handle}; + return std::unique_ptr( + new MappedFile(static_cast(base), length, slop, handle)); #else void* base = mmap(nullptr, file_length, prot, MAP_SHARED, h, file_offset); if (base == MAP_FAILED) { // http://b/119818070 "app crashes when reading asset of zero length". // mmap fails with EINVAL for a zero length region. if (errno == EINVAL && length == 0) { - return MappedFile{const_cast(kEmptyBuffer), 0, 0}; + return std::unique_ptr(new MappedFile(const_cast(kEmptyBuffer), 0, 0)); } - return MappedFile(nullptr, 0, 0); + return nullptr; } - return MappedFile{static_cast(base), length, slop}; + return std::unique_ptr(new MappedFile(static_cast(base), length, slop)); #endif } diff --git a/base/mapped_file_test.cpp b/base/mapped_file_test.cpp index 3629108c9..d21703c78 100644 --- a/base/mapped_file_test.cpp +++ b/base/mapped_file_test.cpp @@ -44,8 +44,6 @@ TEST(mapped_file, zero_length_mapping) { ASSERT_TRUE(tf.fd != -1); auto m = android::base::MappedFile::FromFd(tf.fd, 4096, 0, PROT_READ); - ASSERT_NE(nullptr, m); - EXPECT_TRUE((bool)*m); EXPECT_EQ(0u, m->size()); EXPECT_NE(nullptr, m->data()); } diff --git a/libziparchive/testdata/empty.zip b/libziparchive/testdata/empty.zip new file mode 100644 index 000000000..15cb0ecb3 Binary files /dev/null and b/libziparchive/testdata/empty.zip differ diff --git a/libziparchive/testdata/zero-size-cd.zip b/libziparchive/testdata/zero-size-cd.zip new file mode 100644 index 000000000..b6c8cbeac Binary files /dev/null and b/libziparchive/testdata/zero-size-cd.zip differ diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc index ef29188fb..68837ccf6 100644 --- a/libziparchive/zip_archive.cc +++ b/libziparchive/zip_archive.cc @@ -265,14 +265,10 @@ static int32_t MapCentralDirectory0(const char* debug_file_name, ZipArchive* arc ALOGV("+++ num_entries=%" PRIu32 " dir_size=%" PRIu32 " dir_offset=%" PRIu32, eocd->num_records, eocd->cd_size, eocd->cd_start_offset); - /* - * It all looks good. Create a mapping for the CD, and set the fields - * in archive. - */ - + // It all looks good. Create a mapping for the CD, and set the fields + // in archive. if (!archive->InitializeCentralDirectory(static_cast(eocd->cd_start_offset), static_cast(eocd->cd_size))) { - ALOGE("Zip: failed to intialize central directory.\n"); return kMmapFailed; } @@ -354,7 +350,7 @@ static int32_t ParseZipArchive(ZipArchive* archive) { if (archive->hash_table == nullptr) { ALOGW("Zip: unable to allocate the %u-entry hash_table, entry size: %zu", archive->hash_table_size, sizeof(ZipStringOffset)); - return -1; + return kAllocationFailed; } /* @@ -365,24 +361,25 @@ static int32_t ParseZipArchive(ZipArchive* archive) { const uint8_t* ptr = cd_ptr; for (uint16_t i = 0; i < num_entries; i++) { if (ptr > cd_end - sizeof(CentralDirectoryRecord)) { - ALOGW("Zip: ran off the end (at %" PRIu16 ")", i); + ALOGW("Zip: ran off the end (item #%" PRIu16 ", %zu bytes of central directory)", i, + cd_length); #if defined(__ANDROID__) android_errorWriteLog(0x534e4554, "36392138"); #endif - return -1; + return kInvalidFile; } const CentralDirectoryRecord* cdr = reinterpret_cast(ptr); if (cdr->record_signature != CentralDirectoryRecord::kSignature) { ALOGW("Zip: missed a central dir sig (at %" PRIu16 ")", i); - return -1; + return kInvalidFile; } const off64_t local_header_offset = cdr->local_file_header_offset; if (local_header_offset >= archive->directory_offset) { ALOGW("Zip: bad LFH offset %" PRId64 " at entry %" PRIu16, static_cast(local_header_offset), i); - return -1; + return kInvalidFile; } const uint16_t file_name_length = cdr->file_name_length; @@ -394,12 +391,12 @@ static int32_t ParseZipArchive(ZipArchive* archive) { ALOGW("Zip: file name for entry %" PRIu16 " exceeds the central directory range, file_name_length: %" PRIu16 ", cd_length: %zu", i, file_name_length, cd_length); - return -1; + return kInvalidEntryName; } // Check that file name is valid UTF-8 and doesn't contain NUL (U+0000) characters. if (!IsValidEntryName(file_name, file_name_length)) { ALOGW("Zip: invalid file name at entry %" PRIu16, i); - return -1; + return kInvalidEntryName; } // Add the CDE filename to the hash table. @@ -414,7 +411,7 @@ static int32_t ParseZipArchive(ZipArchive* archive) { ptr += sizeof(CentralDirectoryRecord) + file_name_length + extra_length + comment_length; if ((ptr - cd_ptr) > static_cast(cd_length)) { ALOGW("Zip: bad CD advance (%tu vs %zu) at entry %" PRIu16, ptr - cd_ptr, cd_length, i); - return -1; + return kInvalidFile; } } @@ -422,7 +419,7 @@ static int32_t ParseZipArchive(ZipArchive* archive) { if (!archive->mapped_zip.ReadAtOffset(reinterpret_cast(&lfh_start_bytes), sizeof(uint32_t), 0)) { ALOGW("Zip: Unable to read header for entry at offset == 0."); - return -1; + return kInvalidFile; } if (lfh_start_bytes != LocalFileHeader::kSignature) { @@ -430,7 +427,7 @@ static int32_t ParseZipArchive(ZipArchive* archive) { #if defined(__ANDROID__) android_errorWriteLog(0x534e4554, "64211847"); #endif - return -1; + return kInvalidFile; } ALOGV("+++ zip good scan %" PRIu16 " entries", num_entries); @@ -439,16 +436,8 @@ static int32_t ParseZipArchive(ZipArchive* archive) { } static int32_t OpenArchiveInternal(ZipArchive* archive, const char* debug_file_name) { - int32_t result = -1; - if ((result = MapCentralDirectory(debug_file_name, archive)) != 0) { - return result; - } - - if ((result = ParseZipArchive(archive))) { - return result; - } - - return 0; + int32_t result = MapCentralDirectory(debug_file_name, archive); + return result != 0 ? result : ParseZipArchive(archive); } int32_t OpenArchiveFd(int fd, const char* debug_file_name, ZipArchiveHandle* handle, @@ -1185,7 +1174,7 @@ off64_t MappedZipFile::GetFileLength() const { return result; } else { if (base_ptr_ == nullptr) { - ALOGE("Zip: invalid file map\n"); + ALOGE("Zip: invalid file map"); return -1; } return static_cast(data_length_); @@ -1196,12 +1185,12 @@ off64_t MappedZipFile::GetFileLength() const { bool MappedZipFile::ReadAtOffset(uint8_t* buf, size_t len, off64_t off) const { if (has_fd_) { if (!android::base::ReadFullyAtOffset(fd_, buf, len, off)) { - ALOGE("Zip: failed to read at offset %" PRId64 "\n", off); + ALOGE("Zip: failed to read at offset %" PRId64, off); return false; } } else { if (off < 0 || off > static_cast(data_length_)) { - ALOGE("Zip: invalid offset: %" PRId64 ", data length: %" PRId64 "\n", off, data_length_); + ALOGE("Zip: invalid offset: %" PRId64 ", data length: %" PRId64, off, data_length_); return false; } memcpy(buf, static_cast(base_ptr_) + off, len); @@ -1219,13 +1208,17 @@ bool ZipArchive::InitializeCentralDirectory(off64_t cd_start_offset, size_t cd_s if (mapped_zip.HasFd()) { directory_map = android::base::MappedFile::FromFd(mapped_zip.GetFileDescriptor(), cd_start_offset, cd_size, PROT_READ); - if (!directory_map) return false; + if (!directory_map) { + ALOGE("Zip: failed to map central directory (offset %" PRId64 ", size %zu): %s", + cd_start_offset, cd_size, strerror(errno)); + return false; + } CHECK_EQ(directory_map->size(), cd_size); central_directory.Initialize(directory_map->data(), 0 /*offset*/, cd_size); } else { if (mapped_zip.GetBasePtr() == nullptr) { - ALOGE("Zip: Failed to map central directory, bad mapped_zip base pointer\n"); + ALOGE("Zip: Failed to map central directory, bad mapped_zip base pointer"); return false; } if (static_cast(cd_start_offset) + static_cast(cd_size) > diff --git a/libziparchive/zip_archive_private.h b/libziparchive/zip_archive_private.h index 60fdec0bb..1d05fc718 100644 --- a/libziparchive/zip_archive_private.h +++ b/libziparchive/zip_archive_private.h @@ -42,6 +42,7 @@ static const char* kErrorMessages[] = { "Invalid entry name", "I/O error", "File mapping failed", + "Allocation failed", }; enum ErrorCodes : int32_t { @@ -87,7 +88,10 @@ enum ErrorCodes : int32_t { // We were not able to mmap the central directory or entry contents. kMmapFailed = -12, - kLastErrorCode = kMmapFailed, + // An allocation failed. + kAllocationFailed = -13, + + kLastErrorCode = kAllocationFailed, }; class MappedZipFile { diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc index 8781ab79e..091630407 100644 --- a/libziparchive/zip_archive_test.cc +++ b/libziparchive/zip_archive_test.cc @@ -36,13 +36,9 @@ static std::string test_data_dir = android::base::GetExecutableDirectory() + "/testdata"; -static const std::string kMissingZip = "missing.zip"; static const std::string kValidZip = "valid.zip"; static const std::string kLargeZip = "large.zip"; static const std::string kBadCrcZip = "bad_crc.zip"; -static const std::string kCrashApk = "crash.apk"; -static const std::string kBadFilenameZip = "bad_filename.zip"; -static const std::string kUpdateZip = "dummy-update.zip"; static const std::vector kATxtContents{'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', '\n'}; @@ -52,13 +48,6 @@ static const std::vector kATxtContentsCompressed{'K', 'L', 'J', 'N', 'I static const std::vector kBTxtContents{'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', '\n'}; -static const std::string kATxtName("a.txt"); -static const std::string kBTxtName("b.txt"); -static const std::string kNonexistentTxtName("nonexistent.txt"); -static const std::string kEmptyTxtName("empty.txt"); -static const std::string kLargeCompressTxtName("compress.txt"); -static const std::string kLargeUncompressTxtName("uncompress.txt"); - static int32_t OpenArchiveWrapper(const std::string& name, ZipArchiveHandle* handle) { const std::string abs_path = test_data_dir + "/" + name; return OpenArchive(abs_path.c_str(), handle); @@ -69,19 +58,31 @@ TEST(ziparchive, Open) { ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); CloseArchive(handle); - ASSERT_EQ(-1, OpenArchiveWrapper(kBadFilenameZip, &handle)); + ASSERT_EQ(kInvalidEntryName, OpenArchiveWrapper("bad_filename.zip", &handle)); CloseArchive(handle); } TEST(ziparchive, OutOfBound) { ZipArchiveHandle handle; - ASSERT_EQ(-8, OpenArchiveWrapper(kCrashApk, &handle)); + ASSERT_EQ(kInvalidOffset, OpenArchiveWrapper("crash.apk", &handle)); + CloseArchive(handle); +} + +TEST(ziparchive, EmptyArchive) { + ZipArchiveHandle handle; + ASSERT_EQ(kEmptyArchive, OpenArchiveWrapper("empty.zip", &handle)); + CloseArchive(handle); +} + +TEST(ziparchive, ZeroSizeCentralDirectory) { + ZipArchiveHandle handle; + ASSERT_EQ(kInvalidFile, OpenArchiveWrapper("zero-size-cd.zip", &handle)); CloseArchive(handle); } TEST(ziparchive, OpenMissing) { ZipArchiveHandle handle; - ASSERT_NE(0, OpenArchiveWrapper(kMissingZip, &handle)); + ASSERT_NE(0, OpenArchiveWrapper("missing.zip", &handle)); // Confirm the file descriptor is not going to be mistaken for a valid one. ASSERT_EQ(-1, GetFileDescriptor(handle)); @@ -200,7 +201,7 @@ TEST(ziparchive, FindEntry) { ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, kATxtName, &data)); + ASSERT_EQ(0, FindEntry(handle, "a.txt", &data)); // Known facts about a.txt, from zipinfo -v. ASSERT_EQ(63, data.offset); @@ -211,7 +212,7 @@ TEST(ziparchive, FindEntry) { ASSERT_EQ(static_cast(0x438a8005), data.mod_time); // An entry that doesn't exist. Should be a negative return code. - ASSERT_LT(FindEntry(handle, kNonexistentTxtName, &data), 0); + ASSERT_LT(FindEntry(handle, "this file does not exist", &data), 0); CloseArchive(handle); } @@ -259,7 +260,7 @@ TEST(ziparchive, ExtractToMemory) { // An entry that's deflated. ZipEntry data; - ASSERT_EQ(0, FindEntry(handle, kATxtName, &data)); + ASSERT_EQ(0, FindEntry(handle, "a.txt", &data)); const uint32_t a_size = data.uncompressed_length; ASSERT_EQ(a_size, kATxtContents.size()); uint8_t* buffer = new uint8_t[a_size]; @@ -268,7 +269,7 @@ TEST(ziparchive, ExtractToMemory) { delete[] buffer; // An entry that's stored. - ASSERT_EQ(0, FindEntry(handle, kBTxtName, &data)); + ASSERT_EQ(0, FindEntry(handle, "b.txt", &data)); const uint32_t b_size = data.uncompressed_length; ASSERT_EQ(b_size, kBTxtContents.size()); buffer = new uint8_t[b_size]; @@ -323,7 +324,7 @@ TEST(ziparchive, EmptyEntries) { ASSERT_EQ(0, OpenArchiveFd(tmp_file.fd, "EmptyEntriesTest", &handle, false)); ZipEntry entry; - ASSERT_EQ(0, FindEntry(handle, kEmptyTxtName, &entry)); + ASSERT_EQ(0, FindEntry(handle, "empty.txt", &entry)); ASSERT_EQ(static_cast(0), entry.uncompressed_length); uint8_t buffer[1]; ASSERT_EQ(0, ExtractToMemory(handle, &entry, buffer, 1)); @@ -403,7 +404,7 @@ TEST(ziparchive, ExtractToFile) { ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); ZipEntry entry; - ASSERT_EQ(0, FindEntry(handle, kATxtName, &entry)); + ASSERT_EQ(0, FindEntry(handle, "a.txt", &entry)); ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, tmp_file.fd)); // Assert that the first 8 bytes of the file haven't been clobbered. @@ -425,7 +426,7 @@ TEST(ziparchive, ExtractToFile) { #if !defined(_WIN32) TEST(ziparchive, OpenFromMemory) { - const std::string zip_path = test_data_dir + "/" + kUpdateZip; + const std::string zip_path = test_data_dir + "/dummy-update.zip"; android::base::unique_fd fd(open(zip_path.c_str(), O_RDONLY | O_BINARY)); ASSERT_NE(-1, fd); struct stat sb; @@ -510,27 +511,27 @@ static void ZipArchiveStreamTestUsingMemory(const std::string& zip_file, } TEST(ziparchive, StreamCompressed) { - ZipArchiveStreamTestUsingContents(kValidZip, kATxtName, kATxtContents, false); + ZipArchiveStreamTestUsingContents(kValidZip, "a.txt", kATxtContents, false); } TEST(ziparchive, StreamUncompressed) { - ZipArchiveStreamTestUsingContents(kValidZip, kBTxtName, kBTxtContents, false); + ZipArchiveStreamTestUsingContents(kValidZip, "b.txt", kBTxtContents, false); } TEST(ziparchive, StreamRawCompressed) { - ZipArchiveStreamTestUsingContents(kValidZip, kATxtName, kATxtContentsCompressed, true); + ZipArchiveStreamTestUsingContents(kValidZip, "a.txt", kATxtContentsCompressed, true); } TEST(ziparchive, StreamRawUncompressed) { - ZipArchiveStreamTestUsingContents(kValidZip, kBTxtName, kBTxtContents, true); + ZipArchiveStreamTestUsingContents(kValidZip, "b.txt", kBTxtContents, true); } TEST(ziparchive, StreamLargeCompressed) { - ZipArchiveStreamTestUsingMemory(kLargeZip, kLargeCompressTxtName); + ZipArchiveStreamTestUsingMemory(kLargeZip, "compress.txt"); } TEST(ziparchive, StreamLargeUncompressed) { - ZipArchiveStreamTestUsingMemory(kLargeZip, kLargeUncompressTxtName); + ZipArchiveStreamTestUsingMemory(kLargeZip, "uncompress.txt"); } TEST(ziparchive, StreamCompressedBadCrc) { @@ -539,7 +540,7 @@ TEST(ziparchive, StreamCompressedBadCrc) { ZipEntry entry; std::vector read_data; - ZipArchiveStreamTest(handle, kATxtName, false, false, &entry, &read_data); + ZipArchiveStreamTest(handle, "a.txt", false, false, &entry, &read_data); CloseArchive(handle); } @@ -550,7 +551,7 @@ TEST(ziparchive, StreamUncompressedBadCrc) { ZipEntry entry; std::vector read_data; - ZipArchiveStreamTest(handle, kBTxtName, false, false, &entry, &read_data); + ZipArchiveStreamTest(handle, "b.txt", false, false, &entry, &read_data); CloseArchive(handle); } @@ -647,7 +648,8 @@ TEST(ziparchive, ErrorCodeString) { // Out of bounds. ASSERT_STREQ("Unknown return code", ErrorCodeString(1)); - ASSERT_STREQ("Unknown return code", ErrorCodeString(-13)); + ASSERT_STRNE("Unknown return code", ErrorCodeString(kLastErrorCode)); + ASSERT_STREQ("Unknown return code", ErrorCodeString(kLastErrorCode - 1)); ASSERT_STREQ("I/O error", ErrorCodeString(kIoError)); } @@ -698,7 +700,7 @@ TEST(ziparchive, BrokenLfhSignature) { ASSERT_TRUE(android::base::WriteFully(tmp_file.fd, &kZipFileWithBrokenLfhSignature[0], kZipFileWithBrokenLfhSignature.size())); ZipArchiveHandle handle; - ASSERT_EQ(-1, OpenArchiveFd(tmp_file.fd, "LeadingNonZipBytes", &handle, false)); + ASSERT_EQ(kInvalidFile, OpenArchiveFd(tmp_file.fd, "LeadingNonZipBytes", &handle, false)); } class VectorReader : public zip_archive::Reader {