From 8e6f7227641e05dccfc29dad62e29e730ff8f5ca Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Mon, 8 Feb 2016 16:26:33 -0800 Subject: [PATCH] libziparchive: port unit tests to darwin and windows. Also use ReadFully to replace read, because read can return reading bytes less than requested. And use WriteFully to replace write. Bug: 26962895 Change-Id: Iff0b2bc6d925619a537f7fef682c2a7ad89a2dc2 --- base/file.cpp | 4 -- base/include/android-base/file.h | 4 ++ libziparchive/Android.mk | 11 ++- libziparchive/zip_archive.cc | 51 ++++---------- libziparchive/zip_archive_test.cc | 109 +++++++++++------------------- libziparchive/zip_writer_test.cc | 20 ++++-- 6 files changed, 77 insertions(+), 122 deletions(-) diff --git a/base/file.cpp b/base/file.cpp index bcdfc5e99..da1adba19 100644 --- a/base/file.cpp +++ b/base/file.cpp @@ -29,10 +29,6 @@ #include "cutils/log.h" #include "utils/Compat.h" -#if !defined(_WIN32) -#define O_BINARY 0 -#endif - namespace android { namespace base { diff --git a/base/include/android-base/file.h b/base/include/android-base/file.h index 486befcbb..5342d9878 100644 --- a/base/include/android-base/file.h +++ b/base/include/android-base/file.h @@ -20,6 +20,10 @@ #include #include +#if !defined(_WIN32) && !defined(O_BINARY) +#define O_BINARY 0 +#endif + namespace android { namespace base { diff --git a/libziparchive/Android.mk b/libziparchive/Android.mk index 056b3e132..3cd8b87b1 100644 --- a/libziparchive/Android.mk +++ b/libziparchive/Android.mk @@ -95,13 +95,12 @@ LOCAL_CPP_EXTENSION := .cc LOCAL_CFLAGS := $(libziparchive_common_c_flags) LOCAL_CPPFLAGS := -Wno-unnamed-type-template-args $(libziparchive_common_cpp_flags) LOCAL_SRC_FILES := $(libziparchive_test_files) -LOCAL_SHARED_LIBRARIES := \ - libziparchive-host \ - liblog \ - libbase \ - LOCAL_STATIC_LIBRARIES := \ - libutils \ + libziparchive-host \ libz \ + libbase \ + libutils \ + liblog \ +LOCAL_MODULE_HOST_OS := darwin linux windows include $(BUILD_HOST_NATIVE_TEST) diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc index 3b1e972ce..a2d6fccd1 100644 --- a/libziparchive/zip_archive.cc +++ b/libziparchive/zip_archive.cc @@ -224,9 +224,7 @@ static int32_t MapCentralDirectory0(int fd, const char* debug_file_name, strerror(errno)); return kIoError; } - ssize_t actual = TEMP_FAILURE_RETRY( - read(fd, scan_buffer, static_cast(read_amount))); - if (actual != static_cast(read_amount)) { + if (!android::base::ReadFully(fd, scan_buffer, static_cast(read_amount))) { ALOGW("Zip: read %" PRId64 " failed: %s", static_cast(read_amount), strerror(errno)); return kIoError; @@ -481,8 +479,7 @@ void CloseArchive(ZipArchiveHandle handle) { static int32_t UpdateEntryFromDataDescriptor(int fd, ZipEntry *entry) { uint8_t ddBuf[sizeof(DataDescriptor) + sizeof(DataDescriptor::kOptSignature)]; - ssize_t actual = TEMP_FAILURE_RETRY(read(fd, ddBuf, sizeof(ddBuf))); - if (actual != sizeof(ddBuf)) { + if (!android::base::ReadFully(fd, ddBuf, sizeof(ddBuf))) { return kIoError; } @@ -498,26 +495,14 @@ static int32_t UpdateEntryFromDataDescriptor(int fd, } // Attempts to read |len| bytes into |buf| at offset |off|. -// -// This method uses pread64 on platforms that support it and -// lseek64 + read on platforms that don't. This implies that -// callers should not rely on the |fd| offset being incremented +// Callers should not rely on the |fd| offset being incremented // as a side effect of this call. -static inline ssize_t ReadAtOffset(int fd, uint8_t* buf, size_t len, - off64_t off) { -#if !defined(_WIN32) - return TEMP_FAILURE_RETRY(pread64(fd, buf, len, off)); -#else - // The only supported platform that doesn't support pread at the moment - // is Windows. Only recent versions of windows support unix like forks, - // and even there the semantics are quite different. +static inline bool ReadAtOffset(int fd, uint8_t* buf, size_t len, off64_t off) { if (lseek64(fd, off, SEEK_SET) != off) { ALOGW("Zip: failed seek to offset %" PRId64, off); - return kIoError; + return false; } - - return TEMP_FAILURE_RETRY(read(fd, buf, len)); -#endif + return android::base::ReadFully(fd, buf, len); } static int32_t FindEntry(const ZipArchive* archive, const int ent, @@ -567,9 +552,7 @@ static int32_t FindEntry(const ZipArchive* archive, const int ent, } uint8_t lfh_buf[sizeof(LocalFileHeader)]; - ssize_t actual = ReadAtOffset(archive->fd, lfh_buf, sizeof(lfh_buf), - local_header_offset); - if (actual != sizeof(lfh_buf)) { + if (!ReadAtOffset(archive->fd, lfh_buf, sizeof(lfh_buf), local_header_offset)) { ALOGW("Zip: failed reading lfh name from offset %" PRId64, static_cast(local_header_offset)); return kIoError; @@ -610,10 +593,7 @@ static int32_t FindEntry(const ZipArchive* archive, const int ent, } uint8_t* name_buf = reinterpret_cast(malloc(nameLen)); - ssize_t actual = ReadAtOffset(archive->fd, name_buf, nameLen, - name_offset); - - if (actual != nameLen) { + if (!ReadAtOffset(archive->fd, name_buf, nameLen, name_offset)) { ALOGW("Zip: failed reading lfh name from offset %" PRId64, static_cast(name_offset)); free(name_buf); return kIoError; @@ -942,10 +922,9 @@ static int32_t InflateEntryToWriter(int fd, const ZipEntry* entry, do { /* read as much as we can */ if (zstream.avail_in == 0) { - const ZD_TYPE getSize = (compressed_length > kBufSize) ? kBufSize : compressed_length; - const ZD_TYPE actual = TEMP_FAILURE_RETRY(read(fd, &read_buf[0], getSize)); - if (actual != getSize) { - ALOGW("Zip: inflate read failed (" ZD " vs " ZD ")", actual, getSize); + const size_t getSize = (compressed_length > kBufSize) ? kBufSize : compressed_length; + if (!android::base::ReadFully(fd, read_buf.data(), getSize)) { + ALOGW("Zip: inflate read failed, getSize = %zu: %s", getSize, strerror(errno)); return kIoError; } @@ -1005,11 +984,9 @@ static int32_t CopyEntryToWriter(int fd, const ZipEntry* entry, Writer* writer, // Safe conversion because kBufSize is narrow enough for a 32 bit signed // value. - const ssize_t block_size = (remaining > kBufSize) ? kBufSize : remaining; - const ssize_t actual = TEMP_FAILURE_RETRY(read(fd, &buf[0], block_size)); - - if (actual != block_size) { - ALOGW("CopyFileToFile: copy read failed (" ZD " vs " ZD ")", actual, block_size); + const size_t block_size = (remaining > kBufSize) ? kBufSize : remaining; + if (!android::base::ReadFully(fd, buf.data(), block_size)) { + ALOGW("CopyFileToFile: copy read failed, block_size = %zu: %s", block_size, strerror(errno)); return kIoError; } diff --git a/libziparchive/zip_archive_test.cc b/libziparchive/zip_archive_test.cc index d426dc4f4..6aee1bbdf 100644 --- a/libziparchive/zip_archive_test.cc +++ b/libziparchive/zip_archive_test.cc @@ -21,9 +21,11 @@ #include #include +#include #include #include +#include #include #include #include @@ -91,7 +93,7 @@ TEST(ziparchive, OpenMissing) { } TEST(ziparchive, OpenAssumeFdOwnership) { - int fd = open((test_data_dir + "/" + kValidZip).c_str(), O_RDONLY); + int fd = open((test_data_dir + "/" + kValidZip).c_str(), O_RDONLY | O_BINARY); ASSERT_NE(-1, fd); ZipArchiveHandle handle; ASSERT_EQ(0, OpenArchiveFd(fd, "OpenWithAssumeFdOwnership", &handle)); @@ -101,7 +103,7 @@ TEST(ziparchive, OpenAssumeFdOwnership) { } TEST(ziparchive, OpenDoNotAssumeFdOwnership) { - int fd = open((test_data_dir + "/" + kValidZip).c_str(), O_RDONLY); + int fd = open((test_data_dir + "/" + kValidZip).c_str(), O_RDONLY | O_BINARY); ASSERT_NE(-1, fd); ZipArchiveHandle handle; ASSERT_EQ(0, OpenArchiveFd(fd, "OpenWithAssumeFdOwnership", &handle, false)); @@ -373,30 +375,13 @@ static const uint16_t kAbZip[] = { static const std::string kAbTxtName("ab.txt"); static const size_t kAbUncompressedSize = 270216; -static int make_temporary_file(const char* file_name_pattern) { - char full_path[1024]; - // Account for differences between the host and the target. - // - // TODO: Maybe reuse bionic/tests/TemporaryFile.h. - snprintf(full_path, sizeof(full_path), "/data/local/tmp/%s", file_name_pattern); - int fd = mkstemp(full_path); - if (fd == -1) { - snprintf(full_path, sizeof(full_path), "/tmp/%s", file_name_pattern); - fd = mkstemp(full_path); - } - - return fd; -} - TEST(ziparchive, EmptyEntries) { - char temp_file_pattern[] = "empty_entries_test_XXXXXX"; - int fd = make_temporary_file(temp_file_pattern); - ASSERT_NE(-1, fd); - const ssize_t file_size = sizeof(kEmptyEntriesZip); - ASSERT_EQ(file_size, TEMP_FAILURE_RETRY(write(fd, kEmptyEntriesZip, file_size))); + TemporaryFile tmp_file; + ASSERT_NE(-1, tmp_file.fd); + ASSERT_TRUE(android::base::WriteFully(tmp_file.fd, kEmptyEntriesZip, sizeof(kEmptyEntriesZip))); ZipArchiveHandle handle; - ASSERT_EQ(0, OpenArchiveFd(fd, "EmptyEntriesTest", &handle)); + ASSERT_EQ(0, OpenArchiveFd(tmp_file.fd, "EmptyEntriesTest", &handle)); ZipEntry entry; ZipString empty_name; @@ -406,27 +391,23 @@ TEST(ziparchive, EmptyEntries) { uint8_t buffer[1]; ASSERT_EQ(0, ExtractToMemory(handle, &entry, buffer, 1)); - char output_file_pattern[] = "empty_entries_output_XXXXXX"; - int output_fd = make_temporary_file(output_file_pattern); - ASSERT_NE(-1, output_fd); - ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, output_fd)); + + TemporaryFile tmp_output_file; + ASSERT_NE(-1, tmp_output_file.fd); + ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, tmp_output_file.fd)); struct stat stat_buf; - ASSERT_EQ(0, fstat(output_fd, &stat_buf)); + ASSERT_EQ(0, fstat(tmp_output_file.fd, &stat_buf)); ASSERT_EQ(0, stat_buf.st_size); - - close(fd); - close(output_fd); } TEST(ziparchive, EntryLargerThan32K) { - char temp_file_pattern[] = "entry_larger_than_32k_test_XXXXXX"; - int fd = make_temporary_file(temp_file_pattern); - ASSERT_NE(-1, fd); - ASSERT_TRUE(android::base::WriteFully(fd, reinterpret_cast(kAbZip), + TemporaryFile tmp_file; + ASSERT_NE(-1, tmp_file.fd); + ASSERT_TRUE(android::base::WriteFully(tmp_file.fd, reinterpret_cast(kAbZip), sizeof(kAbZip) - 1)); ZipArchiveHandle handle; - ASSERT_EQ(0, OpenArchiveFd(fd, "EntryLargerThan32KTest", &handle)); + ASSERT_EQ(0, OpenArchiveFd(tmp_file.fd, "EntryLargerThan32KTest", &handle)); ZipEntry entry; ZipString ab_name; @@ -439,21 +420,21 @@ TEST(ziparchive, EntryLargerThan32K) { ASSERT_EQ(0, ExtractToMemory(handle, &entry, &buffer[0], buffer.size())); // Extract the entry to a file. - char output_file_pattern[] = "entry_larger_than_32k_test_output_XXXXXX"; - int output_fd = make_temporary_file(output_file_pattern); - ASSERT_NE(-1, output_fd); - ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, output_fd)); + TemporaryFile tmp_output_file; + ASSERT_NE(-1, tmp_output_file.fd); + ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, tmp_output_file.fd)); // Make sure the extracted file size is as expected. struct stat stat_buf; - ASSERT_EQ(0, fstat(output_fd, &stat_buf)); + ASSERT_EQ(0, fstat(tmp_output_file.fd, &stat_buf)); ASSERT_EQ(kAbUncompressedSize, static_cast(stat_buf.st_size)); // Read the file back to a buffer and make sure the contents are // the same as the memory buffer we extracted directly to. std::vector file_contents(kAbUncompressedSize); - ASSERT_EQ(0, lseek64(output_fd, 0, SEEK_SET)); - ASSERT_TRUE(android::base::ReadFully(output_fd, &file_contents[0], file_contents.size())); + ASSERT_EQ(0, lseek64(tmp_output_file.fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFully(tmp_output_file.fd, &file_contents[0], + file_contents.size())); ASSERT_EQ(file_contents, buffer); for (int i = 0; i < 90072; ++i) { @@ -462,35 +443,28 @@ TEST(ziparchive, EntryLargerThan32K) { ASSERT_EQ('b', line[1]); ASSERT_EQ('\n', line[2]); } - - close(fd); - close(output_fd); } TEST(ziparchive, TrailerAfterEOCD) { - char temp_file_pattern[] = "trailer_after_eocd_test_XXXXXX"; - int fd = make_temporary_file(temp_file_pattern); - ASSERT_NE(-1, fd); + TemporaryFile tmp_file; + ASSERT_NE(-1, tmp_file.fd); // Create a file with 8 bytes of random garbage. static const uint8_t trailer[] = { 'A' ,'n', 'd', 'r', 'o', 'i', 'd', 'z' }; - const ssize_t file_size = sizeof(kEmptyEntriesZip); - const ssize_t trailer_size = sizeof(trailer); - ASSERT_EQ(file_size, TEMP_FAILURE_RETRY(write(fd, kEmptyEntriesZip, file_size))); - ASSERT_EQ(trailer_size, TEMP_FAILURE_RETRY(write(fd, trailer, trailer_size))); + ASSERT_TRUE(android::base::WriteFully(tmp_file.fd, kEmptyEntriesZip, sizeof(kEmptyEntriesZip))); + ASSERT_TRUE(android::base::WriteFully(tmp_file.fd, trailer, sizeof(trailer))); ZipArchiveHandle handle; - ASSERT_GT(0, OpenArchiveFd(fd, "EmptyEntriesTest", &handle)); + ASSERT_GT(0, OpenArchiveFd(tmp_file.fd, "EmptyEntriesTest", &handle)); } TEST(ziparchive, ExtractToFile) { - char kTempFilePattern[] = "zip_archive_input_XXXXXX"; - int fd = make_temporary_file(kTempFilePattern); - ASSERT_NE(-1, fd); + TemporaryFile tmp_file; + ASSERT_NE(-1, tmp_file.fd); const uint8_t data[8] = { '1', '2', '3', '4', '5', '6', '7', '8' }; - const ssize_t data_size = sizeof(data); + const size_t data_size = sizeof(data); - ASSERT_EQ(data_size, TEMP_FAILURE_RETRY(write(fd, data, data_size))); + ASSERT_TRUE(android::base::WriteFully(tmp_file.fd, data, data_size)); ZipArchiveHandle handle; ASSERT_EQ(0, OpenArchiveWrapper(kValidZip, &handle)); @@ -499,28 +473,25 @@ TEST(ziparchive, ExtractToFile) { ZipString name; SetZipString(&name, kATxtName); ASSERT_EQ(0, FindEntry(handle, name, &entry)); - ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, fd)); + ASSERT_EQ(0, ExtractEntryToFile(handle, &entry, tmp_file.fd)); // Assert that the first 8 bytes of the file haven't been clobbered. uint8_t read_buffer[data_size]; - ASSERT_EQ(0, lseek64(fd, 0, SEEK_SET)); - ASSERT_EQ(data_size, TEMP_FAILURE_RETRY(read(fd, read_buffer, data_size))); + ASSERT_EQ(0, lseek64(tmp_file.fd, 0, SEEK_SET)); + ASSERT_TRUE(android::base::ReadFully(tmp_file.fd, read_buffer, data_size)); ASSERT_EQ(0, memcmp(read_buffer, data, data_size)); // Assert that the remainder of the file contains the incompressed data. std::vector uncompressed_data(entry.uncompressed_length); - ASSERT_EQ(static_cast(entry.uncompressed_length), - TEMP_FAILURE_RETRY( - read(fd, &uncompressed_data[0], entry.uncompressed_length))); + ASSERT_TRUE(android::base::ReadFully(tmp_file.fd, uncompressed_data.data(), + entry.uncompressed_length)); ASSERT_EQ(0, memcmp(&uncompressed_data[0], kATxtContents.data(), kATxtContents.size())); // Assert that the total length of the file is sane - ASSERT_EQ(data_size + static_cast(kATxtContents.size()), - lseek64(fd, 0, SEEK_END)); - - close(fd); + ASSERT_EQ(static_cast(data_size + kATxtContents.size()), + lseek64(tmp_file.fd, 0, SEEK_END)); } static void ZipArchiveStreamTest( diff --git a/libziparchive/zip_writer_test.cc b/libziparchive/zip_writer_test.cc index fe0846db6..16a574d99 100644 --- a/libziparchive/zip_writer_test.cc +++ b/libziparchive/zip_writer_test.cc @@ -154,12 +154,22 @@ void ConvertZipTimeToTm(uint32_t& zip_time, struct tm* tm) { tm->tm_mday = (zip_time >> 16) & 0x1f; } +static struct tm MakeTm() { + struct tm tm; + memset(&tm, 0, sizeof(struct tm)); + tm.tm_year = 2001 - 1900; + tm.tm_mon = 1; + tm.tm_mday = 12; + tm.tm_hour = 18; + tm.tm_min = 30; + tm.tm_sec = 20; + return tm; +} + TEST_F(zipwriter, WriteUncompressedZipFileWithAlignedFlagAndTime) { ZipWriter writer(file_); - struct tm tm; - memset(&tm, 0, sizeof(struct tm)); - ASSERT_TRUE(strptime("18:30:20 1/12/2001", "%H:%M:%S %d/%m/%Y", &tm) != nullptr); + struct tm tm = MakeTm(); time_t time = mktime(&tm); ASSERT_EQ(0, writer.StartEntryWithTime("align.txt", ZipWriter::kAlign32, time)); ASSERT_EQ(0, writer.WriteBytes("he", 2)); @@ -210,9 +220,7 @@ TEST_F(zipwriter, WriteUncompressedZipFileWithAlignedValue) { TEST_F(zipwriter, WriteUncompressedZipFileWithAlignedValueAndTime) { ZipWriter writer(file_); - struct tm tm; - memset(&tm, 0, sizeof(struct tm)); - ASSERT_TRUE(strptime("18:30:20 1/12/2001", "%H:%M:%S %d/%m/%Y", &tm) != nullptr); + struct tm tm = MakeTm(); time_t time = mktime(&tm); ASSERT_EQ(0, writer.StartAlignedEntryWithTime("align.txt", 0, time, 4096)); ASSERT_EQ(0, writer.WriteBytes("he", 2));