fix: isolate ZIP reader errors and preserve full entry names

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.
 

src/odr/internal/zip/zip_archive.cpp

@@ -9,8 +9,10 @@
 #include <odr/internal/zip/zip_util.hpp>
 
 #include <algorithm>
+#include <limits>
 #include <ostream>
 #include <string>
+#include <utility>
 
 #include <miniz/miniz.h>
 
@@ -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<std::streamoff>(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<WriteSink *>(opaque);
     std::ostream &o = *s->out;
+    if (offset > static_cast<std::uint64_t>(
+                     std::numeric_limits<std::streamoff>::max() - s->base) ||
+        !std::in_range<std::streamsize>(size)) {
+      return std::size_t{0};
+    }
     const std::streamoff position =
         s->base + static_cast<std::streamoff>(offset);
     if (static_cast<std::streamoff>(o.tellp()) != position) {

src/odr/internal/zip/zip_util.cpp

@@ -7,6 +7,8 @@
 #include <algorithm>
 #include <array>
 #include <cstring>
+#include <limits>
+#include <utility>
 
 namespace odr::internal::zip::util {
 
@@ -16,8 +18,8 @@ class ReaderBuffer final : public std::streambuf {
 public:
   ReaderBuffer(std::shared_ptr<const Archive> 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<const Archive> m_archive;
+  mz_zip_archive m_zip;
   std::unique_ptr<mz_zip_reader_extract_iter_state,
                   decltype(&mz_zip_reader_extract_iter_free)>
       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<std::size_t>(stat.m_uncomp_size)) {
+      throw FileReadError();
+    }
+    return static_cast<std::size_t>(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<std::istream> 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<FileInZipIstream>(
@@ -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<char, MZ_ZIP_MAX_ARCHIVE_FILENAME_SIZE> filename{};
-  mz_zip_reader_get_filename(m_archive->zip(), m_index, filename.data(),
-                             static_cast<mz_uint>(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<std::streamoff>(offset) ||
+      !std::in_range<std::streamsize>(size)) {
+    return 0;
+  }
   std::unique_ptr<std::istream> stream;
   {
     std::lock_guard lock(m_mutex);
@@ -220,7 +245,12 @@ Archive::Archive(std::shared_ptr<abstract::File> file)
     throw NullPointerError("Archive: file is nullptr");
   }
   m_source = std::make_unique<ReadSource>(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<std::istream *>(opaque);
+    if (!std::in_range<std::streamsize>(s)) {
+      return 0;
+    }
     in->read(static_cast<char *>(buffer), static_cast<std::streamsize>(s));
-    return in->gcount();
+    return in->bad() ? 0 : static_cast<std::size_t>(in->gcount());
   };
 
   // Without the flag the size only lands in a trailing data descriptor, which

src/odr/internal/zip/zip_util.hpp

@@ -43,8 +43,7 @@ class ReadSource final {
   mutable std::vector<std::unique_ptr<std::istream>> 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<Archive> {
 public:
   explicit Archive(std::shared_ptr<abstract::File> file);
@@ -72,7 +71,7 @@ class Archive final : public std::enable_shared_from_this<Archive> {
     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;

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<MemoryFile>(suffix));
+  }
+  std::ostringstream out;
+  written.save(out);
+  const auto file = std::make_shared<MemoryFile>(out.str());
+  const auto zip = std::make_shared<util::Archive>(file);
+  const auto other = std::make_shared<util::Archive>(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<util::Archive>(std::make_shared<MemoryFile>(bytes));
+  const auto file = zip->begin()->file();
+  std::vector<std::thread> 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);
+}