Code review: writer-path use-after-free, sys.modules corruption, GIL held across all reader I/O

[soloturn]

AI-assisted review. Filed by agent driven by @soloturn via GDD.

Reviewed the Cython/C++ binding layer (libzim/libzim.pyx, libzim/libwrapper.h, libzim/libwrapper.cpp, libzim/zim.pxd) for performance, memory consumption, simplification, and error-proneness, weighted toward the first two.

Memory (highest severity)

1. libzim/libzim.pyx:111-123, contract at :284-291 — use-after-free: WritingBlob's backing bytes are freed while libzim still holds the zim::Blob. blob_cy_call_fct binds the returned WritingBlob to a local, moves blob.c_blob out, and returns; Cython then decrefs the local. zim::Blob(const char*, size_type) is non-owning by design (per the code's own comment), so once the local dies, its backing bytes object is freed and the zim::Blob handed to ContentProviderWrapper::feed() dangles. The base class works around this by stashing self._blob = next(...), but feed() is a documented, overridable extension point, and the project's own test (tests/test_libzim_creator.py:785-789) does the unsafe thing directly (returns Blob("1") without keeping a reference). Silent heap use-after-free on the writer's hottest path; likely to corrupt ZIM content non-deterministically under memory pressure with larger chunks. Fix: make the lifetime structural — have ContentProviderWrapper hold the PyObject* of the returned blob itself, not rely on the subclass convention.
2. libzim/libzim.pyx:123 → libzim/libwrapper.h:78,96-107 — null-pointer dereference whenever a user's feed() raises. The exception path returns move(zim.Blob()); wrapper::Blob()'s default constructor leaves mp_base null, and the implicit operator zim::Blob() dereferences it before callMethodOnObj even checks error. Any exception inside a user's feed() segfaults the interpreter instead of raising RuntimeError (the equivalent get_size failure path is tested; this one isn't). Fix: give wrapper::Blob a valid empty state, or null-check in the conversion operator, and check error before converting.
3. libzim/libzim.pyx:119,232-238 — WritingBlob.size() derefs null after the blob was consumed. return move(blob.c_blob) moves the unique_ptr out of the live Python object's member with no "moved-from" flag; a later blob.size() call on the same (still valid from Python's view) object dereferences the null pointer and crashes. Fix: copy instead of move, or set a consumed flag and raise from size().
4. libzim/libzim.pyx:912,947-957 — Item.content permanently pins a whole decompressed cluster, with no release. self._blob is cached forever on first access; a zim::Blob holds a shared_ptr to the entire decompressed cluster buffer, not just the item's slice. A consumer holding a list of Items (common when walking an archive) pins one full cluster per item — megabytes each — completely bypassing set_cluster_cache_max_size. Looks like unbounded RSS growth/a leak in practice. Fix: drop the cache when the blob's view count returns to 0, or expose an explicit release.

Performance

