fix: thread safety, lifetime and bounds-checking follow-ups

[andiwand]

🤖 Generated with Claude Code

Stacked on #658 — review that one first; this PR's diff is against it.

The findings the review turned up that needed more than a surgical edit, so
they were held back out of #658. Mostly concurrency, lifetime and
bounds-checking. 32 files, +660 / −172.

Verification

Same sweep as #658, plus a check that this round changes no output at all.

Build (lib + tests + CLI + JNI)	clean, 0 warnings
Full suite	767 tests, 758 passed, 9 skipped, 0 failed
Reference output vs #658	byte-identical — 72 differing files before and after, no new ones
JNI JUnit (ctest)	passed
Python bindings	build clean, 49/49 pytest
Apple bindings (ODR_APPLE=ON)	build clean, 0 compiler warnings

What is fixed

DynamicUnit's registry was a data race. A function-local static whose
unordered_map is mutated on every DynamicUnit construction, unlocked.
Every Measure built while rendering goes through it, and HttpServer serves
from a thread pool — so two concurrent translates raced. Now a shared_mutex
with a read-locked fast path and a transparent hash so the lookup does not
allocate a key. std::localtime in the logger had the same shape (a shared
static tm) and is now localtime_r / localtime_s, with output unchanged.

Form XObjects could corrupt their caller's graphics state. invoke_x_object
and show_type3 ran a form's content stream against the caller's q/Q
stack, so an unbalanced form destroyed saved states or leaked a CTM. A
ContentScope now pins a restore floor and pops back to exactly the entry
depth.

OOXML encryption headers were read without bounds checks. The
EncryptionInfo path advanced its offset by an attacker-controlled
encryption_header_size, decrypt read the first 8 bytes through an unaligned
reinterpret_cast, and derive_key built a string from a char[16] salt
using a uint32 length taken from the file — a 4 GB read past a 16-byte field.
All go through a bounds-checked cursor now.

pyodr.Error caught almost nothing. py::exception creates the type but
installs no translator, so every unregistered odr::Exception subclass —
NoZipFile, InvalidPath, ServerBindFailed, … — surfaced as a bare
RuntimeError. Registered properly, and deliberately based on
RuntimeError so existing except RuntimeError code keeps working while
except pyodr.Error starts working. Verified both catch paths at runtime.

The GIL was held across every long-running call — translate, warmup,
write, bring_offline, save, decrypt, open — blocking all other Python
threads for the duration. Released, scoped narrowly where the binding builds a
py::bytes inside the lambda. Both re-entry paths (the PyLogger trampoline
and a Python resource_locator through pybind11/functional.h) were read and
confirmed to re-acquire the GIL themselves.

Also: string::split with an empty delimiter never advanced and looped
forever; TemporaryDiskFile was copyable so two objects owned one path and the
first destructor deleted the file under the other; VirtualFileWalker::equals
compared iterators from two different maps; list_file_types built
ZipFile/CfbFile outside its try, so a truncated container threw out of
what is only a probe and killed the graceful fallback in open_file and
magic::mimetype; the PDF type 4 operators cast operands to int32 with a
bare static_cast (undefined out of range) and idiv/mod divided by zero
for any divisor below 1; object streams got the cycle guard the page tree
already had; CFB now derives its sector size from sector_shift and validates
it against [MS-CFB] 2.2; SVM read_primitive throws on a short read instead of
leaving the destination uninitialized and sizing buffers from an unvalidated
length prefix.

The SVM change is the one that could plausibly reject a file that used to
parse, so it was checked rather than assumed: a byte-exact re-implementation of
the header and action loop was run over all four SVM fixtures plus the one
embedded in image-2.odp. Every one consumes to the exact last byte with the
declared action count and never reads short. Likewise all 30 CFB fixtures in
the corpus are major version 3 / sector_shift 9, so they derive 512 exactly
as before.

Deliberately still not fixed

Each of these is a product decision rather than a bug fix, so it wants an
owner's call rather than a drive-by:

• pptx styles never resolve. The .pptx style readers look for
  WordprocessingML attribute names that do not exist in DrawingML —
  rFonts/@ascii instead of a:latin/@typeface, @color/@highlight instead
  of a:solidFill/a:highlight, a:pPr/@jc instead of @algn. So font
  family, colour, background and paragraph alignment are silently never applied
  for pptx, while presentation/README.md ticks them as supported. Fixing it
  changes every pptx reference output.
• Apple nonnull that can be nil. ~40 properties behind
  guarded_value(..., nil) are declared non-nullable, so a genuine engine
  exception puts nil into a slot Swift imports as non-optional. The honest fix
  is nullable (an API change) or returning a default-constructed value (a
  behaviour change).
• ODRElement has no isEqual:/hash, so two wrappers for the same node
  compare unequal in Swift and the type is unusable as a Set key — while JNI
  exposes the same thing as Element.isSame. And ODRElement.exists is
  documented as reachable but is always YES.
• ODF repeated rows/cells inflate linearly. A non-empty row with a large
  table:number-rows-repeated materialises one registry element per repeat.
  Needs a clamp policy, not a local edit.
• File::location() is noexcept and so still cannot report a null impl;
  it returns FileLocation::memory. Making it throw means dropping noexcept
  from a public signature.
• TemporaryDiskFile / TemporaryDiskFileFactory turn out to be dead code —
  nothing outside their own .cpp uses either. Worth deleting, separately.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/odr/table_position.cpp:39

• to_row_num can silently wrap result on very long inputs: std::uint64_t overflow wraps modulo 2^64, and the subsequent > uint32_t::max() check may miss it. This can yield an incorrect row index instead of rejecting out-of-range input.
  src/odr/table_position.cpp:28
• The exception message "s is empty" is unclear (it doesn’t say what input is empty) and looks like a leftover from a different parameter name. A more specific message improves diagnosability without changing behavior.
  src/odr/internal/ooxml/ooxml_crypto.cpp:33
• ByteReader::seek throws "truncated ooxml crypto stream" both when seeking past the end and when seeking backwards. The backward-seek case indicates an invalid offset / inconsistent length field rather than truncation, so the message is misleading.

  /// Jumps to @p offset, which must lie ahead of the cursor and inside the
  /// stream.
  void seek(const std::uint64_t offset) {
    if (offset < m_offset || offset > m_data.size()) {
      throw std::runtime_error("truncated ooxml crypto stream");
    }
    m_offset = static_cast<std::size_t>(offset);

src/odr/html.cpp:131

• The dedup loop builds an unordered_set without reserving, which can cause repeated rehashes for large resources vectors. Reserving upfront makes the O(n) dedup path more predictable.

    std::unordered_set<std::string> seen;
    const auto removed =
        std::ranges::remove_if(resources, [&seen](const auto &resource) {
          return !seen.insert(resource.first.path()).second;
        });