From a0417e5f3460844a69cd3533e250ba31ca2776bf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 18:47:59 +0000 Subject: [PATCH 1/4] Fix the UniversalDetector uchardet_t handle lifecycle The uchardet_t handle was only ever released by an explicit close(). There is no __dealloc__, so a detector that was simply dropped leaked it -- and dropping is the documented pattern, because reading `result` finalizes detection on its own precisely so callers can stop without closing. Measured at 19,458 bytes per detector: 20,000 dropped detectors grow RSS by 380 MB on the current branch head, and by 0 with this change. The fix is four inseparable parts: - __dealloc__ releases the handle. - close() and feed()'s error path clear _ud after uchardet_delete(). This is not optional hygiene: tp_dealloc still runs for the object afterwards, so __dealloc__ WITHOUT these assignments turns every explicitly closed detector into a double free. Verified -- a build with __dealloc__ and no NULLing segfaults (rc=139) on the first close-then- drop, where the full change exits 0. - Allocation moves from __init__ to __cinit__, which runs exactly once and cannot be re-entered from Python. This also fixes a pre-existing segfault: a detector built without running __init__ -- via __new__, or a subclass that does not call super().__init__() -- had _ud NULL and dereferenced it on the first feed(). - Every remaining uchardet_* call site is guarded on _ud. Operating on a released detector stays a silent no-op rather than becoming an error: close() has to remain idempotent, and feed()/reset() were already no-ops once _closed was set. __init__ deliberately still resets the stream rather than becoming empty. Making it a no-op would have been a silent wrong-answer regression -- d.__init__() on a live detector would concatenate the next feed() onto the previous stream and report a bogus mixed-encoding label instead of starting fresh. It resets the live handle and allocates a new one only when the previous was closed, so re-init cannot leak either. Separately, detect_with_confidence() now uses try/finally. Assigning uchardet_get_encoding() to a `bytes` is a PyBytes_FromString that can raise MemoryError and jump to Cython's error label, skipping the uchardet_delete() underneath it. A failed uchardet_new() now raises MemoryError rather than being dereferenced. src/tests/test_lifecycle.py covers all of it. Against the unfixed build the suite segfaults on the __new__ case and fails the leak assertion; the re-init tests pass there by construction and exist to keep the behaviour from regressing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TWqKLkfwd8fPxhU4KjXUVB --- CHANGES.rst | 13 +++ src/cchardet/_cchardet.pyx | 95 ++++++++++++---- src/tests/test_lifecycle.py | 208 ++++++++++++++++++++++++++++++++++++ 3 files changed, 298 insertions(+), 18 deletions(-) create mode 100644 src/tests/test_lifecycle.py diff --git a/CHANGES.rst b/CHANGES.rst index 22f45e5..1fab577 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -17,6 +17,19 @@ CHANGES do re-enable the GIL on import; 3.2.0 is the first release that carries the declaration. +- fix a memory leak in ``UniversalDetector``: the underlying ``uchardet_t`` + handle was only released by an explicit ``close()``, so every detector that + was simply dropped -- the documented pattern, since reading ``result`` + finalizes detection on its own -- leaked roughly 19 KB. ``__dealloc__`` now + releases it, and ``close()`` and ``feed()``'s error path clear the handle so + it cannot be released twice. Allocation moved from ``__init__`` to + ``__cinit__``, which also fixes a segfault when a detector was built without + running ``__init__`` (via ``__new__``, or a subclass that does not call + ``super().__init__()``); ``__init__`` still resets the stream, so calling it + again on a live detector starts fresh as before. ``detect()`` no longer + leaks its detector if building the result string raises, and a failed + ``uchardet_new()`` now raises ``MemoryError`` instead of dereferencing NULL. + - document threading expectations for the Python API (`#55`_). ``detect()`` is safe to call concurrently from multiple threads, while a ``UniversalDetector`` instance holds the state of a single stream and must diff --git a/src/cchardet/_cchardet.pyx b/src/cchardet/_cchardet.pyx index 6aac488..d09c7df 100644 --- a/src/cchardet/_cchardet.pyx +++ b/src/cchardet/_cchardet.pyx @@ -44,6 +44,10 @@ cdef int handle_data_chunked(uchardet_t ud, const_char_ptr data, size_t length): def detect_with_confidence(bytes msg): cdef size_t length = len(msg) cdef const_char_ptr data = msg + cdef uchardet_t ud + cdef int result + cdef bytes detected_charset = b"" + cdef float detected_confidence = 0.0 # Encoding-only callers do not need freedesktop uchardet's expensive # language-model pass when the entire payload is already valid UTF-8. @@ -56,21 +60,26 @@ def detect_with_confidence(bytes msg): else: return b"UTF-8", 0.99 - cdef uchardet_t ud = uchardet_new() + ud = uchardet_new() + if ud == NULL: + raise MemoryError("uchardet_new() failed") - cdef int result = handle_data_chunked(ud, data, length) - if result != 0: - uchardet_delete(ud) - raise Exception("Handle data error") + # try/finally rather than a uchardet_delete() before each exit: assigning + # uchardet_get_encoding() to a `bytes` is a PyBytes_FromString, which can + # raise MemoryError and jump straight to Cython's error label. That skipped + # the delete underneath it and leaked the detector. + try: + result = handle_data_chunked(ud, data, length) + if result != 0: + raise Exception("Handle data error") - uchardet_data_end(ud) + uchardet_data_end(ud) - cdef bytes detected_charset = b"" - cdef float detected_confidence = 0.0 - if uchardet_get_n_candidates(ud) > 0: - detected_charset = uchardet_get_encoding(ud, 0) - detected_confidence = uchardet_get_confidence(ud, 0) - uchardet_delete(ud) + if uchardet_get_n_candidates(ud) > 0: + detected_charset = uchardet_get_encoding(ud, 0) + detected_confidence = uchardet_get_confidence(ud, 0) + finally: + uchardet_delete(ud) if detected_charset: return detected_charset, detected_confidence @@ -97,17 +106,60 @@ cdef class UniversalDetector: cdef bytes _detected_charset cdef float _detected_confidence - def __init__(self): + # Handle lifecycle: `_ud` is non-NULL for exactly as long as the handle is + # owned, and NULL once released. Every uchardet_* call site below is + # guarded on that, so operating on a released detector is a silent no-op + # rather than an error -- close() has to stay idempotent, and feed()/reset() + # were already no-ops once _closed was set, so raising would be a behaviour + # change. _finalize()/_read_candidate() are `cdef void` and cannot + # propagate an exception at all; a NULL there degrades to "no candidates". + def __cinit__(self): + # Allocation lives here rather than in __init__ because __cinit__ runs + # exactly once, before the object is reachable from Python, and cannot + # be re-entered. Allocating in __init__ meant a second __init__() call + # overwrote the live handle and leaked it. It also left _ud NULL for an + # object built via __new__ or by a subclass that skips + # super().__init__(), so the first feed() dereferenced NULL. self._ud = uchardet_new() + if self._ud == NULL: + raise MemoryError("uchardet_new() failed") + self._done = 0 + self._finalized = 0 + self._closed = 0 + self._detected_charset = b"" + self._detected_confidence = 0.0 + + @cython.critical_section + def __init__(self): + # Re-initialising in place has to start a genuinely fresh stream: + # `d.__init__()` used to install a brand new handle, and callers who + # rely on that must keep getting a clean detector rather than one that + # silently concatenates the next feed() onto the previous stream. + # Allocation still cannot leak -- the live handle is reset, and only a + # released one is replaced. + if self._ud == NULL: + self._ud = uchardet_new() + if self._ud == NULL: + raise MemoryError("uchardet_new() failed") + else: + uchardet_reset(self._ud) self._done = 0 self._finalized = 0 self._closed = 0 self._detected_charset = b"" self._detected_confidence = 0.0 + def __dealloc__(self): + # Deliberately not decorated with @cython.critical_section: the object + # is being destroyed and is no longer reachable, so there is nothing to + # serialise against, and taking a lock on a dying object is unsound. + if self._ud != NULL: + uchardet_delete(self._ud) + self._ud = NULL + @cython.critical_section def reset(self): - if not self._closed: + if not self._closed and self._ud != NULL: self._done = 0 self._finalized = 0 self._detected_charset = b"" @@ -120,7 +172,7 @@ cdef class UniversalDetector: cdef const_char_ptr data cdef int result - if self._closed or self._finalized: + if self._closed or self._finalized or self._ud == NULL: return length = len(msg) @@ -131,6 +183,7 @@ cdef class UniversalDetector: if result != 0: self._closed = 1 uchardet_delete(self._ud) + self._ud = NULL raise Exception("Handle data error") cdef void _finalize(self): # freedesktop uchardet only publishes candidates from DataEnd(); before @@ -139,7 +192,8 @@ cdef class UniversalDetector: # the only point at which a result exists. Idempotent -- safe to call # from both result and close(). See issue #35. if not self._finalized: - uchardet_data_end(self._ud) + if self._ud != NULL: + uchardet_data_end(self._ud) self._read_candidate() self._finalized = 1 self._done = 1 @@ -148,11 +202,16 @@ cdef class UniversalDetector: def close(self): if not self._closed: self._finalize() - uchardet_delete(self._ud) + if self._ud != NULL: + # Clearing _ud is inseparable from having a __dealloc__: + # tp_dealloc still runs for this object afterwards, so without + # it every explicitly closed detector is a double free. + uchardet_delete(self._ud) + self._ud = NULL self._closed = 1 cdef void _read_candidate(self): - if uchardet_get_n_candidates(self._ud) > 0: + if self._ud != NULL and uchardet_get_n_candidates(self._ud) > 0: self._detected_charset = uchardet_get_encoding(self._ud, 0) self._detected_confidence = uchardet_get_confidence(self._ud, 0) else: diff --git a/src/tests/test_lifecycle.py b/src/tests/test_lifecycle.py new file mode 100644 index 0000000..2e26987 --- /dev/null +++ b/src/tests/test_lifecycle.py @@ -0,0 +1,208 @@ +"""Lifecycle of the C ``uchardet_t`` handle owned by ``UniversalDetector``. + +The handle is a raw C++ allocation (``uchardet_new()`` -> ``new +nsUniversalDetector``). Python's garbage collector, ``sys.getrefcount()`` and +``tracemalloc`` are all blind to it -- they only see the ``PyObject`` wrapper, +which was never the thing that leaked. So the invariants are asserted two ways: + +1. Deterministic behavioural tests, which are the CI gate. They cannot flake: + losing a ``_ud = NULL`` assignment turns ``close()`` + drop into a double + free, i.e. a SIGSEGV inside ``uchardet_delete``, not a soft assertion. +2. One resident-set-size test, run in a subprocess so the measurement is + isolated, with a threshold far below the unpatched signal (~19 KB per + detector). +""" + +import gc +import os +import subprocess +import sys +import textwrap + +import pytest + +import cchardet +from cchardet import _cchardet + +# Not ASCII and not valid UTF-8, so it takes the full uchardet path -- the +# module short-circuits pure UTF-8 input before allocating a detector. +SAMPLE = "한국어 감사합니다 안녕하세요".encode("euc-kr") +OTHER = "Привет мир как дела сегодня хорошо".encode("cp1251") + + +def test_close_then_drop_does_not_double_free(): + """``close()`` releases the handle; ``__dealloc__`` must not release it again. + + ``close()`` has to clear ``_ud`` after ``uchardet_delete()``, because + ``tp_dealloc`` still runs for the same object afterwards. Adding + ``__dealloc__`` without that assignment makes every explicitly closed + detector a double free -- verified to segfault, not merely to warn. + """ + for _ in range(100): + detector = _cchardet.UniversalDetector() + detector.feed(SAMPLE) + detector.close() + del detector + gc.collect() + + +def test_close_is_idempotent(): + detector = _cchardet.UniversalDetector() + detector.feed(SAMPLE) + detector.close() + detector.close() + detector.close() + + +def test_methods_after_close_are_silent_no_ops(): + """Once the handle is released, every uchardet_* call site is skipped. + + ``reset()`` and ``feed()`` were already no-ops on a closed detector, so the + NULL guards preserve that contract rather than starting to raise. + """ + detector = _cchardet.UniversalDetector() + detector.feed(SAMPLE) + detector.close() + closed_result = detector.result + + detector.reset() + detector.feed(SAMPLE) + + assert detector.result == closed_result + assert detector.done is True + + +def test_result_without_close_still_releases_the_handle(): + """The ``result`` property finalizes as a side effect precisely so callers + can stop without closing -- which is what made the missing ``__dealloc__`` + so easy to hit.""" + detector = _cchardet.UniversalDetector() + detector.feed(SAMPLE) + encoding, confidence = detector.result + assert encoding is not None and confidence > 0 + del detector + gc.collect() + + +def test_drop_without_feeding_or_closing(): + for _ in range(100): + _cchardet.UniversalDetector() + gc.collect() + + +def test_reinit_starts_a_fresh_stream(): + """``d.__init__()`` must reset the stream, not concatenate onto it. + + Allocation lives in ``__cinit__`` so a repeat ``__init__()`` cannot leak + the live handle -- but ``__init__`` still has to reset that handle. A + version that simply did nothing silently fed the next payload into the + previous stream and reported a bogus mixed-encoding answer. + """ + baseline = _cchardet.UniversalDetector() + baseline.feed(OTHER) + expected = baseline.result + + detector = _cchardet.UniversalDetector() + detector.feed(SAMPLE) + detector.__init__() + assert detector.done is False + detector.feed(OTHER) + assert detector.result == expected + + # Same again, but after the first stream was finalized by reading result. + detector = _cchardet.UniversalDetector() + detector.feed(SAMPLE) + _ = detector.result + detector.__init__() + detector.feed(OTHER) + assert detector.result == expected + + +def test_reinit_after_close_revives_the_detector(): + """A closed detector gets a brand new handle, matching the behaviour from + when ``uchardet_new()`` lived in ``__init__``.""" + baseline = _cchardet.UniversalDetector() + baseline.feed(OTHER) + expected = baseline.result + + detector = _cchardet.UniversalDetector() + detector.feed(SAMPLE) + detector.close() + + detector.__init__() + assert detector.done is False + detector.feed(OTHER) + assert detector.result == expected + + +def test_uninitialized_instance_does_not_crash(): + """``__cinit__`` runs for every construction path, so the handle exists even + when ``__init__`` never runs. Allocating in ``__init__`` left ``_ud`` NULL + here and the first ``feed()`` dereferenced it.""" + detector = _cchardet.UniversalDetector.__new__(_cchardet.UniversalDetector) + detector.feed(SAMPLE) + assert detector.result[0] is not None + + class Subclass(_cchardet.UniversalDetector): + def __init__(self): # deliberately does not call super().__init__() + pass + + detector = Subclass() + detector.feed(SAMPLE) + assert detector.result[0] is not None + + +def test_constructor_still_rejects_arguments(): + """A no-argument ``__cinit__`` would silently swallow extra constructor + arguments; the explicit ``__init__`` keeps this a TypeError.""" + with pytest.raises(TypeError): + _cchardet.UniversalDetector(1) + + +def test_public_wrapper_context_manager_round_trip(): + with cchardet.UniversalDetector() as detector: + detector.feed(SAMPLE) + assert detector.result["encoding"] is not None + + +# ru_maxrss is KB on Linux but bytes on macOS, and Windows has no resource +# module at all. The leak is platform-independent, so measuring it on Linux is +# enough and avoids encoding the per-platform unit quirks into a CI gate. +@pytest.mark.skipif( + not sys.platform.startswith("linux"), reason="ru_maxrss units are platform-specific" +) +def test_handle_is_not_leaked(): + """Dropping detectors without close() must not grow the heap. + + Before ``__dealloc__`` existed this leaked ~19 KB per detector, so 5000 of + them cost ~95 MB. The threshold sits well below that signal and well above + interpreter noise. + """ + program = textwrap.dedent( + """ + import resource + from cchardet import _cchardet + + SAMPLE = "한국어 감사합니다 안녕하세요".encode("euc-kr") + + def rss_kb(): + return resource.getrusage(resource.RUSAGE_SELF).ru_maxrss + + for _ in range(200): # settle the allocator first + d = _cchardet.UniversalDetector(); d.feed(SAMPLE); _ = d.result + + before = rss_kb() + for _ in range(5000): # note: no close() + d = _cchardet.UniversalDetector(); d.feed(SAMPLE); _ = d.result + print(rss_kb() - before) + """ + ) + completed = subprocess.run( + [sys.executable, "-c", program], + capture_output=True, + text=True, + check=True, + env={**os.environ, "PYTHONIOENCODING": "utf-8"}, + ) + growth_kb = int(completed.stdout.strip()) + assert growth_kb < 20_000, f"RSS grew by {growth_kb} KB; the handle is leaking" From aba8e22c90ab73eebd3a9fc2f59045676c87b9d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 22:07:55 +0000 Subject: [PATCH 2/4] Translate uchardet's C++ allocation failures instead of aborting Follow-up to the handle-lifecycle work: two remaining out-of-memory gaps, both found by review of that change. uchardet is C++ and allocates with plain `new`, which throws std::bad_alloc rather than returning NULL -- uchardet_new() is `new HandleUniversalDetector`, HandleData() news the group probers, Reset() news nsMBCSGroupProber's code-point buffers, DataEnd() reports candidates into a std::vector. uchardet's own `if (nsnull == ...) return NS_ERROR_OUT_OF_MEMORY` checks are therefore dead code, and the NULL checks on this side never fire either. The exception instead unwound out of the extension into CPython's C frames, which is undefined behaviour and observably std::terminate(): the new test aborts with SIGABRT on an unpatched build, reproducibly. Declaring the allocating entry points `except +` makes Cython translate it to MemoryError. uchardet_delete() is left alone -- it runs a destructor, and it is called from __dealloc__ where nothing could be propagated. The NULL checks stay for implementations that do return NULL. close() now releases the handle in a `finally`. _finalize() can raise -- uchardet_data_end() is now `except +`, and _read_candidate() assigns uchardet_get_encoding() to a `bytes`, a PyBytes_FromString that can raise MemoryError. Being `cdef void` does not swallow that: since Cython 3 those propagate via a PyErr_Occurred() check at the call site, and the generated code jumped straight past the uchardet_delete(), leaving an explicit close() that released nothing with _closed unset. This mirrors the try/finally already used in detect_with_confidence(). The new test pins the allocation-failure path: it caps RLIMIT_AS, mmaps the remaining address space away and holds detectors until uchardet's `new` has to reach the OS. It is Linux-only, runs in a subprocess, and skips rather than fails if an allocator will not be squeezed, so the only way it reports failure is the crash it exists to catch. The close() path has no test: reaching it needs a Python-level allocation failure inside finalization, and under this kind of pressure data_end()'s small allocations are still served from the free list. It was verified by inspecting the generated C++ instead. --- CHANGES.rst | 10 ++++ src/cchardet/_cchardet.pyx | 62 +++++++++++++++++++------ src/tests/test_lifecycle.py | 93 +++++++++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 14 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 1fab577..331afde 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -30,6 +30,16 @@ CHANGES leaks its detector if building the result string raises, and a failed ``uchardet_new()`` now raises ``MemoryError`` instead of dereferencing NULL. +- stop aborting the interpreter when uchardet runs out of memory. uchardet is + C++ and allocates with plain ``new``, which throws ``std::bad_alloc`` rather + than returning NULL, so its own out-of-memory checks never fired and the + exception unwound out of the extension into CPython's C frames -- undefined + behaviour, in practice ``std::terminate()`` and a ``SIGABRT``. Every + allocating uchardet entry point is now declared ``except +``, so an + allocation failure surfaces as a normal ``MemoryError``. Relatedly, + ``close()`` now releases the handle in a ``finally``, so it cannot return + having released nothing when finalizing the stream raises. + - document threading expectations for the Python API (`#55`_). ``detect()`` is safe to call concurrently from multiple threads, while a ``UniversalDetector`` instance holds the state of a single stream and must diff --git a/src/cchardet/_cchardet.pyx b/src/cchardet/_cchardet.pyx index d09c7df..e4eb1b2 100644 --- a/src/cchardet/_cchardet.pyx +++ b/src/cchardet/_cchardet.pyx @@ -8,13 +8,31 @@ cdef extern from *: # Upstream freedesktop uchardet (>= 0.1.0) multi-candidate API. uchardet returns # an ordered list of candidate encodings; we take the first (best) one. +# +# Every entry point that allocates is declared `except +`. uchardet is C++ and +# allocates with plain `new` -- uchardet_new() is `new HandleUniversalDetector`, +# HandleData() news the group probers, Reset() news nsMBCSGroupProber's +# code-point buffers, DataEnd() reports candidates into a std::vector. On a +# conforming compiler those throw std::bad_alloc rather than returning NULL, so +# uchardet's own `if (nsnull == ...) return NS_ERROR_OUT_OF_MEMORY` checks are +# dead code. This module is built as C++ (cython_language=cpp), but the frames +# above it are CPython's C ones: letting the exception unwind through them is +# undefined behaviour, in practice std::terminate(). `except +` makes Cython +# wrap the call and translate std::bad_alloc into MemoryError instead. The NULL +# checks below are kept as well -- they cost nothing and still cover any +# implementation that does return NULL, including a system libuchardet built +# with -fno-exceptions. +# +# uchardet_delete() is deliberately left alone: it runs the destructor, which +# is implicitly noexcept, and it is called from __dealloc__ where an exception +# could not be propagated anyway. The getters only index a std::vector. cdef extern from "uchardet.h": ctypedef void* uchardet_t - cdef uchardet_t uchardet_new() + cdef uchardet_t uchardet_new() except + cdef void uchardet_delete(uchardet_t ud) - cdef int uchardet_handle_data(uchardet_t ud, const_char_ptr data, size_t length) - cdef void uchardet_data_end(uchardet_t ud) - cdef void uchardet_reset(uchardet_t ud) + cdef int uchardet_handle_data(uchardet_t ud, const_char_ptr data, size_t length) except + + cdef void uchardet_data_end(uchardet_t ud) except + + cdef void uchardet_reset(uchardet_t ud) except + cdef size_t uchardet_get_n_candidates(uchardet_t ud) cdef const_char_ptr uchardet_get_encoding(uchardet_t ud, size_t candidate) cdef float uchardet_get_confidence(uchardet_t ud, size_t candidate) @@ -111,8 +129,11 @@ cdef class UniversalDetector: # guarded on that, so operating on a released detector is a silent no-op # rather than an error -- close() has to stay idempotent, and feed()/reset() # were already no-ops once _closed was set, so raising would be a behaviour - # change. _finalize()/_read_candidate() are `cdef void` and cannot - # propagate an exception at all; a NULL there degrades to "no candidates". + # change. A NULL in _finalize()/_read_candidate() degrades to "no + # candidates". Note that being `cdef void` does not make those two + # noexcept: since Cython 3 they propagate exceptions like any other cdef + # function, via a PyErr_Occurred() check at the call site. close() relies + # on that being true (see the try/finally there). def __cinit__(self): # Allocation lives here rather than in __init__ because __cinit__ runs # exactly once, before the object is reachable from Python, and cannot @@ -201,14 +222,27 @@ cdef class UniversalDetector: @cython.critical_section def close(self): if not self._closed: - self._finalize() - if self._ud != NULL: - # Clearing _ud is inseparable from having a __dealloc__: - # tp_dealloc still runs for this object afterwards, so without - # it every explicitly closed detector is a double free. - uchardet_delete(self._ud) - self._ud = NULL - self._closed = 1 + # try/finally for exactly the reason detect_with_confidence() uses + # one. _finalize() can raise: uchardet_data_end() is `except +`, and + # _read_candidate() assigns uchardet_get_encoding() to a `bytes`, + # which is a PyBytes_FromString that can raise MemoryError. Being + # `cdef void` does not swallow that -- Cython 3 propagates out of a + # void cdef function via a PyErr_Occurred() check at the call site, + # so the generated code jumped straight past the uchardet_delete() + # below. An explicit close() could then return having released + # nothing, with _closed still unset. Releasing the handle is the one + # thing close() must do even when it cannot build a result. + try: + self._finalize() + finally: + if self._ud != NULL: + # Clearing _ud is inseparable from having a __dealloc__: + # tp_dealloc still runs for this object afterwards, so + # without it every explicitly closed detector is a double + # free. + uchardet_delete(self._ud) + self._ud = NULL + self._closed = 1 cdef void _read_candidate(self): if self._ud != NULL and uchardet_get_n_candidates(self._ud) > 0: diff --git a/src/tests/test_lifecycle.py b/src/tests/test_lifecycle.py index 2e26987..cf82f6b 100644 --- a/src/tests/test_lifecycle.py +++ b/src/tests/test_lifecycle.py @@ -206,3 +206,96 @@ def rss_kb(): ) growth_kb = int(completed.stdout.strip()) assert growth_kb < 20_000, f"RSS grew by {growth_kb} KB; the handle is leaking" + + +# RLIMIT_AS is the lever that makes this deterministic, and it only means what +# we need it to mean on Linux. The bug is platform-independent, so testing it +# on one platform is enough. +@pytest.mark.skipif( + not sys.platform.startswith("linux"), reason="RLIMIT_AS pressure is Linux-specific" +) +def test_allocation_failure_raises_instead_of_aborting(): + """An out-of-memory uchardet must raise ``MemoryError``, not kill the process. + + uchardet allocates with plain ``new``, so allocation failure throws + ``std::bad_alloc``; its own ``if (nsnull == ...) return + NS_ERROR_OUT_OF_MEMORY`` checks are dead code. Without ``except +`` on the + allocating entry points that exception unwinds out of the extension into + CPython's C frames, which is undefined behaviour -- and observably + ``std::terminate()``: this program aborts with SIGABRT and + ``terminate called after throwing an instance of 'std::bad_alloc'`` on an + unpatched build, reproducibly, where the patched build exits 0. + + Run in a subprocess because it deliberately exhausts the address space. + """ + program = textwrap.dedent( + """ + import mmap, resource + from cchardet import _cchardet + + SAMPLE = "한국어 감사합니다".encode("euc-kr") + + # Warm the code paths first: nothing after the limit is applied should + # need a lazy import or a first-touch allocation of its own. + d = _cchardet.UniversalDetector(); d.feed(SAMPLE); _ = d.result + del d + + _soft, hard = resource.getrlimit(resource.RLIMIT_AS) + with open("/proc/self/statm") as fh: + usage = int(fh.read().split()[0]) * 4096 + resource.setrlimit(resource.RLIMIT_AS, (usage + (8 << 20), hard)) + + # Consume the remaining address space down to page granularity. mmap is + # a direct syscall, so this is exact and does not disturb pymalloc. + blocks = [] + size = 1 << 20 + while size >= 4096: + try: + blocks.append(mmap.mmap(-1, size)) + except (OSError, MemoryError, ValueError): + size >>= 1 + + # Hold every detector, so the C++ heap free list drains and uchardet's + # `new` has to go to the OS -- otherwise it just recycles the warm-up + # allocation and never fails. + held = [] + outcome = "no-pressure" + try: + for _ in range(100000): + d = _cchardet.UniversalDetector() + d.feed(SAMPLE) + held.append(d) + except MemoryError: + outcome = "MemoryError" + + # Lift the limit before anything else: interpreter shutdown and even + # freeing can need to allocate, and stdout is a pipe here, so an abort + # after this point would discard the buffered answer. + resource.setrlimit(resource.RLIMIT_AS, (_soft, hard)) + for b in blocks: + b.close() + del held + print(outcome, flush=True) + """ + ) + completed = subprocess.run( + [sys.executable, "-c", program], + capture_output=True, + text=True, + env={**os.environ, "PYTHONIOENCODING": "utf-8"}, + ) + + assert completed.returncode >= 0, ( + f"interpreter killed by signal {-completed.returncode} -- a C++ exception " + f"escaped into CPython's C frames:\n{completed.stderr}" + ) + assert "std::bad_alloc" not in completed.stderr, ( + f"std::bad_alloc was not translated:\n{completed.stderr}" + ) + + outcome = completed.stdout.strip().splitlines()[-1] if completed.stdout.strip() else "" + if outcome == "no-pressure": + # Some allocators will not let us squeeze hard enough. Nothing was + # proven, but nothing crashed either -- do not fail on that. + pytest.skip("could not force an allocation failure on this allocator") + assert outcome == "MemoryError", f"unexpected outcome {outcome!r}: {completed.stderr}" From dc184fc5ec1d5a9c839e14a3e30e059e671de68e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 22:48:21 +0000 Subject: [PATCH 3/4] Test that a close() which raises still releases the handle The close() half of the previous commit shipped without a test: real memory pressure could not reach it, because the C++ allocations inside uchardet_data_end() are small and keep being served from the heap free list long after the address space is exhausted, so close() simply succeeded. _testcapi.set_nomemory() is the right lever instead. It fails PyMem_*/PyObject_* precisely and on demand while leaving C++ `new` alone, so the failure lands exactly where the bug lives: the PyBytes_FromString in _read_candidate(). The two injection techniques are not interchangeable -- RLIMIT_AS reaches uchardet's `new` and nothing else, set_nomemory() reaches the Python allocator and nothing else -- so each test uses the one that reaches its bug. Two tests, because the obvious assertion is only a proxy. That a failed close() leaves the detector reporting "closed" shows _closed was set, not that uchardet_delete() ran. So the second test holds every detector whose close() raised: nothing is dropped, __dealloc__ never runs, and close() is the only thing that can have released a handle. Without the finally that leaks ~19 KB per detector, the same signature as the missing __dealloc__; with it, ~87 B, which is just the PyObject wrappers. Verified by reverting only the finally, leaving `except +` in place: both tests fail, the other twelve pass. Both skip where _testcapi is absent (PyPy, stripped builds); the RSS one is Linux-only for the usual ru_maxrss reason. --- src/tests/test_lifecycle.py | 184 +++++++++++++++++++++++++++++++++++- 1 file changed, 181 insertions(+), 3 deletions(-) diff --git a/src/tests/test_lifecycle.py b/src/tests/test_lifecycle.py index cf82f6b..d33e1b3 100644 --- a/src/tests/test_lifecycle.py +++ b/src/tests/test_lifecycle.py @@ -8,9 +8,16 @@ 1. Deterministic behavioural tests, which are the CI gate. They cannot flake: losing a ``_ud = NULL`` assignment turns ``close()`` + drop into a double free, i.e. a SIGSEGV inside ``uchardet_delete``, not a soft assertion. -2. One resident-set-size test, run in a subprocess so the measurement is - isolated, with a threshold far below the unpatched signal (~19 KB per - detector). +2. Resident-set-size tests, run in a subprocess so the measurement is isolated, + with thresholds far below the unpatched signal (~19 KB per detector). + +The out-of-memory paths need the failure to be injected, and the two levers are +not interchangeable. Capping ``RLIMIT_AS`` and exhausting the address space is +what makes the *C++* ``new`` inside uchardet fail; it cannot be aimed at the +Python allocator, and it is too coarse to fail a small allocation on demand, +because the heap free list keeps serving those. ``_testcapi.set_nomemory()`` +is the opposite: it fails ``PyMem_*``/``PyObject_*`` precisely and on demand, +and leaves C++ ``new`` alone. So each test uses the one that reaches its bug. """ import gc @@ -30,6 +37,51 @@ OTHER = "Привет мир как дела сегодня хорошо".encode("cp1251") +def _require_nomemory_hook(): + """``_testcapi.set_nomemory()`` makes the *Python* allocator fail on demand. + + That is the lever for the two tests below: they need finalization to raise + part-way through ``close()``. Real memory pressure cannot do it -- the C++ + allocations inside ``uchardet_data_end()`` are small and get served from the + heap free list long after the address space is exhausted (measured). The + hook is precise instead, and it only touches ``PyMem_*``/``PyObject_*``, so + ``uchardet_data_end()`` still succeeds and the failure lands exactly where + the bug lives: the ``PyBytes_FromString`` in ``_read_candidate()``. + + It is a CPython-internal test module -- absent on PyPy, and strippable. + """ + testcapi = pytest.importorskip( + "_testcapi", reason="needs CPython's allocator-failure injection" + ) + if not hasattr(testcapi, "set_nomemory"): + pytest.skip("this build's _testcapi has no set_nomemory()") + + +# Installing the allocator hook is process-global and leaves the interpreter in +# a delicate state, so both tests run it in a subprocess rather than risk +# poisoning the rest of the session. +_FAILING_CLOSE_PREAMBLE = """ +import _testcapi +from cchardet import _cchardet + +SAMPLE = "한국어 감사합니다 안녕하세요".encode("euc-kr") + +def failing_close(detector): + "close() a detector with every Python allocation failing." + try: + _testcapi.set_nomemory(0) + detector.close() + except MemoryError: + return "MemoryError" + except BaseException as exc: + return type(exc).__name__ + else: + return "no-error" + finally: + _testcapi.remove_mem_hooks() +""" + + def test_close_then_drop_does_not_double_free(): """``close()`` releases the handle; ``__dealloc__`` must not release it again. @@ -299,3 +351,129 @@ def test_allocation_failure_raises_instead_of_aborting(): # proven, but nothing crashed either -- do not fail on that. pytest.skip("could not force an allocation failure on this allocator") assert outcome == "MemoryError", f"unexpected outcome {outcome!r}: {completed.stderr}" + + +def test_close_still_releases_the_handle_when_finalizing_raises(): + """``close()`` must release the handle even if it cannot build the result. + + ``close()`` calls ``_finalize()`` first, and ``_finalize()`` can raise: + ``_read_candidate()`` assigns ``uchardet_get_encoding()`` to a ``bytes``, + which is a ``PyBytes_FromString``. Being ``cdef void`` does not make that + safe -- since Cython 3 a void ``cdef`` function propagates exceptions via a + ``PyErr_Occurred()`` check at the call site, so without a ``finally`` the + generated code jumps straight past ``uchardet_delete()`` *and* past + ``self._closed = 1``. + + The detector is then left wide open: not closed, handle still held. That is + observable, and it is what this test pins. On a build without the + ``finally`` the failed ``close()`` is simply undone -- reading ``result`` + afterwards silently re-finalizes the stream and hands back ``UHC`` -- where + the fixed build reports a closed detector. + """ + _require_nomemory_hook() + + program = _FAILING_CLOSE_PREAMBLE + textwrap.dedent( + """ + # The same stream, closed normally: proves the sample still detects, so + # a `None` from the victim below means "released", not "never worked". + reference = _cchardet.UniversalDetector() + reference.feed(SAMPLE) + reference.close() + print("reference", reference.result[0] is not None, flush=True) + + victim = _cchardet.UniversalDetector() + victim.feed(SAMPLE) + print("raised", failing_close(victim), flush=True) + + # A detector whose close() released the handle has nothing left to + # finalize, so result stays empty. One that did not re-finalizes here + # and answers as if close() had never been called. + print("after", "closed" if victim.result[0] is None else "open", flush=True) + + victim.close() # still idempotent + del victim # and not a double free + print("survived", flush=True) + """ + ) + completed = subprocess.run( + [sys.executable, "-c", program], + capture_output=True, + text=True, + env={**os.environ, "PYTHONIOENCODING": "utf-8"}, + ) + assert completed.returncode == 0, ( + f"subprocess died (rc={completed.returncode}):\n{completed.stderr}" + ) + + reported = dict( + line.split(" ", 1) for line in completed.stdout.strip().splitlines() if " " in line + ) + assert reported.get("reference") == "True", "the sample stopped detecting" + assert reported.get("raised") == "MemoryError", ( + f"close() did not raise from finalization: {reported}\n{completed.stderr}" + ) + assert reported.get("after") == "closed", ( + "close() raised and left the detector open -- the handle was not " + "released; it needs to be freed in a finally" + ) + assert "survived" in completed.stdout + + +@pytest.mark.skipif( + not sys.platform.startswith("linux"), reason="ru_maxrss units are platform-specific" +) +def test_failed_close_does_not_leak_the_handle(): + """The direct measurement behind the test above: the handle is really gone. + + ``result`` reporting "closed" is a proxy -- it shows ``_closed`` was set, + not that ``uchardet_delete()`` ran. So hold every detector whose ``close()`` + raised: nothing is dropped, ``__dealloc__`` never runs, and the only thing + that can have released a handle is ``close()`` itself. Without the + ``finally`` this leaks the full ~19 KB per detector, the same signature as + the missing ``__dealloc__``. + """ + _require_nomemory_hook() + + program = _FAILING_CLOSE_PREAMBLE + textwrap.dedent( + """ + import resource + + N = 3000 + + def rss_kb(): + return resource.getrusage(resource.RUSAGE_SELF).ru_maxrss + + held = [] # nothing is ever dropped, so __dealloc__ cannot help + + def closed_detector(): + detector = _cchardet.UniversalDetector() + detector.feed(SAMPLE) + failing_close(detector) + held.append(detector) + + for _ in range(200): # settle the allocator first + closed_detector() + + before = rss_kb() + for _ in range(N): + closed_detector() + print(len(held), (rss_kb() - before) * 1024 // N, flush=True) + """ + ) + completed = subprocess.run( + [sys.executable, "-c", program], + capture_output=True, + text=True, + check=True, + env={**os.environ, "PYTHONIOENCODING": "utf-8"}, + ) + held, per_detector = (int(field) for field in completed.stdout.split()) + + assert held == 3200, "detectors were dropped; __dealloc__ could mask the leak" + # Measured: ~87 B/detector (just the PyObject wrappers) with the finally, + # ~19,500 B/detector without it. The threshold sits between the two, orders + # of magnitude clear of both. + assert per_detector < 1000, ( + f"a close() that raised leaked {per_detector} B/detector; the handle " + f"is not being released in a finally" + ) From 86f450ebc1bdb77e831a02b9d8a39ecba2fa84a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 22:54:28 +0000 Subject: [PATCH 4/4] Make the allocation-failure test bias towards skipping It went red on 3.10, 3.13 and 3.14 with "cannot allocate memory for thread-local data: ABORT" and an empty verdict. That is the dynamic loader dying under the squeeze, not uchardet: starving a process this hard makes it fragile in ways unrelated to what is being tested, and the test reported that as a failure. Two changes. The verdict is now written to a file the instant the MemoryError is caught -- still under pressure, using only an fd and a bytes object prepared beforehand -- instead of being printed at the end, where cleanup or interpreter shutdown dying first would erase it. And only the actual bug signature (std::bad_alloc / "terminate called" in stderr) counts as failure; anything else means the experiment did not run on this runner, which is a skip. The test still catches the bug it exists for: with `except +` reverted it fails 3/3 with rc=-6. With the fix it passes 5/5, and the CI failure mode now lands on the skip path rather than the assert. --- src/tests/test_lifecycle.py | 63 ++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/tests/test_lifecycle.py b/src/tests/test_lifecycle.py index d33e1b3..a89fde2 100644 --- a/src/tests/test_lifecycle.py +++ b/src/tests/test_lifecycle.py @@ -266,7 +266,7 @@ def rss_kb(): @pytest.mark.skipif( not sys.platform.startswith("linux"), reason="RLIMIT_AS pressure is Linux-specific" ) -def test_allocation_failure_raises_instead_of_aborting(): +def test_allocation_failure_raises_instead_of_aborting(tmp_path): """An out-of-memory uchardet must raise ``MemoryError``, not kill the process. uchardet allocates with plain ``new``, so allocation failure throws @@ -274,21 +274,35 @@ def test_allocation_failure_raises_instead_of_aborting(): NS_ERROR_OUT_OF_MEMORY`` checks are dead code. Without ``except +`` on the allocating entry points that exception unwinds out of the extension into CPython's C frames, which is undefined behaviour -- and observably - ``std::terminate()``: this program aborts with SIGABRT and - ``terminate called after throwing an instance of 'std::bad_alloc'`` on an - unpatched build, reproducibly, where the patched build exits 0. + ``std::terminate()``: on an unpatched build this program dies with SIGABRT + and ``terminate called after throwing an instance of 'std::bad_alloc'``, + reproducibly, where the patched build reports ``MemoryError``. Run in a subprocess because it deliberately exhausts the address space. + + Deliberately biased towards skipping. Squeezing a process this hard makes + it fragile in ways that have nothing to do with uchardet -- a runner can + die in the dynamic loader ("cannot allocate memory for thread-local data") + before the experiment even finishes. So the verdict is written to a file + the instant it is known, rather than printed at the end where any later + death would erase it, and only the specific ``std::bad_alloc`` signature is + treated as failure. Anything else means the experiment did not run, not + that the code is broken. """ + verdict = tmp_path / "verdict" + verdict.touch() + program = textwrap.dedent( """ - import mmap, resource + import os, sys, mmap, resource from cchardet import _cchardet SAMPLE = "한국어 감사합니다".encode("euc-kr") - # Warm the code paths first: nothing after the limit is applied should - # need a lazy import or a first-touch allocation of its own. + # Everything the post-squeeze section needs, prepared while allocation + # still works: the open fd, the message bytes, and every code path. + fd = os.open(sys.argv[1], os.O_WRONLY) + VERDICT = b"MemoryError" d = _cchardet.UniversalDetector(); d.feed(SAMPLE); _ = d.result del d @@ -311,46 +325,43 @@ def test_allocation_failure_raises_instead_of_aborting(): # `new` has to go to the OS -- otherwise it just recycles the warm-up # allocation and never fails. held = [] - outcome = "no-pressure" try: for _ in range(100000): d = _cchardet.UniversalDetector() d.feed(SAMPLE) held.append(d) except MemoryError: - outcome = "MemoryError" + # Record it here, still under pressure, using only objects that + # already exist. Cleanup and interpreter shutdown come next and can + # themselves die on a starved runner; the answer is already on disk. + os.write(fd, VERDICT) - # Lift the limit before anything else: interpreter shutdown and even - # freeing can need to allocate, and stdout is a pipe here, so an abort - # after this point would discard the buffered answer. resource.setrlimit(resource.RLIMIT_AS, (_soft, hard)) for b in blocks: b.close() del held - print(outcome, flush=True) """ ) completed = subprocess.run( - [sys.executable, "-c", program], + [sys.executable, "-c", program, str(verdict)], capture_output=True, text=True, env={**os.environ, "PYTHONIOENCODING": "utf-8"}, ) - assert completed.returncode >= 0, ( - f"interpreter killed by signal {-completed.returncode} -- a C++ exception " - f"escaped into CPython's C frames:\n{completed.stderr}" - ) - assert "std::bad_alloc" not in completed.stderr, ( - f"std::bad_alloc was not translated:\n{completed.stderr}" + # The one true failure signature: the C++ exception reached CPython's C + # frames and std::terminate ran. + escaped = "std::bad_alloc" in completed.stderr or "terminate called" in completed.stderr + assert not escaped, ( + f"std::bad_alloc escaped into CPython's C frames instead of being " + f"translated (rc={completed.returncode}):\n{completed.stderr}" ) - outcome = completed.stdout.strip().splitlines()[-1] if completed.stdout.strip() else "" - if outcome == "no-pressure": - # Some allocators will not let us squeeze hard enough. Nothing was - # proven, but nothing crashed either -- do not fail on that. - pytest.skip("could not force an allocation failure on this allocator") - assert outcome == "MemoryError", f"unexpected outcome {outcome!r}: {completed.stderr}" + if verdict.read_bytes() != b"MemoryError": + pytest.skip( + f"allocation pressure did not reach uchardet on this runner " + f"(rc={completed.returncode}): {completed.stderr.strip()[:200]}" + ) def test_close_still_releases_the_handle_when_finalizing_raises():