5. The entire reader path holds the GIL across blocking I/O and zstd decompression. libzim/zim.pxd:119-183 declares except + but no nogil for the reader API (getData, Archive() open, check(), search, getResults); the writer path already correctly releases the GIL at several sites (:516,539,564,587,592,599). Item.content decompresses up to a full cluster with the GIL held; Archive.check() checksums potentially GBs of I/O with the GIL held; Searcher.search runs a Xapian query with the GIL held. Multi-threaded consumers (a threaded ZIM HTTP server, parallel readers) get zero parallelism and multi-hundred-ms GIL stalls freezing every unrelated thread. Fix: add nogil to the reader declarations (matching the writer's pattern) and wrap the heavy call sites in with nogil; libzim's Archive is documented thread-safe for concurrent reads.
6. libzim/libwrapper.cpp:34-42 — import_libzim() runs unconditionally on every ObjWrapper construction (WriterItemWrapper, ContentProviderWrapper, IndexDataWrapper — 2-3 per item added), each doing a module import plus dict/signature lookups across ~11 exported API functions. For a large write job (mwoffliner/zimit adding millions of items) this is millions of redundant resolutions. Fix: hoist to a one-time static-guarded initialization.
7. libzim/libzim.pyx:84-86,94-100,199 — getattr(obj, method.decode('UTF-8')) allocates a fresh non-interned Python string per call, then does an uncached getattr. Per item added, libzim calls 6+ virtual methods on the user's object (get_path, get_title, get_mimetype, get_hints, get_contentprovider, get_indexdata) plus per-chunk get_size/feed — roughly 10+ transient allocations and un-interned lookups per entry, multiplied by millions of entries. Fix: pass method names as pre-interned PyObject* constants.
8. libzim/libwrapper.cpp:224-235 — getIndexData makes three separate Python round-trips per item (obj_has_attribute, method_is_none, then the actual call) to answer one question; two exist only to probe. Fix: a single PyObject_GetAttrString with a branch on null/None/callable.
9. libzim/libzim.pyx:195-205,188-193 — hints_cy_call_fct builds an intermediate dict comprehension that convertToCppHints then re-iterates a second time; one alloc plus two full traversals per item added.
10. libzim/libzim.pyx:1593 → libzim/libwrapper.h:237-239 — suggestion iteration heap-allocates and deep-copies (new Base(base)) a full zim::SuggestionItem — including snippet computation, the expensive part — just to read one field (getPath()), then discards it. SearchResultSet.__iter__ does this correctly by calling getPath() directly without materializing the item.

Error-proneness

11. libzim/libzim.pyx:822-844,903-927,772-795 (and Search, SearchResultSet, SuggestionSearch, SuggestionResultSet) — every wrapper class is default-constructible from pure Python and segfaults on first use. None define __cinit__, so Entry()/Item() succeed with a null mp_base; both classes are exported in reader_public_objects. Entry().title (or memoryview(ReadingBlob()) via __getbuffer__) crashes the interpreter with no traceback — reachable accidentally via copy.copy/pickle/type(x)() patterns, not just deliberate misuse. Fix: __cinit__ raising TypeError, with internal factories bypassing it via __new__.
12. libzim/zim.pxd:81-82,86-92,120,188,196-198,200-212 — several C++ declarations are missing except +, inconsistently (the same class has it on one method but not its sibling — e.g. Entry::getPath at :121 has it, getTitle at :120 doesn't). Without it, a C++ exception unwinds unhandled out through the CPython eval loop and aborts the process instead of raising a Python exception.
13. libzim/libzim.pyx:63-77 — sys.modules is poisoned with the wrong keys. The registration loop rebinds its name parameter, so sys.modules[name] = module at the end uses the last member's name, not the module's actual name — after import libzim, sys.modules contains bogus entries like sys.modules["Searcher"] == <module libzim.search> and sys.modules["IndexData"] == <module libzim.writer>. Any unrelated import Searcher or import IndexData anywhere in the same process silently returns an unrelated libzim submodule; the intended sys.modules["libzim.writer"] key is never set. Fix: use a distinct loop variable, register under the original name.
14. libzim/libzim.pyx:482-495 — add_illustration has three issues in one method: (a) declares int size while the underlying C++ signature takes unsigned int, so add_illustration(-1, png) silently wraps to 4294967295; (b) it's the only add_* method missing the if not self._started: raise RuntimeError(...) guard every sibling has; (c) its C++ declaration is except + nogil but the call site doesn't use with nogil, unlike its siblings.
15. libzim/libzim.pyx:597-601 — Creator.__exit__ has if True or exc_type is None: — a disabled condition, so finishZimCreation() runs unconditionally even when the with block raised. A with Creator(...) block that dies mid-write still writes a complete-looking but silently-truncated ZIM. Also, if finishZimCreation itself throws, self._started = False (meant to track state) is skipped since it isn't in a finally.

Minor (verified, lower impact)

• libzim/libzim.pyx:797-799 — ReadingBlob.__dealloc__ raising RuntimeError("Blob has views") is dead code: __getbuffer__ increfs buffer.obj, so view_count > 0 implies a live reference and __dealloc__ can't run while views exist; even if reached, an exception in __dealloc__ is only printed, never propagated.
• libzim/libwrapper.cpp:50-55 — ObjWrapper::operator=(ObjWrapper&&) overwrites m_obj without decref'ing the old value — a reference leak, currently unused but a live footgun on a movable type.
• libzim/libzim.pyx:310-312 — BaseWritingItem.__init__ sets local get_indexdata = None (missing self.), a no-op; masked only because WriterItemWrapper::getIndexData's attribute-probe fallback handles the absence correctly anyway.
• libzim/libzim.pyx:1417 (module docstring) — documents with Archive(fpath) as zim:, but Archive defines no __enter__/__exit__; copying the documented snippet raises TypeError. README.md uses the correct non-context-manager form.
• libzim/libzim.pyx:1308-1319 — the DeprecationWarning on get_illustration_sizes points users to get_illustration_infos(), which doesn't exist anywhere in the codebase.
• libzim/libzim.pyx:1149 — bytes(self.c_archive.getMetadata(...)) is redundant; Cython already converts std::string→bytes.
• libzim/libzim.pyx:188-193 vs :200 — convertToCppHints requires Hint enum keys (raising AttributeError on raw ints) while hints_cy_call_fct silently filters out non-Hint keys instead — two code paths for the same concept with opposite failure modes.
• libzim/libwrapper.h:232 vs :233-235 — FORWARD(bool, operator==) on SuggestionIterator expands to a call with no valid conversion (operator!= is hand-written specifically to work around this); zim.pxd:209 declares operator== anyway, so using it from Cython would be a compile error.
• libzim/libzim.pyx:1011-1017 — Archive.__eq__ performs expanduser().resolve() filesystem syscalls on every comparison; the type-check guard is also a roundabout spelling of isinstance.