Merge "libziparchive: Use ReadAtOffset exclusively"
diff --git a/base/file.cpp b/base/file.cpp
index a2f2887..2f697a1 100644
--- a/base/file.cpp
+++ b/base/file.cpp
@@ -153,6 +153,37 @@
return true;
}
+#if defined(_WIN32)
+// Windows implementation of pread. Note that this DOES move the file descriptors read position,
+// but it does so atomically.
+static ssize_t pread(int fd, void* data, size_t byte_count, off64_t offset) {
+ DWORD bytes_read;
+ OVERLAPPED overlapped;
+ memset(&overlapped, 0, sizeof(OVERLAPPED));
+ overlapped.Offset = static_cast<DWORD>(offset);
+ overlapped.OffsetHigh = static_cast<DWORD>(offset >> 32);
+ if (!ReadFile(reinterpret_cast<HANDLE>(_get_osfhandle(fd)), data, static_cast<DWORD>(byte_count),
+ &bytes_read, &overlapped)) {
+ // In case someone tries to read errno (since this is masquerading as a POSIX call)
+ errno = EIO;
+ return -1;
+ }
+ return static_cast<ssize_t>(bytes_read);
+}
+#endif
+
+bool ReadFullyAtOffset(int fd, void* data, size_t byte_count, off64_t offset) {
+ uint8_t* p = reinterpret_cast<uint8_t*>(data);
+ while (byte_count > 0) {
+ ssize_t n = TEMP_FAILURE_RETRY(pread(fd, p, byte_count, offset));
+ if (n <= 0) return false;
+ p += n;
+ byte_count -= n;
+ offset += n;
+ }
+ return true;
+}
+
bool WriteFully(int fd, const void* data, size_t byte_count) {
const uint8_t* p = reinterpret_cast<const uint8_t*>(data);
size_t remaining = byte_count;
diff --git a/base/include/android-base/file.h b/base/include/android-base/file.h
index 651f529..af14892 100644
--- a/base/include/android-base/file.h
+++ b/base/include/android-base/file.h
@@ -42,6 +42,17 @@
#endif
bool ReadFully(int fd, void* data, size_t byte_count);
+
+// Reads `byte_count` bytes from the file descriptor at the specified offset.
+// Returns false if there was an IO error or EOF was reached before reading `byte_count` bytes.
+//
+// NOTE: On Linux/Mac, this function wraps pread, which provides atomic read support without
+// modifying the read pointer of the file descriptor. On Windows, however, the read pointer does
+// get modified. This means that ReadFullyAtOffset can be used concurrently with other calls to the
+// same function, but concurrently seeking or reading incrementally can lead to unexpected
+// behavior.
+bool ReadFullyAtOffset(int fd, void* data, size_t byte_count, off64_t offset);
+
bool WriteFully(int fd, const void* data, size_t byte_count);
bool RemoveFileIfExists(const std::string& path, std::string* err = nullptr);
diff --git a/libziparchive/include/ziparchive/zip_archive_stream_entry.h b/libziparchive/include/ziparchive/zip_archive_stream_entry.h
index a40b799..b4766f8 100644
--- a/libziparchive/include/ziparchive/zip_archive_stream_entry.h
+++ b/libziparchive/include/ziparchive/zip_archive_stream_entry.h
@@ -40,7 +40,8 @@
ZipArchiveHandle handle_;
- uint32_t crc32_;
+ off64_t offset_ = 0;
+ uint32_t crc32_ = 0u;
};
#endif // LIBZIPARCHIVE_ZIPARCHIVESTREAMENTRY_H_
diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc
index 17c268b..ad40d42 100644
--- a/libziparchive/zip_archive.cc
+++ b/libziparchive/zip_archive.cc
@@ -435,13 +435,20 @@
static int32_t ValidateDataDescriptor(MappedZipFile& mapped_zip, ZipEntry* entry) {
uint8_t ddBuf[sizeof(DataDescriptor) + sizeof(DataDescriptor::kOptSignature)];
- if (!mapped_zip.ReadData(ddBuf, sizeof(ddBuf))) {
+ off64_t offset = entry->offset;
+ if (entry->method != kCompressStored) {
+ offset += entry->compressed_length;
+ } else {
+ offset += entry->uncompressed_length;
+ }
+
+ if (!mapped_zip.ReadAtOffset(ddBuf, sizeof(ddBuf), offset)) {
return kIoError;
}
const uint32_t ddSignature = *(reinterpret_cast<const uint32_t*>(ddBuf));
- const uint16_t offset = (ddSignature == DataDescriptor::kOptSignature) ? 4 : 0;
- const DataDescriptor* descriptor = reinterpret_cast<const DataDescriptor*>(ddBuf + offset);
+ const uint16_t ddOffset = (ddSignature == DataDescriptor::kOptSignature) ? 4 : 0;
+ const DataDescriptor* descriptor = reinterpret_cast<const DataDescriptor*>(ddBuf + ddOffset);
// Validate that the values in the data descriptor match those in the central
// directory.
@@ -899,7 +906,9 @@
/* read as much as we can */
if (zstream.avail_in == 0) {
const size_t getSize = (compressed_length > kBufSize) ? kBufSize : compressed_length;
- if (!mapped_zip.ReadData(read_buf.data(), getSize)) {
+ off64_t offset = entry->offset + (entry->compressed_length - compressed_length);
+ // Make sure to read at offset to ensure concurrent access to the fd.
+ if (!mapped_zip.ReadAtOffset(read_buf.data(), getSize, offset)) {
ALOGW("Zip: inflate read failed, getSize = %zu: %s", getSize, strerror(errno));
return kIoError;
}
@@ -962,12 +971,15 @@
uint64_t crc = 0;
while (count < length) {
uint32_t remaining = length - count;
+ off64_t offset = entry->offset + count;
- // Safe conversion because kBufSize is narrow enough for a 32 bit signed
- // value.
+ // Safe conversion because kBufSize is narrow enough for a 32 bit signed value.
const size_t block_size = (remaining > kBufSize) ? kBufSize : remaining;
- if (!mapped_zip.ReadData(buf.data(), block_size)) {
- ALOGW("CopyFileToFile: copy read failed, block_size = %zu: %s", block_size, strerror(errno));
+
+ // Make sure to read at offset to ensure concurrent access to the fd.
+ if (!mapped_zip.ReadAtOffset(buf.data(), block_size, offset)) {
+ ALOGW("CopyFileToFile: copy read failed, block_size = %zu, offset = %" PRId64 ": %s",
+ block_size, static_cast<int64_t>(offset), strerror(errno));
return kIoError;
}
@@ -986,12 +998,6 @@
int32_t ExtractToWriter(ZipArchiveHandle handle, ZipEntry* entry, Writer* writer) {
ZipArchive* archive = reinterpret_cast<ZipArchive*>(handle);
const uint16_t method = entry->method;
- off64_t data_offset = entry->offset;
-
- if (!archive->mapped_zip.SeekToOffset(data_offset)) {
- ALOGW("Zip: lseek to data at %" PRId64 " failed", static_cast<int64_t>(data_offset));
- return kIoError;
- }
// this should default to kUnknownCompressionMethod.
int32_t return_value = -1;
@@ -1111,52 +1117,21 @@
}
}
-bool MappedZipFile::SeekToOffset(off64_t offset) {
- if (has_fd_) {
- if (lseek64(fd_, offset, SEEK_SET) != offset) {
- ALOGE("Zip: lseek to %" PRId64 " failed: %s\n", offset, strerror(errno));
- return false;
- }
- return true;
- } else {
- if (offset < 0 || offset > static_cast<off64_t>(data_length_)) {
- ALOGE("Zip: invalid offset: %" PRId64 ", data length: %" PRId64 "\n", offset, data_length_);
- return false;
- }
-
- read_pos_ = offset;
- return true;
- }
-}
-
-bool MappedZipFile::ReadData(uint8_t* buffer, size_t read_amount) {
- if (has_fd_) {
- if (!android::base::ReadFully(fd_, buffer, read_amount)) {
- ALOGE("Zip: read from %d failed\n", fd_);
- return false;
- }
- } else {
- memcpy(buffer, static_cast<uint8_t*>(base_ptr_) + read_pos_, read_amount);
- read_pos_ += read_amount;
- }
- return true;
-}
-
// Attempts to read |len| bytes into |buf| at offset |off|.
bool MappedZipFile::ReadAtOffset(uint8_t* buf, size_t len, off64_t off) {
-#if !defined(_WIN32)
if (has_fd_) {
- if (static_cast<size_t>(TEMP_FAILURE_RETRY(pread64(fd_, buf, len, off))) != len) {
+ if (!android::base::ReadFullyAtOffset(fd_, buf, len, off)) {
ALOGE("Zip: failed to read at offset %" PRId64 "\n", off);
return false;
}
- return true;
+ } else {
+ if (off < 0 || off > static_cast<off64_t>(data_length_)) {
+ ALOGE("Zip: invalid offset: %" PRId64 ", data length: %" PRId64 "\n", off, data_length_);
+ return false;
+ }
+ memcpy(buf, static_cast<uint8_t*>(base_ptr_) + off, len);
}
-#endif
- if (!SeekToOffset(off)) {
- return false;
- }
- return ReadData(buf, len);
+ return true;
}
void CentralDirectory::Initialize(void* map_base_ptr, off64_t cd_start_offset, size_t cd_size) {
diff --git a/libziparchive/zip_archive_private.h b/libziparchive/zip_archive_private.h
index 840f1af..174aa3f 100644
--- a/libziparchive/zip_archive_private.h
+++ b/libziparchive/zip_archive_private.h
@@ -93,14 +93,10 @@
class MappedZipFile {
public:
explicit MappedZipFile(const int fd)
- : has_fd_(true), fd_(fd), base_ptr_(nullptr), data_length_(0), read_pos_(0) {}
+ : has_fd_(true), fd_(fd), base_ptr_(nullptr), data_length_(0) {}
explicit MappedZipFile(void* address, size_t length)
- : has_fd_(false),
- fd_(-1),
- base_ptr_(address),
- data_length_(static_cast<off64_t>(length)),
- read_pos_(0) {}
+ : has_fd_(false), fd_(-1), base_ptr_(address), data_length_(static_cast<off64_t>(length)) {}
bool HasFd() const { return has_fd_; }
@@ -110,10 +106,6 @@
off64_t GetFileLength() const;
- bool SeekToOffset(off64_t offset);
-
- bool ReadData(uint8_t* buffer, size_t read_amount);
-
bool ReadAtOffset(uint8_t* buf, size_t len, off64_t off);
private:
@@ -127,8 +119,6 @@
void* const base_ptr_;
const off64_t data_length_;
- // read_pos_ is the offset to the base_ptr_ where we read data from.
- size_t read_pos_;
};
class CentralDirectory {
diff --git a/libziparchive/zip_archive_stream_entry.cc b/libziparchive/zip_archive_stream_entry.cc
index 50352ef..9ec89b1 100644
--- a/libziparchive/zip_archive_stream_entry.cc
+++ b/libziparchive/zip_archive_stream_entry.cc
@@ -38,13 +38,8 @@
static constexpr size_t kBufSize = 65535;
bool ZipArchiveStreamEntry::Init(const ZipEntry& entry) {
- ZipArchive* archive = reinterpret_cast<ZipArchive*>(handle_);
- off64_t data_offset = entry.offset;
- if (!archive->mapped_zip.SeekToOffset(data_offset)) {
- ALOGW("lseek to data at %" PRId64 " failed: %s", data_offset, strerror(errno));
- return false;
- }
crc32_ = entry.crc32;
+ offset_ = entry.offset;
return true;
}
@@ -61,11 +56,11 @@
protected:
bool Init(const ZipEntry& entry) override;
- uint32_t length_;
+ uint32_t length_ = 0u;
private:
std::vector<uint8_t> data_;
- uint32_t computed_crc32_;
+ uint32_t computed_crc32_ = 0u;
};
bool ZipArchiveStreamEntryUncompressed::Init(const ZipEntry& entry) {
@@ -89,7 +84,7 @@
size_t bytes = (length_ > data_.size()) ? data_.size() : length_;
ZipArchive* archive = reinterpret_cast<ZipArchive*>(handle_);
errno = 0;
- if (!archive->mapped_zip.ReadData(data_.data(), bytes)) {
+ if (!archive->mapped_zip.ReadAtOffset(data_.data(), bytes, offset_)) {
if (errno != 0) {
ALOGE("Error reading from archive fd: %s", strerror(errno));
} else {
@@ -104,6 +99,7 @@
}
computed_crc32_ = crc32(computed_crc32_, data_.data(), data_.size());
length_ -= bytes;
+ offset_ += bytes;
return &data_;
}
@@ -129,9 +125,9 @@
z_stream z_stream_;
std::vector<uint8_t> in_;
std::vector<uint8_t> out_;
- uint32_t uncompressed_length_;
- uint32_t compressed_length_;
- uint32_t computed_crc32_;
+ uint32_t uncompressed_length_ = 0u;
+ uint32_t compressed_length_ = 0u;
+ uint32_t computed_crc32_ = 0u;
};
// This method is using libz macros with old-style-casts
@@ -210,7 +206,7 @@
size_t bytes = (compressed_length_ > in_.size()) ? in_.size() : compressed_length_;
ZipArchive* archive = reinterpret_cast<ZipArchive*>(handle_);
errno = 0;
- if (!archive->mapped_zip.ReadData(in_.data(), bytes)) {
+ if (!archive->mapped_zip.ReadAtOffset(in_.data(), bytes, offset_)) {
if (errno != 0) {
ALOGE("Error reading from archive fd: %s", strerror(errno));
} else {
@@ -220,6 +216,7 @@
}
compressed_length_ -= bytes;
+ offset_ += bytes;
z_stream_.next_in = in_.data();
z_stream_.avail_in = bytes;
}
