fix: raise SystemError when a key's C slot fails without an exception

src/internal/utils.rs

@@ -16,13 +16,23 @@ pub unsafe fn pyobject_hash(
 ) -> pyo3::PyResult<u64> {
     let py_hash = pyo3::ffi::PyObject_Hash(arg1);
     if crate::hashbrown::util::unlikely(py_hash == -1) {
-        // SAFETY: PyObject_Hash never returns -1 on success.
-        return Err(pyo3::PyErr::take(py).unwrap());
+        // PyObject_Hash never returns -1 on success.
+        return Err(take_error(py));
     }
 
     Ok(mix_hash(py_hash as u64))
 }
 
+/// Takes the exception set by a failed C API call.
+///
+/// A broken C type can report an error without setting one; like CPython, raise
+/// `SystemError` then instead of panicking (release builds abort on panic).
+#[cold]
+fn take_error(py: pyo3::Python<'_>) -> pyo3::PyErr {
+    pyo3::PyErr::take(py)
+        .unwrap_or_else(|| new_py_error!(PySystemError, "error return without exception set"))
+}
+
 /// Spreads entropy of a Python hash across all 64 bits.
 ///
 /// CPython hashes are frequently low-entropy in the high bits: `hash(n) == n` for
@@ -55,7 +65,7 @@ pub unsafe fn pyobject_equal(
     let boolean = pyo3::ffi::PyObject_RichCompareBool(arg1, arg2, pyo3::ffi::Py_EQ);
 
     if boolean < 0 {
-        Err(pyo3::PyErr::take(py).unwrap_unchecked())
+        Err(take_error(py))
     } else {
         Ok(boolean == 1)
     }

tests/test_hashing.py

@@ -5,6 +5,10 @@
 hashes alone do not).
 """
 
+import platform
+import subprocess
+import sys
+
 import pytest
 
 import cachebox
@@ -79,3 +83,74 @@ def test_many_low_entropy_keys(cache_cls, keys):
         del c[k]
     for i, k in enumerate(keys):
         assert (k in c) == (i % 2 == 1)
+
+
+# A C type whose tp_hash / tp_richcompare report an error without setting an
+# exception, as a buggy extension could. CPython turns that into SystemError.
+BROKEN_C_KEY = """
+import ctypes
+import sys
+
+import cachebox
+
+name, slot = sys.argv[1], sys.argv[2]
+cls = getattr(cachebox, name)
+cache = cls(0, 3600) if name == "TTLCache" else cls(0)
+
+HASH = ctypes.CFUNCTYPE(ctypes.c_ssize_t, ctypes.c_void_p)
+RICHCMP = ctypes.CFUNCTYPE(ctypes.c_void_p, ctypes.c_void_p, ctypes.c_void_p, ctypes.c_int)
+broken = slot == "hash"
+tp_hash = HASH(lambda o: -1 if broken else 1)
+tp_richcompare = RICHCMP(lambda a, b, op: None)  # NULL
+
+
+class Slot(ctypes.Structure):
+    _fields_ = [("slot", ctypes.c_int), ("pfunc", ctypes.c_void_p)]
+
+
+class Spec(ctypes.Structure):
+    _fields_ = [
+        ("name", ctypes.c_char_p),
+        ("basicsize", ctypes.c_int),
+        ("itemsize", ctypes.c_int),
+        ("flags", ctypes.c_uint),
+        ("slots", ctypes.POINTER(Slot)),
+    ]
+
+
+slots = (Slot * 3)(
+    Slot(59, ctypes.cast(tp_hash, ctypes.c_void_p)),  # Py_tp_hash
+    Slot(67, ctypes.cast(tp_richcompare, ctypes.c_void_p)),  # Py_tp_richcompare
+    Slot(0, None),
+)
+spec = Spec(b"broken.Key", object.__basicsize__, 0, 0, slots)
+ctypes.pythonapi.PyType_FromSpec.restype = ctypes.py_object
+ctypes.pythonapi.PyType_FromSpec.argtypes = [ctypes.POINTER(Spec)]
+Key = ctypes.pythonapi.PyType_FromSpec(ctypes.byref(spec))
+
+if not broken:
+    cache[Key()] = 1  # same hash: the lookup below has to compare
+try:
+    cache.get(Key())
+except SystemError:
+    print("SystemError")
+"""
+
+
+@pytest.mark.skipif(
+    platform.python_implementation() != "CPython",
+    reason="builds a C type with ctypes.pythonapi",
+)
+@pytest.mark.parametrize("slot", ["hash", "eq"])
+def test_c_key_error_without_exception(cache_cls, slot):
+    # Before the fix the process aborted, hence a child process.
+    done = subprocess.run(
+        [sys.executable, "-c", BROKEN_C_KEY, cache_cls.__name__, slot],
+        capture_output=True,
+        text=True,
+        timeout=60,
+        check=False,
+    )
+    assert done.stdout.strip() == "SystemError", (
+        done.stderr or f"exit code {done.returncode}"
+    )