Fix device reopen errors and failed-open handle ownership

src/methods.cpp

@@ -20,6 +20,13 @@
 #include <sstream>
 #include <string>
 
+// 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<PyObject, PyObjectDecref>;
+
 // 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<PyObject*>* 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<int __stdcall(void*, int*)> icsneoClosePort(lib, "icsneoClosePort");
+        ice::Function<void __stdcall(void*)> 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) {

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,15 +91,13 @@ 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]);
         PyObject* data = PyTuple_GetSlice(temp, 0, obj->msg.NumberBytesHeader);
         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;

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)

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; }