diff --git a/src/internal/utils.rs b/src/internal/utils.rs index 8db7c39..a7984a4 100644 --- a/src/internal/utils.rs +++ b/src/internal/utils.rs @@ -16,13 +16,23 @@ pub unsafe fn pyobject_hash( ) -> pyo3::PyResult { 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) } diff --git a/tests/test_hashing.py b/tests/test_hashing.py index 7b6e613..1612290 100644 --- a/tests/test_hashing.py +++ b/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}" + )