From 84f175149f15b6083fde252c56b217bf73638378 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 4 Oct 2026 23:00:18 +0200 Subject: [PATCH] fix: isolate ZIP reader errors and preserve full entry names --- CHANGELOG.md | 4 ++ src/odr/internal/zip/zip_archive.cpp | 10 ++++ src/odr/internal/zip/zip_util.cpp | 69 ++++++++++++++++------ src/odr/internal/zip/zip_util.hpp | 5 +- test/src/internal/zip/zip_archive_test.cpp | 40 +++++++++++++ 5 files changed, 107 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24c00d0ee..e50b8c3bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- ZIP preserves full entry names, distinguishes iterators from different + archives, and isolates miniz error state during concurrent reads. Entry + sizes and stream offsets are checked before narrowing. + - XML helpers preserve embedded zero bytes when detecting UTF-16/32, report stream failures, and read encoding names only from declaration attributes. diff --git a/src/odr/internal/zip/zip_archive.cpp b/src/odr/internal/zip/zip_archive.cpp index 13aba1003..437e75484 100644 --- a/src/odr/internal/zip/zip_archive.cpp +++ b/src/odr/internal/zip/zip_archive.cpp @@ -9,8 +9,10 @@ #include #include +#include #include #include +#include #include @@ -98,6 +100,9 @@ void ZipArchive::save(std::ostream &out) const { // miniz addresses the output by absolute offset and rewrites local headers. WriteSink sink{&out, static_cast(out.tellp())}; + if (sink.base < 0) { + throw ZipSaveError(); + } Writer writer; auto &archive = writer.archive; @@ -106,6 +111,11 @@ void ZipArchive::save(std::ostream &out) const { const void *buffer, const std::size_t size) { const auto s = static_cast(opaque); std::ostream &o = *s->out; + if (offset > static_cast( + std::numeric_limits::max() - s->base) || + !std::in_range(size)) { + return std::size_t{0}; + } const std::streamoff position = s->base + static_cast(offset); if (static_cast(o.tellp()) != position) { diff --git a/src/odr/internal/zip/zip_util.cpp b/src/odr/internal/zip/zip_util.cpp index 6fd8418bb..70ddee765 100644 --- a/src/odr/internal/zip/zip_util.cpp +++ b/src/odr/internal/zip/zip_util.cpp @@ -7,6 +7,8 @@ #include #include #include +#include +#include namespace odr::internal::zip::util { @@ -16,8 +18,8 @@ class ReaderBuffer final : public std::streambuf { public: ReaderBuffer(std::shared_ptr archive, const std::uint32_t index) - : m_archive{std::move(archive)}, - m_iter{mz_zip_reader_extract_iter_new(m_archive->zip(), index, 0), + : m_archive{std::move(archive)}, m_zip{*m_archive->zip()}, + m_iter{mz_zip_reader_extract_iter_new(&m_zip, index, 0), mz_zip_reader_extract_iter_free} { if (m_iter == nullptr) { throw FileReadError(); @@ -63,6 +65,7 @@ class ReaderBuffer final : public std::streambuf { } std::shared_ptr m_archive; + mz_zip_archive m_zip; std::unique_ptr m_iter; @@ -97,9 +100,13 @@ class FileInZip final : public abstract::File { return m_archive->file()->location(); } [[nodiscard]] std::size_t size() const override { + auto zip = *m_archive->zip(); mz_zip_archive_file_stat stat{}; - mz_zip_reader_file_stat(m_archive->zip(), m_index, &stat); - return stat.m_uncomp_size; + if (!mz_zip_reader_file_stat(&zip, m_index, &stat) || + !std::in_range(stat.m_uncomp_size)) { + throw FileReadError(); + } + return static_cast(stat.m_uncomp_size); } [[nodiscard]] std::string name() const override { return m_name; } @@ -112,10 +119,11 @@ class FileInZip final : public abstract::File { } [[nodiscard]] std::unique_ptr stream() const override { - if (mz_zip_reader_is_file_encrypted(m_archive->zip(), m_index)) { + auto zip = *m_archive->zip(); + if (mz_zip_reader_is_file_encrypted(&zip, m_index)) { throw UnsupportedOperation("cannot read encrypted zip entry"); } - if (!mz_zip_reader_is_file_supported(m_archive->zip(), m_index)) { + if (!mz_zip_reader_is_file_supported(&zip, m_index)) { throw UnsupportedOperation("zip entry not supported"); } return std::make_unique( @@ -130,25 +138,38 @@ class FileInZip final : public abstract::File { } // namespace -bool Archive::Entry::is_file() const { - return !mz_zip_reader_is_file_a_directory(m_archive->zip(), m_index); -} +bool Archive::Entry::is_file() const { return !is_directory(); } bool Archive::Entry::is_directory() const { - return mz_zip_reader_is_file_a_directory(m_archive->zip(), m_index); + auto zip = *m_archive->zip(); + return mz_zip_reader_is_file_a_directory(&zip, m_index); } RelPath Archive::Entry::path() const { - std::array filename{}; - mz_zip_reader_get_filename(m_archive->zip(), m_index, filename.data(), - static_cast(filename.size())); - // a leading slash is malformed (APPNOTE.TXT 4.4.17.1) and read away - return Path(filename.data()).make_relative(); + auto zip = *m_archive->zip(); + const mz_uint size = mz_zip_reader_get_filename(&zip, m_index, nullptr, 0); + if (size == 0) { + throw FileReadError(); + } + std::string filename(size, '\0'); + if (mz_zip_reader_get_filename(&zip, m_index, filename.data(), size) != + size) { + throw FileReadError(); + } + filename.pop_back(); + if (filename.find('\0') != std::string::npos) { + throw FileReadError(); + } + // APPNOTE.TXT 4.4.17.1 forbids a leading slash; tolerate it on read. + return Path(filename).make_relative(); } Method Archive::Entry::method() const { + auto zip = *m_archive->zip(); mz_zip_archive_file_stat stat{}; - mz_zip_reader_file_stat(m_archive->zip(), m_index, &stat); + if (!mz_zip_reader_file_stat(&zip, m_index, &stat)) { + throw FileReadError(); + } switch (stat.m_method) { case 0: return Method::STORED; @@ -187,6 +208,10 @@ std::size_t ReadSource::read(const std::uint64_t offset, void *buffer, return amount; } + if (!std::in_range(offset) || + !std::in_range(size)) { + return 0; + } std::unique_ptr stream; { std::lock_guard lock(m_mutex); @@ -220,7 +245,12 @@ Archive::Archive(std::shared_ptr file) throw NullPointerError("Archive: file is nullptr"); } m_source = std::make_unique(m_file); - open_from_file(m_zip, *m_file, *m_source); + try { + open_from_file(m_zip, *m_file, *m_source); + } catch (...) { + mz_zip_end(&m_zip); + throw; + } } Archive::~Archive() { mz_zip_end(&m_zip); } @@ -268,8 +298,11 @@ bool util::append_file(mz_zip_archive &archive, const std::string &path, auto read_callback = [](void *opaque, std::uint64_t /*offset*/, void *buffer, const std::size_t s) -> std::size_t { const auto in = static_cast(opaque); + if (!std::in_range(s)) { + return 0; + } in->read(static_cast(buffer), static_cast(s)); - return in->gcount(); + return in->bad() ? 0 : static_cast(in->gcount()); }; // Without the flag the size only lands in a trailing data descriptor, which diff --git a/src/odr/internal/zip/zip_util.hpp b/src/odr/internal/zip/zip_util.hpp index ff17ad13f..b9e28e97c 100644 --- a/src/odr/internal/zip/zip_util.hpp +++ b/src/odr/internal/zip/zip_util.hpp @@ -43,8 +43,7 @@ class ReadSource final { mutable std::vector> m_streams; }; -/// Entries can be read concurrently. `mz_zip_archive::m_last_error` cannot, and -/// is never read. +/// Readers share immutable directory data and copy miniz's mutable error state. class Archive final : public std::enable_shared_from_this { public: explicit Archive(std::shared_ptr file); @@ -72,7 +71,7 @@ class Archive final : public std::enable_shared_from_this { Entry &operator=(Entry &&) noexcept = default; [[nodiscard]] bool operator==(const Entry &other) const { - return m_index == other.m_index; + return m_archive == other.m_archive && m_index == other.m_index; } [[nodiscard]] bool is_file() const; diff --git a/test/src/internal/zip/zip_archive_test.cpp b/test/src/internal/zip/zip_archive_test.cpp index f52bf465f..9d0117736 100644 --- a/test/src/internal/zip/zip_archive_test.cpp +++ b/test/src/internal/zip/zip_archive_test.cpp @@ -367,3 +367,43 @@ TEST(ZipArchive, failed_output_reports_save_errors) { out.setstate(std::ios::badbit); EXPECT_ANY_THROW(zip.save(out)); } + +TEST(ZipArchive, long_entry_names_remain_distinct) { + ZipArchive written; + const std::string prefix(600, 'a'); + for (const auto suffix : {"one", "two"}) { + written.insert_file(written.end(), RelPath(prefix + suffix), + std::make_shared(suffix)); + } + std::ostringstream out; + written.save(out); + const auto file = std::make_shared(out.str()); + const auto zip = std::make_shared(file); + const auto other = std::make_shared(file); + EXPECT_NE(zip->begin(), other->begin()); + const auto filesystem = ZipArchive(zip).as_filesystem(); + for (const auto suffix : {"one", "two"}) { + const auto entry = filesystem->open(AbsPath("/" + prefix + suffix)); + ASSERT_NE(entry, nullptr); + EXPECT_EQ(MemoryFile(*entry).content(), suffix); + } +} + +TEST(ZipArchive, concurrent_corrupt_reads_fail_independently) { + auto bytes = entry_archive(std::string(8192, 'x'), 0); + const auto central = bytes.find("PK\x01\x02"); + ASSERT_NE(central, std::string::npos); + bytes[central + 16] ^= 1; + const auto zip = + std::make_shared(std::make_shared(bytes)); + const auto file = zip->begin()->file(); + std::vector readers; + for (std::size_t i = 0; i < 8; ++i) { + readers.emplace_back( + [&file] { EXPECT_THROW((void)MemoryFile(*file), FileReadError); }); + } + for (auto &reader : readers) { + reader.join(); + } + EXPECT_EQ(zip->zip()->m_last_error, MZ_ZIP_NO_ERROR); +}