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

libzim/libzim.pyx、libzim/libwrapper.h、libzim/libwrapper.cpp、libzim/zim.pxd の中で重大度が最も高い指摘から始め、次に tests/test_libzim_creator.py の参照されたケースを実行し、不足しているエラーパスのカバレッジを調査します。このレビューは、選択した指摘に対象を絞った修正とリグレッションカバレッジが用意されて初めて完了します。メモリ、例外、GIL、登録に関する独立した問題が多数あるため、1つの最初のコントリビューションとして扱うのは適していません。

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.