libziparchive: Use ReadAtOffset exclusively
The use of ReadAtOffset is meant to allow concurrent access
to the zip archive once it has been loaded. There were places
where this was the case, and some places that did a seek + read
combination, which could lead to data races.
NOTE: On Windows, we are not using pread as the implementation of
ReadAtOffset, therefore the guarantees on Windows are weaker.
On Linux, pread allows the file descriptor to be read at a specific
offset without changing the read pointer. This allows inherited fd's
and duped fds to be read concurrently.
On Windows, we use the ReadFile API, which allows for an atomic seek +
read operation, but modifies the read pointer. This means that any mix
use of ReadAtOffset and Read will have races. Just using ReadAtOffset is
safe.
For the Windows case, this is fine as the libziparchive code now only
uses ReadAtOffset.
Bug: 62184114
Bug: 62101783
Test: make ziparchive-tests (existing tests pass)
Change-Id: Ia7f9a30af2216682cdd9d578d26e84bc46773bb9
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;
}