Fix the UniversalDetector uchardet_t handle lifecycle

CHANGES.rst

@@ -17,6 +17,29 @@ 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.
+
+- 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

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)
@@ -44,6 +62,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 +78,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 +124,63 @@ 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. 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
+        # 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 +193,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 +204,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,20 +213,39 @@ 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
 
     @cython.critical_section
     def close(self):
         if not self._closed:
-            self._finalize()
-            uchardet_delete(self._ud)
-            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 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: