diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..8a77fb94 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -20,6 +20,13 @@ #include #include +// Own a Python reference while the GIL is held, including on early returns. +struct PyObjectDecref +{ + void operator()(PyObject* object) const { Py_XDECREF(object); } +}; +using PyObjectRef = std::unique_ptr; + // This class allows RAII of the python GIL. This is a C++ replacement of // Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS class PyAllowThreads @@ -794,12 +801,12 @@ bool _convertListOrTupleToArray(PyObject* obj, std::vector* results) PyObject* _getPythonModuleObject(const char* module_name, const char* module_object_name) { // Before we do anything, we need to grab the python s_device_settings ctype.Structure. - PyObject* module = PyImport_ImportModule(module_name); + PyObjectRef module(PyImport_ImportModule(module_name)); if (!module) { return set_ics_exception(exception_runtime_error(), "_getPythonModuleObject(): Failed to import module"); } // Grab the module Dictionary - PyObject* module_dict = PyModule_GetDict(module); + PyObject* module_dict = PyModule_GetDict(module.get()); if (!module_dict) { return set_ics_exception(exception_runtime_error(), "_getPythonModuleObject(): Failed to grab module dict from module"); @@ -823,13 +830,13 @@ PyObject* _getPythonModuleObject(const char* module_name, const char* module_obj int _isPythonModuleObject_IsInstance(PyObject* object, const char* module_name, const char* module_object_name) { // Before we do anything, we need to grab the python s_device_settings ctype.Structure. - PyObject* module = PyImport_ImportModule(module_name); + PyObjectRef module(PyImport_ImportModule(module_name)); if (!module) { set_ics_exception(exception_runtime_error(), "_isPythonModuleObjectInstanceOf(): Failed to import module"); return -1; } // Grab the module Dictionary - PyObject* module_dict = PyModule_GetDict(module); + PyObject* module_dict = PyModule_GetDict(module.get()); if (!module_dict) { set_ics_exception(exception_runtime_error(), "_isPythonModuleObjectInstanceOf(): Failed to grab module dict from module"); @@ -940,14 +947,14 @@ bool PyNeoDeviceEx_GetHandle(PyObject* object, void** handle) set_ics_exception(exception_runtime_error(), "Object is not of type PyNeoDeviceEx"); return false; } - PyObject* _handle = PyObject_GetAttrString(object, "_handle"); + PyObjectRef _handle(PyObject_GetAttrString(object, "_handle")); if (!_handle) { return false; } - if (!PyCapsule_CheckExact(_handle)) { + if (!PyCapsule_CheckExact(_handle.get())) { return true; } - void* ptr = PyCapsule_GetPointer(_handle, NULL); + void* ptr = PyCapsule_GetPointer(_handle.get(), NULL); if (!ptr) { return false; } @@ -965,21 +972,21 @@ bool PyNeoDeviceEx_SetHandle(PyObject* object, void* handle) set_ics_exception(exception_runtime_error(), "Object is not of type PyNeoDeviceEx"); return false; } - PyObject* _handle = PyObject_GetAttrString(object, "_handle"); + PyObjectRef _handle(PyObject_GetAttrString(object, "_handle")); if (!_handle) { return false; } - if (!PyCapsule_CheckExact(_handle) && handle) { - PyObject* capsule = PyCapsule_New(handle, NULL, __destroy_PyNeoDeviceEx_Handle); + if (!PyCapsule_CheckExact(_handle.get()) && handle) { + PyObjectRef capsule(PyCapsule_New(handle, NULL, __destroy_PyNeoDeviceEx_Handle)); if (!capsule) { return false; } - if (PyObject_SetAttrString(object, "_handle", capsule) != 0) { + if (PyObject_SetAttrString(object, "_handle", capsule.get()) != 0) { return false; } } else if (handle) { - if (!PyCapsule_SetPointer(_handle, handle)) { - return NULL; + if (PyCapsule_SetPointer(_handle.get(), handle) != 0) { + return false; } } else { if (PyObject_SetAttrString(object, "_handle", Py_None) != 0) { @@ -1296,6 +1303,13 @@ PyObject* meth_open_device(PyObject* self, PyObject* args, PyObject* keywords) if (!PyNeoDeviceEx_GetHandle(device, &handle)) { return NULL; } + if (handle) { + return set_ics_exception(exception_runtime_error(), "Device is already open; close it before reopening."); + } + // Resolve cleanup before opening so a failed Python handle assignment + // cannot leave us with a native handle we have no way to release. + ice::Function icsneoClosePort(lib, "icsneoClosePort"); + ice::Function icsneoFreeObject(lib, "icsneoFreeObject"); // Get the NeoDeviceEx from PyNeoDeviceEx Py_buffer buffer = {}; NeoDeviceEx* nde = NULL; @@ -1318,6 +1332,15 @@ PyObject* meth_open_device(PyObject* self, PyObject* args, PyObject* keywords) gil.restore(); PyBuffer_Release(&buffer); if (!PyNeoDeviceEx_SetHandle(device, handle)) { + PyObject *error_type, *error_value, *error_traceback; + PyErr_Fetch(&error_type, &error_value, &error_traceback); + if (handle) { + int error_count = 0; + auto cleanup_gil = PyAllowThreads(); + icsneoClosePort(handle, &error_count); + icsneoFreeObject(handle); + } + PyErr_Restore(error_type, error_value, error_traceback); return NULL; } if (device_need_ref_inc) { diff --git a/src/object_spy_message.cpp b/src/object_spy_message.cpp index 0c1c4a7a..ec337e21 100644 --- a/src/object_spy_message.cpp +++ b/src/object_spy_message.cpp @@ -64,11 +64,8 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name) #endif PyErr_Format(PyExc_TypeError, "attribute name must be string, not '%.200s'", attr_name->ob_type->tp_name); return NULL; - } else { - Py_INCREF(attr_name); } if (PyUnicode_CompareWithASCIIString(attr_name, "Data") == 0) { - Py_DECREF(attr_name); spy_message_object* obj = (spy_message_object*)o; PyObject* temp = Py_BuildValue("(i,i,i,i,i,i,i,i)", obj->msg.Data[0], @@ -83,7 +80,6 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name) Py_DECREF(temp); return data; } else if (PyUnicode_CompareWithASCIIString(attr_name, "AckBytes") == 0) { - Py_DECREF(attr_name); spy_message_object* obj = (spy_message_object*)o; return Py_BuildValue("(i,i,i,i,i,i,i,i)", obj->msg.AckBytes[0], @@ -95,7 +91,6 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name) obj->msg.AckBytes[6], obj->msg.AckBytes[7]); } else if (PyUnicode_CompareWithASCIIString(attr_name, "Header") == 0) { - Py_DECREF(attr_name); spy_message_j1850_object* obj = (spy_message_j1850_object*)o; PyObject* temp = Py_BuildValue("(i,i,i,i)", obj->msg.Header[0], obj->msg.Header[1], obj->msg.Header[2], obj->msg.Header[3]); @@ -103,7 +98,6 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name) Py_DECREF(temp); return data; } else if (PyUnicode_CompareWithASCIIString(attr_name, "ExtraDataPtr") == 0) { - Py_DECREF(attr_name); spy_message_j1850_object* obj = (spy_message_j1850_object*)o; unsigned char* ExtraDataPtr = (unsigned char*)obj->msg.ExtraDataPtr; bool extra_data_ptr_enabled = obj->msg.ExtraDataPtrEnabled != 0; diff --git a/tests/_reference_helpers.py b/tests/_reference_helpers.py new file mode 100644 index 00000000..7130404a --- /dev/null +++ b/tests/_reference_helpers.py @@ -0,0 +1,125 @@ +"""Run in a subprocess by test_reference_leaks.py with a mock native library.""" +import ctypes +import importlib +import os +import sys + +import ics +import pytest + + +@pytest.fixture(autouse=True, scope="module") +def mock_library(): + ics.override_library_name(os.environ["ICS_REFERENCE_MOCK"]) + + +@pytest.fixture +def device(): + device = ics.PyNeoDeviceEx() + device._auto_handle_close = False + return device + + +def assert_stable(target, action): + for _ in range(50): + action() + before = sys.getrefcount(target) + for _ in range(1000): + action() + assert sys.getrefcount(target) == before + + +def expect_error(action, error): + with pytest.raises(error): + action() + + +def assert_capsule_ownership(capsules, device_references=0): + new = ctypes.pythonapi.PyCapsule_New + new.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_void_p] + new.restype = ctypes.py_object + control = [new(0x1234, None, None)] + # Compare equally owned list elements. CPython versions can differ in the + # temporary references they use for local variables and getrefcount calls. + for index in range(len(capsules)): + assert sys.getrefcount(capsules[index]) == sys.getrefcount(control[0]) + device_references + + +def test_handle_creation_and_clear(device): + for _ in range(50): + assert ics.open_device(device) is device + capsules = [device._handle] + assert_capsule_ownership(capsules, device_references=1) + assert device._Handle == 0x1234 + assert ics.close_device(device) == 0 + assert device._handle is None + assert_capsule_ownership(capsules) + + +@pytest.mark.parametrize("kind", ["capsule", "noncapsule", "named_capsule"]) +def test_handle_lookup(device, kind): + if kind == "noncapsule": + handle = object() + else: + new = ctypes.pythonapi.PyCapsule_New + new.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_void_p] + new.restype = ctypes.py_object + handle = new(0x1234, b"wrong_name" if kind == "named_capsule" else None, None) + device._handle = handle + if kind == "named_capsule": + action = lambda: expect_error(lambda: ics.get_device_status(device), ValueError) + else: + action = lambda: ics.get_device_status(device) + assert_stable(handle, action) + + +@pytest.mark.parametrize("failure", ["missing", "raises"]) +def test_handle_attribute_failure(device, monkeypatch, failure): + if failure == "missing": + monkeypatch.delattr(ics.PyNeoDeviceEx, "_handle") + expect_error(lambda: ics.get_device_status(device), AttributeError) + else: + rejected = [] + + def reject(self, value): + rejected.append(value) + raise ValueError("reject handle assignment") + monkeypatch.setattr(ics.PyNeoDeviceEx, "_handle", property(lambda self: sentinel, reject)) + sentinel = object() + assert_stable(sentinel, lambda: expect_error(lambda: ics.open_device(device), ValueError)) + # A failing setter never stole the helper's newly created reference. + assert_capsule_ownership(rejected) + + +@pytest.mark.parametrize("helper", ["construct", "isinstance"]) +@pytest.mark.parametrize("case", ["success", "missing", "raises", "not_module"]) +def test_module_references(device, monkeypatch, helper, case): + name = "ics_device_status" if helper == "construct" else "st_cm_iso157652_rx_message" + module_name = "ics.structures." + name + module = importlib.import_module(module_name) + cls = getattr(module, name) + message = cls() + action = (lambda: ics.get_device_status(device)) if helper == "construct" else ( + lambda: ics.iso15765_receive_message(device, 0, message) + ) + if case == "missing": + monkeypatch.delattr(module, name) + elif case == "raises": + if helper == "construct": + def raises(): + raise ValueError("constructor failed") + replacement = raises + else: + class Meta(type): + def __instancecheck__(self, instance): + raise ValueError("instance check failed") + replacement = Meta("FailingClass", (), {}) + monkeypatch.setattr(module, name, replacement) + elif case == "not_module": + module = object() + monkeypatch.setitem(sys.modules, module_name, module) + if case != "success": + original_action = action + error = ValueError if helper == "isinstance" and case == "raises" else ics.RuntimeError + action = lambda: expect_error(original_action, error) + assert_stable(module, action) diff --git a/tests/reference_mock.c b/tests/reference_mock.c new file mode 100644 index 00000000..0e5ea088 --- /dev/null +++ b/tests/reference_mock.c @@ -0,0 +1,16 @@ +/* No hardware access or external runtime dependencies. */ +#ifdef _WIN32 +#define API __declspec(dllexport) __stdcall +#else +#define API +#endif +int API icsneoOpenDevice(void* device, void** handle, unsigned char* networks, + int config, int options, void* extra, unsigned long reserved) +{ + *handle = (void*)0x1234; + return 1; +} +int API icsneoClosePort(void* handle, int* errors) { *errors = 0; return 1; } +void API icsneoFreeObject(void* handle) {} +int API icsneoGetDeviceStatus(void* handle, void* status, void* size) { return 1; } +int API icsneoISO15765_ReceiveMessage(void* handle, unsigned int index, void* message) { return 1; } diff --git a/tests/test_device_handle_lifecycle.py b/tests/test_device_handle_lifecycle.py new file mode 100644 index 00000000..df5cfaa9 --- /dev/null +++ b/tests/test_device_handle_lifecycle.py @@ -0,0 +1,142 @@ +"""Hardware-free handle ownership tests, isolated from the process-wide DLL override.""" + +import os +from pathlib import Path +import shutil +import subprocess +import sys + +import pytest + + +@pytest.fixture(scope="module") +def handle_library(tmp_path_factory): + root = tmp_path_factory.mktemp("handle-library") + source = root / "mock.c" + source.write_text( + r""" +#ifdef _WIN32 +#define API __declspec(dllexport) +#define CALL __stdcall +#else +#define API __attribute__((visibility("default"))) +#define CALL +#endif +static int opens, closes, frees, fail_open, fail_close; +static char handles[16]; +API int CALL icsneoOpenDevice(void* device, void** handle, unsigned char* networks, + int config, int options, void* extra, unsigned long reserved) { + ++opens; + if (fail_open) return 0; + *handle = &handles[opens % 16]; + return 1; +} +API int CALL icsneoClosePort(void* handle, int* errors) { ++closes; *errors = 0; return !fail_close; } +API void CALL icsneoFreeObject(void* handle) { ++frees; } +API int counts(int which) { return which == 0 ? opens : which == 1 ? closes : frees; } +API void failures(int opening, int closing) { fail_open = opening; fail_close = closing; } +""" + ) + if sys.platform == "win32": + compiler = shutil.which("clang-cl") + linker = shutil.which("lld-link") + if not compiler or not linker: + pytest.skip("handle mock requires clang-cl and lld-link") + library = root / "mock.dll" + obj = root / "mock.obj" + bits = 64 if sys.maxsize > 2**32 else 32 + subprocess.run( + [compiler, f"-m{bits}", "/nologo", "/c", "/GS-", "/Zl", str(source), f"/Fo{obj}"], + check=True, timeout=60, + ) + command = [linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", str(obj)] + if bits == 32: + # ice loads undecorated names; x86 stdcall exports are decorated. + command.extend([ + "/export:icsneoOpenDevice=_icsneoOpenDevice@28", + "/export:icsneoClosePort=_icsneoClosePort@8", + "/export:icsneoFreeObject=_icsneoFreeObject@4", + ]) + subprocess.run(command, check=True, timeout=60) + else: + compiler = shutil.which("cc") + if not compiler: + pytest.skip("handle mock requires a C compiler") + library = root / ("mock.dylib" if sys.platform == "darwin" else "mock.so") + subprocess.run([compiler, "-shared", "-fPIC", str(source), "-o", str(library)], check=True, timeout=60) + return library + + +def run_case(library, code): + import ics + + env = os.environ.copy() + env["PYTHONPATH"] = str(Path(ics.__file__).resolve().parent.parent) + result = subprocess.run( + [sys.executable, "-c", """ +import ctypes +import ics +import sys +import pytest +ics.override_library_name(sys.argv[1]) +mock = ctypes.CDLL(sys.argv[1]) +mock.counts.argtypes = [ctypes.c_int] +mock.counts.restype = ctypes.c_int +mock.failures.argtypes = [ctypes.c_int, ctypes.c_int] +mock.failures.restype = None +device = ics.PyNeoDeviceEx() +device._auto_handle_close = False +def counts(): + return tuple(mock.counts(i) for i in range(3)) +""" + code, str(library)], + env=env, capture_output=True, text=True, timeout=30, + ) + assert result.returncode == 0, result.stdout + result.stderr + + +def test_reopen_rejected_without_native_open_and_close_allows_reopen(handle_library): + run_case(handle_library, """ +assert ics.open_device(device) is device +capsule = device._handle +handle = device._Handle +with pytest.raises(ics.RuntimeError, match="already open"): + ics.open_device(device) +assert counts() == (1, 0, 0) +assert device._handle is capsule and device._Handle == handle +assert ics.close_device(device) == 0 +assert device._handle is None +assert counts() == (1, 1, 1) +assert ics.open_device(device) is device +assert device._Handle != handle +assert ics.close_device(device) == 0 +assert ics.close_device(device) == 0 +assert counts() == (2, 2, 2) +""") + + +@pytest.mark.parametrize("fail_close", [0, 1]) +def test_failed_python_assignment_releases_new_handle(handle_library, fail_close): + run_case(handle_library, f"mock.failures(0, {fail_close})\n" + """ +def reject_handle(self, value): + raise MemoryError("injected handle assignment failure") +ics.PyNeoDeviceEx._handle = property(lambda self: None, reject_handle) +with pytest.raises(MemoryError, match="injected handle assignment failure"): + ics.open_device(device) +assert device._handle is None +assert counts() == (1, 1, 1) +""") + + +def test_native_open_failure_does_not_publish_or_close_handle(handle_library): + run_case(handle_library, r""" +mock.failures(1, 0) +with pytest.raises(ics.RuntimeError, match=r"icsneoOpenDevice\(\) Failed"): + ics.open_device(device) +assert device._handle is None +assert counts() == (1, 0, 0) +mock.failures(0, 0) +assert ics.open_device(device) is device +assert ics.close_device(device) == 0 +assert counts() == (2, 1, 1) +""") + diff --git a/tests/test_reference_leaks.py b/tests/test_reference_leaks.py new file mode 100644 index 00000000..23165728 --- /dev/null +++ b/tests/test_reference_leaks.py @@ -0,0 +1,71 @@ +"""Reference ownership regressions; native calls run only against a mock library.""" +import os +from pathlib import Path +import shutil +import struct +import subprocess +import sys + +import ics +import pytest + + +@pytest.mark.parametrize("message_type", [ics.SpyMessage, ics.SpyMessageJ1850]) +@pytest.mark.parametrize("attribute", ["StatusBitField", "Data", "AckBytes", "Header", "ExtraDataPtr", "missing_attribute"]) +def test_attribute_name_reference_count(message_type, attribute): + message = message_type() + name = "".join([attribute[:-1], attribute[-1:]]) + + def read(): + try: + getattr(message, name) + except AttributeError: + assert attribute == "missing_attribute" + + for _ in range(50): + read() + before = sys.getrefcount(name) + for _ in range(1000): + read() + assert sys.getrefcount(name) == before + + +def test_native_helper_reference_counts(tmp_path): + clang = shutil.which("clang") + if not clang: + pytest.skip("clang is required to build the hardware-free mock library") + source = Path(__file__).with_name("reference_mock.c") + library = tmp_path / ("reference_mock.dll" if sys.platform == "win32" else "reference_mock.so") + if sys.platform == "win32": + linker = shutil.which("lld-link") + if not linker: + pytest.skip("lld-link is required to link the hardware-free mock library") + target = "x86_64-pc-windows-msvc" if struct.calcsize("P") == 8 else "i686-pc-windows-msvc" + obj = tmp_path / "reference_mock.obj" + subprocess.run([clang, "-target", target, "-c", str(source), "-o", str(obj)], check=True) + exports = [] + if struct.calcsize("P") == 4: + # ice looks up undecorated names; i686 stdcall exports include stack sizes. + exports = [ + f"/export:{name}=_{name}@{size}" + for name, size in [ + ("icsneoOpenDevice", 28), + ("icsneoClosePort", 8), + ("icsneoFreeObject", 4), + ("icsneoGetDeviceStatus", 12), + ("icsneoISO15765_ReceiveMessage", 12), + ] + ] + subprocess.run( + [linker, "/dll", "/noentry", "/nodefaultlib", str(obj), f"/out:{library}", *exports], check=True + ) + else: + subprocess.run([clang, "-shared", "-fPIC", str(source), "-o", str(library)], check=True) + # Isolate the global library override and module monkeypatches from the suite. + env = os.environ.copy() + env["ICS_REFERENCE_MOCK"] = str(library) + result = subprocess.run( + [sys.executable, "-m", "pytest", "-q", str(Path(__file__).with_name("_reference_helpers.py"))], + env=env, capture_output=True, text=True, timeout=120, + ) + assert result.returncode == 0, result.stdout + result.stderr