From b3b0101ef6cdc2fe55db3cd2747b1c9eae09c632 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:06:32 -0400 Subject: [PATCH 1/8] fix: handle SpyMessage attribute deletion --- src/object_spy_message.cpp | 7 ++++++- tests/test_spy_message.py | 41 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/object_spy_message.cpp b/src/object_spy_message.cpp index 0c1c4a7a..4367623a 100644 --- a/src/object_spy_message.cpp +++ b/src/object_spy_message.cpp @@ -138,6 +138,11 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name) static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* value) { + // tp_setattro receives NULL for deletion. Let descriptors reject deletion + // before any custom setter inspects the value or changes message storage. + if (value == NULL) + return PyObject_GenericSetAttr(o, name, value); + spy_message_object* obj = (spy_message_object*)o; if (PyUnicode_CompareWithASCIIString(name, "Data") == 0) { Py_ssize_t length = _copy_byte_tuple(value, name, obj->msg.Data, sizeof(obj->msg.Data)); @@ -484,4 +489,4 @@ bool setup_spy_message_object(PyObject* module) Py_INCREF(&spy_message_j1850_object_type); PyModule_AddObject(module, SPY_MESSAGE_J1850_OBJECT_NAME, (PyObject*)&spy_message_j1850_object_type); return true; -} \ No newline at end of file +} diff --git a/tests/test_spy_message.py b/tests/test_spy_message.py index f25371da..dbe9bf75 100644 --- a/tests/test_spy_message.py +++ b/tests/test_spy_message.py @@ -1,7 +1,48 @@ +import os +import subprocess +import sys +import textwrap + import pytest import ics +@pytest.mark.parametrize("message_type", ["SpyMessage", "SpyMessageJ1850"]) +@pytest.mark.parametrize("attribute", ["Data", "AckBytes", "Header", "ExtraDataPtr", "Protocol", "ExtraDataPtrEnabled"]) +@pytest.mark.parametrize("initialized", [False, True]) +def test_attribute_deletion_is_safe(message_type, attribute, initialized): + # Isolate native crashes so every affected attribute/type is reported. + code = textwrap.dedent(f""" + import ics + + msg = ics.{message_type}() + if {initialized!r}: + msg.Protocol = ics.SPY_PROTOCOL_CANFD + msg.Data = (1, 2, 3) + msg.AckBytes = (4, 5) + msg.Header = (6, 7) + msg.ExtraDataPtr = (8, 9, 10) + fields = ("Data", "AckBytes", "Header", "ExtraDataPtr", "Protocol", + "ExtraDataPtrEnabled", "NumberBytesData", "NumberBytesHeader") + before = tuple(getattr(msg, field) for field in fields) + try: + delattr(msg, {attribute!r}) + except (AttributeError, TypeError): + pass + else: + raise AssertionError("deletion should be rejected") + assert tuple(getattr(msg, field) for field in fields) == before + # The message remains usable, including replacing and freeing its buffer. + msg.ExtraDataPtr = (11, 12) + assert msg.ExtraDataPtr == (11, 12) + del msg + """) + env = os.environ.copy() + env["PYTHONPATH"] = os.pathsep.join(str(path) for path in sys.path) + result = subprocess.run([sys.executable, "-c", code], env=env, capture_output=True, text=True, timeout=30) + assert result.returncode == 0, f"exit {result.returncode}\n{result.stdout}\n{result.stderr}" + + def test_data_roundtrip(): msg = ics.SpyMessage() msg.Data = (1, 2, 3, 4, 5, 6, 7, 8) From e56291f008009fc57362d3063bb986d3519915c8 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:32:21 -0400 Subject: [PATCH 2/8] fix: validate transmit message types Reject unsupported objects before reading native message fields or transmitting any part of a batch. Preserve both supported message layouts and cover validation with a hardware-free native stub. --- src/methods.cpp | 23 +++--- tests/test_transmit_messages.py | 121 ++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 10 deletions(-) create mode 100644 tests/test_transmit_messages.py diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..1b913270 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -1771,6 +1771,17 @@ PyObject* meth_transmit_messages(PyObject* self, PyObject* args) if (!PyNeoDeviceEx_GetHandle(obj, &handle)) { return NULL; } + // Validate the entire batch before taking native pointers or transmitting. + // A non-tuple argument represents a single message. + const Py_ssize_t message_count = PyTuple_CheckExact(temp) ? PyTuple_Size(temp) : 1; + for (Py_ssize_t i = 0; i < message_count; ++i) { + PyObject* message = PyTuple_CheckExact(temp) ? PyTuple_GetItem(temp, i) : temp; + if (!PySpyMessage_CheckExact(message) && !PySpyMessageJ1850_CheckExact(message)) { + return set_ics_exception(PyExc_TypeError, + "Message must be of type " MODULE_NAME "." SPY_MESSAGE_OBJECT_NAME " or " + MODULE_NAME "." SPY_MESSAGE_J1850_OBJECT_NAME); + } + } PyObject* tuple = temp; if (!PyTuple_CheckExact(temp)) { tuple = Py_BuildValue("(O)", temp); @@ -1791,20 +1802,12 @@ PyObject* meth_transmit_messages(PyObject* self, PyObject* args) ice::Function icsneoTxMessages(lib, "icsneoTxMessages"); const Py_ssize_t TUPLE_COUNT = PyTuple_Size(tuple); icsSpyMessage** msgs = new icsSpyMessage*[static_cast(TUPLE_COUNT)](); - for (int i = 0; i < TUPLE_COUNT; ++i) { + for (Py_ssize_t i = 0; i < TUPLE_COUNT; ++i) { spy_message_object* _obj = (spy_message_object*)PyTuple_GetItem(tuple, static_cast(i)); - if (!_obj) { - if (created_tuple) { - Py_XDECREF(tuple); - } - delete[] msgs; - return set_ics_exception(exception_runtime_error(), - "Tuple item must be of " MODULE_NAME "." SPY_MESSAGE_OBJECT_NAME); - } msgs[i] = &(_obj->msg); } auto gil = PyAllowThreads(); - for (int i = 0; i < TUPLE_COUNT; ++i) { + for (Py_ssize_t i = 0; i < TUPLE_COUNT; ++i) { if (!icsneoTxMessages(handle, msgs[i], (msgs[i]->NetworkID2 << 8) | msgs[i]->NetworkID, 1)) { gil.restore(); if (created_tuple) { diff --git a/tests/test_transmit_messages.py b/tests/test_transmit_messages.py new file mode 100644 index 00000000..6b498f9e --- /dev/null +++ b/tests/test_transmit_messages.py @@ -0,0 +1,121 @@ +"""Hardware-free transmit validation using a native call-counting stub.""" + +import os +from pathlib import Path +import shutil +import subprocess +import sys + +import pytest + + +@pytest.fixture(scope="module") +def transmit_library(tmp_path_factory): + directory = tmp_path_factory.mktemp("transmit-library") + source = directory / "mock.c" + source.write_text( + """ +#ifdef _WIN32 +#define API __declspec(dllexport) +#define CALL __stdcall +#else +#define API +#define CALL +#endif +static int calls; +static int network; +static int result = 1; +API int CALL icsneoTxMessages(void* handle, void* message, int net, int count) { + ++calls; + network = net; + return result; +} +API int review_calls(void) { return calls; } +API int review_network(void) { return network; } +API void review_result(int value) { result = value; } +""" + ) + if sys.platform == "win32": + compiler = shutil.which("clang-cl") + linker = shutil.which("lld-link") + if not compiler or not linker: + pytest.skip("Native transmit tests require clang-cl and lld-link") + library = directory / "mock.dll" + obj = directory / "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 + ) + command = [linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", str(obj)] + if bits == 32: + command.append("/export:icsneoTxMessages=_icsneoTxMessages@16") + subprocess.run(command, check=True) + else: + compiler = shutil.which("cc") + if not compiler: + pytest.skip("Native transmit tests require a C compiler") + library = directory / ("mock.dylib" if sys.platform == "darwin" else "mock.so") + subprocess.run([compiler, "-shared", "-fPIC", str(source), "-o", str(library)], check=True) + return library + + +def test_transmit_message_types(transmit_library): + # Isolate the process-global library override from other tests. No real + # device is opened and the mock never dereferences a message pointer. + script = r''' +import ctypes +import sys +import ics + +ics.override_library_name(sys.argv[1]) +library = ctypes.CDLL(sys.argv[1]) +device = ics.PyNeoDeviceEx() +device._auto_handle_close = False +invalid_device = ics.PyNeoDeviceEx() +invalid_device._auto_handle_close = False + +for invalid in (None, 42, object(), "message", [], {}, invalid_device): + for argument in (invalid, (invalid,), (ics.SpyMessage(), invalid), + (invalid, ics.SpyMessageJ1850()), + (ics.SpyMessage(), invalid, ics.SpyMessageJ1850())): + before = library.review_calls() + refs = sys.getrefcount(invalid) + try: + ics.transmit_messages(device, argument) + except TypeError as error: + assert "SpyMessage" in str(error) + else: + raise AssertionError(f"accepted invalid input: {argument!r}") + assert library.review_calls() == before, "partially transmitted invalid batch" + assert sys.getrefcount(invalid) == refs, "invalid input leaked a reference" + +standard = ics.SpyMessage() +j1850 = ics.SpyMessageJ1850() +standard.NetworkID = 0x34 +standard.NetworkID2 = 0x12 +j1850.NetworkID = 0x78 +j1850.NetworkID2 = 0x56 +for argument, count, network in ((standard, 1, 0x1234), (j1850, 1, 0x5678), + ((standard, j1850), 2, 0x5678), ((), 0, 0x5678)): + before = library.review_calls() + assert ics.transmit_messages(device, argument) is None + assert library.review_calls() == before + count + assert library.review_network() == network + +library.review_result(0) +before = library.review_calls() +try: + ics.transmit_messages(device, (standard, j1850)) +except ics.RuntimeError: + pass +else: + raise AssertionError("native transmit failure was ignored") +assert library.review_calls() == before + 1 +''' + # Pass the actual imported package location even when pytest was launched + # with an adjusted sys.path instead of an installed wheel. + import ics + + environment = os.environ.copy() + environment["PYTHONPATH"] = os.pathsep.join([str(Path(ics.__file__).resolve().parent.parent), *sys.path]) + subprocess.run([sys.executable, "-c", script, str(transmit_library)], env=environment, check=True) From 17e08afbb7e35d0c7a1ed594a78d8944679e95c5 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:33:01 -0400 Subject: [PATCH 3/8] fix: release helper and attribute references --- src/methods.cpp | 31 +++++---- src/object_spy_message.cpp | 6 -- tests/_reference_helpers.py | 115 ++++++++++++++++++++++++++++++++++ tests/reference_mock.c | 16 +++++ tests/test_reference_leaks.py | 56 +++++++++++++++++ 5 files changed, 206 insertions(+), 18 deletions(-) create mode 100644 tests/_reference_helpers.py create mode 100644 tests/reference_mock.c create mode 100644 tests/test_reference_leaks.py diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..b92fbec3 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,20 +972,20 @@ 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)) { + if (!PyCapsule_SetPointer(_handle.get(), handle)) { return NULL; } } else { 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..28f5aea5 --- /dev/null +++ b/tests/_reference_helpers.py @@ -0,0 +1,115 @@ +"""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 test_handle_creation_and_clear(device): + for _ in range(50): + assert ics.open_device(device) is device + capsule = device._handle + assert sys.getrefcount(capsule) == 3 # local, device, getrefcount argument + assert device._Handle == 0x1234 + assert ics.close_device(device) == 0 + assert device._handle is None + assert sys.getrefcount(capsule) == 2 + + +@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. + for capsule in rejected: + assert sys.getrefcount(capsule) == 3 # list, loop local, getrefcount + + +@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_reference_leaks.py b/tests/test_reference_leaks.py new file mode 100644 index 00000000..32a88baa --- /dev/null +++ b/tests/test_reference_leaks.py @@ -0,0 +1,56 @@ +"""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) + subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", str(obj), f"/out:{library}"], 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 From 78642ddc13b8c7fd840ea3c38c8814a2fdf023e5 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:35:44 -0400 Subject: [PATCH 4/8] test: export mock APIs on 32-bit Windows --- tests/test_reference_leaks.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/test_reference_leaks.py b/tests/test_reference_leaks.py index 32a88baa..23165728 100644 --- a/tests/test_reference_leaks.py +++ b/tests/test_reference_leaks.py @@ -43,7 +43,22 @@ def test_native_helper_reference_counts(tmp_path): 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) - subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", str(obj), f"/out:{library}"], 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. From 471d658a1da3b460010fac2d8e55533e61f58b3f Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:37:21 -0400 Subject: [PATCH 5/8] fix: bound message payload reads by capacity Track payload capacity independently of mutable wire lengths to prevent heap overreads. Validate length changes, reads, and native writes; preserve DLL-owned buffers and handle reentrant integer conversions safely. Fixes #240 --- include/object_spy_message.h | 6 + src/methods.cpp | 27 +++- src/object_spy_message.cpp | 94 +++++++++++-- tests/extra_data_capacity_mock.cpp | 45 +++++++ tests/test_extra_data_capacity.py | 165 +++++++++++++++++++++++ tests/test_extra_data_capacity_native.py | 92 +++++++++++++ 6 files changed, 418 insertions(+), 11 deletions(-) create mode 100644 tests/extra_data_capacity_mock.cpp create mode 100644 tests/test_extra_data_capacity.py create mode 100644 tests/test_extra_data_capacity_native.py diff --git a/include/object_spy_message.h b/include/object_spy_message.h index 44778a34..53aed674 100644 --- a/include/object_spy_message.h +++ b/include/object_spy_message.h @@ -39,6 +39,8 @@ typedef struct { PyObject_HEAD icsSpyMessage msg; bool noExtraDataPtrCleanup; + // Private bound, independent of writable protocol/length/ownership fields. + size_t extraDataCapacity; } spy_message_object; #pragma pack(pop) @@ -47,6 +49,7 @@ typedef struct { PyObject_HEAD icsSpyMessageJ1850 msg; bool noExtraDataPtrCleanup; + size_t extraDataCapacity; } spy_message_j1850_object; #pragma pack(pop) @@ -63,5 +66,8 @@ extern PyTypeObject spy_message_j1850_object_type; #define PySpyMessageJ1850_GetObject(obj) ((spy_message_j1850_object*)obj) bool setup_spy_message_object(PyObject* module); +size_t spy_message_extra_data_length(const icsSpyMessage& msg); +bool spy_message_validate_extra_data(const spy_message_object* obj); +void spy_message_record_received_extra_data(PyObject* obj); #endif // _OBJECT_SPY_MESSAGE_H_ diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..ae924059 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -1782,6 +1782,19 @@ PyObject* meth_transmit_messages(PyObject* self, PyObject* args) if (!PyTuple_CheckExact(tuple)) { return set_ics_exception(exception_argument_error(), "Second argument must be of tuple type!"); } + for (Py_ssize_t i = 0; i < PyTuple_Size(tuple); ++i) { + PyObject* item = PyTuple_GetItem(tuple, i); + if (!PySpyMessage_CheckExact(item) && !PySpyMessageJ1850_CheckExact(item)) { + if (created_tuple) + Py_DECREF(tuple); + return set_ics_exception(exception_runtime_error(), "Expected SpyMessage or SpyMessageJ1850"); + } + if (!spy_message_validate_extra_data((spy_message_object*)item)) { + if (created_tuple) + Py_DECREF(tuple); + return NULL; + } + } try { ice::Library* lib = dll_get_library(); if (!lib) { @@ -1901,6 +1914,7 @@ PyObject* meth_get_messages(PyObject* self, PyObject* args) // Looks like icsneo40 does its own memory management so don't delete when we dealloc msg->noExtraDataPtrCleanup = true; } + spy_message_record_received_extra_data(_obj); PyTuple_SetItem(tuple, i, _obj); } PyObject* result = Py_BuildValue("(O,i)", tuple, errors); @@ -2726,6 +2740,7 @@ PyObject* meth_coremini_read_tx_message(PyObject* self, PyObject* args) // Scrip } gil.restore(); } + spy_message_record_received_extra_data(msg); return msg; } catch (ice::Exception& ex) { return set_ics_exception(exception_runtime_error(), (char*)ex.what()); @@ -2777,7 +2792,7 @@ PyObject* meth_coremini_read_rx_message(PyObject* self, PyObject* args) // Scrip auto gil = PyAllowThreads(); if (!icsneoScriptReadRxMessage(handle, index, - &PySpyMessageJ1850_GetObject(msg_mask)->msg, + &PySpyMessageJ1850_GetObject(msg)->msg, &PySpyMessageJ1850_GetObject(msg_mask)->msg)) { gil.restore(); return set_ics_exception(exception_runtime_error(), "icsneoScriptReadRxMessage() Failed"); @@ -2804,6 +2819,8 @@ PyObject* meth_coremini_read_rx_message(PyObject* self, PyObject* args) // Scrip } gil.restore(); } + spy_message_record_received_extra_data(msg); + spy_message_record_received_extra_data(msg_mask); return Py_BuildValue("(O,O)", msg, msg_mask); } catch (ice::Exception& ex) { return set_ics_exception(exception_runtime_error(), (char*)ex.what()); @@ -2844,6 +2861,8 @@ PyObject* meth_coremini_write_tx_message(PyObject* self, PyObject* args) // icsn } msg = (void*)&PySpyMessage_GetObject(msg_obj)->msg; } + if (!spy_message_validate_extra_data((spy_message_object*)msg_obj)) + return NULL; try { ice::Library* lib = dll_get_library(); if (!lib) { @@ -2883,6 +2902,12 @@ PyObject* meth_coremini_write_rx_message(PyObject* self, PyObject* args) // icsn if (!PyNeoDeviceEx_GetHandle(obj, &handle)) { return NULL; } + if ((PySpyMessage_CheckExact(msg_obj) || PySpyMessageJ1850_CheckExact(msg_obj)) && + !spy_message_validate_extra_data((spy_message_object*)msg_obj)) + return NULL; + if ((PySpyMessage_CheckExact(msg_mask_obj) || PySpyMessageJ1850_CheckExact(msg_mask_obj)) && + !spy_message_validate_extra_data((spy_message_object*)msg_mask_obj)) + return NULL; void* msg = NULL; void* msg_mask = NULL; if (j1850) { diff --git a/src/object_spy_message.cpp b/src/object_spy_message.cpp index 0c1c4a7a..881d9426 100644 --- a/src/object_spy_message.cpp +++ b/src/object_spy_message.cpp @@ -1,11 +1,37 @@ #include "object_spy_message.h" +size_t spy_message_extra_data_length(const icsSpyMessage& msg) +{ + if (msg.Protocol == SPY_PROTOCOL_A2B || msg.Protocol == SPY_PROTOCOL_ETHERNET || + msg.Protocol == SPY_PROTOCOL_SPI || msg.Protocol == SPY_PROTOCOL_WBMS) + return (msg.NumberBytesHeader << 8) | msg.NumberBytesData; + return msg.NumberBytesData; +} + +bool spy_message_validate_extra_data(const spy_message_object* obj) +{ + if (obj->msg.ExtraDataPtr && spy_message_extra_data_length(obj->msg) > obj->extraDataCapacity) { + PyErr_SetString(PyExc_ValueError, "ExtraDataPtr length exceeds its buffer capacity"); + return false; + } + return true; +} + +void spy_message_record_received_extra_data(PyObject* o) +{ + spy_message_object* obj = (spy_message_object*)o; + // The DLL owns the pointer; its original reported length is the readable bound. + obj->extraDataCapacity = obj->msg.ExtraDataPtr ? spy_message_extra_data_length(obj->msg) : 0; + obj->noExtraDataPtrCleanup = true; +} + static int spy_message_object_alloc(spy_message_object* self, PyObject* args, PyObject* kwds) { (void)args; (void)kwds; memset(&self->msg, 0, sizeof(self->msg)); self->noExtraDataPtrCleanup = false; + self->extraDataCapacity = 0; return 0; } @@ -104,6 +130,8 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name) return data; } else if (PyUnicode_CompareWithASCIIString(attr_name, "ExtraDataPtr") == 0) { Py_DECREF(attr_name); + if (!spy_message_validate_extra_data((spy_message_object*)o)) + return NULL; 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; @@ -114,14 +142,7 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name) && obj->msg.ExtraDataPtr != NULL) { extra_data_ptr_enabled = true; } - int actual_size = 0; - // Some newer protocols are packing the length into NumberBytesHeader also so lets handle it here... - if (obj->msg.Protocol == SPY_PROTOCOL_A2B || obj->msg.Protocol == SPY_PROTOCOL_ETHERNET || - obj->msg.Protocol == SPY_PROTOCOL_SPI || obj->msg.Protocol == SPY_PROTOCOL_WBMS) { - actual_size = (obj->msg.NumberBytesHeader << 8) | obj->msg.NumberBytesData; - } else { - actual_size = obj->msg.NumberBytesData; - } + size_t actual_size = spy_message_extra_data_length(((spy_message_object*)o)->msg); if (extra_data_ptr_enabled && actual_size && obj->msg.ExtraDataPtr) { PyObject* tuple = PyTuple_New(actual_size); for (int i = 0; i < actual_size; ++i) { @@ -136,7 +157,7 @@ static PyObject* spy_message_object_getattr(PyObject* o, PyObject* attr_name) } } -static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* value) +static int spy_message_object_setattr_impl(PyObject* o, PyObject* name, PyObject* value) { spy_message_object* obj = (spy_message_object*)o; if (PyUnicode_CompareWithASCIIString(name, "Data") == 0) { @@ -201,6 +222,7 @@ static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* val if (obj->msg.ExtraDataPtr != NULL && !obj->noExtraDataPtrCleanup) delete[] (unsigned char*)obj->msg.ExtraDataPtr; obj->msg.ExtraDataPtr = buffer; + obj->extraDataCapacity = (size_t)length; if (packs_length) obj->msg.NumberBytesHeader = static_cast(length >> 8); obj->msg.NumberBytesData = static_cast(length & 0xFF); @@ -220,6 +242,7 @@ static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* val if (obj->msg.ExtraDataPtr != NULL) { delete[] (unsigned char*)obj->msg.ExtraDataPtr; obj->msg.ExtraDataPtr = NULL; + obj->extraDataCapacity = 0; } } else if (enabled != 0 && obj->msg.Protocol == SPY_PROTOCOL_ETHERNET) { // Ethernet always needs to be set to 0 @@ -231,6 +254,57 @@ static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* val } } +static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* value) +{ + // These assignments can reinterpret or enlarge the payload length. Roll back + // the complete native message, including Data/Header bytes, on failure. + bool scalar_length = PyUnicode_CompareWithASCIIString(name, "NumberBytesData") == 0 || + PyUnicode_CompareWithASCIIString(name, "NumberBytesHeader") == 0 || + PyUnicode_CompareWithASCIIString(name, "Protocol") == 0; + bool data = PyUnicode_CompareWithASCIIString(name, "Data") == 0; + bool header = PyUnicode_CompareWithASCIIString(name, "Header") == 0; + bool changes_length = scalar_length || data || header; + // __index__ can execute arbitrary Python, including replacing the payload. + // Finish all such conversions BEFORE snapshotting the message for rollback. + PyObject* normalized = NULL; + if (value && scalar_length) { + normalized = PyNumber_Index(value); + if (!normalized) + return -1; + long byte = PyLong_AsLong(normalized); + if ((byte == -1 && PyErr_Occurred()) || byte < 0 || byte > 255) { + Py_DECREF(normalized); + if (!PyErr_Occurred()) + PyErr_SetString(PyExc_ValueError, "Message length and protocol fields must be in range 0..255"); + return -1; + } + // Reject truncation rather than letting T_UBYTE issue a warning inside + // the transaction: a user warning hook can also replace the payload. + } else if (value && (data || header) && PyTuple_Check(value) && + PyTuple_Size(value) <= (data ? 8 : 4)) { + normalized = PyTuple_New(PyTuple_Size(value)); + if (!normalized) + return -1; + for (Py_ssize_t i = 0; i < PyTuple_Size(value); ++i) { + PyObject* byte = PyNumber_Index(PyTuple_GetItem(value, i)); + if (!byte) { + Py_DECREF(normalized); + return -1; + } + PyTuple_SET_ITEM(normalized, i, byte); + } + } + spy_message_object* obj = (spy_message_object*)o; + icsSpyMessage original = obj->msg; + int result = spy_message_object_setattr_impl(o, name, normalized ? normalized : value); + Py_XDECREF(normalized); + if (result == 0 && changes_length && !spy_message_validate_extra_data(obj)) { + obj->msg = original; + return -1; + } + return result; +} + static PyMemberDef spy_message_object_members[] = { { "StatusBitField", T_UINT, offsetof(spy_message_object, msg.StatusBitField), 0, "StatusBitField" }, { "StatusBitField2", T_UINT, offsetof(spy_message_object, msg.StatusBitField2), 0, "StatusBitField2" }, @@ -484,4 +558,4 @@ bool setup_spy_message_object(PyObject* module) Py_INCREF(&spy_message_j1850_object_type); PyModule_AddObject(module, SPY_MESSAGE_J1850_OBJECT_NAME, (PyObject*)&spy_message_j1850_object_type); return true; -} \ No newline at end of file +} diff --git a/tests/extra_data_capacity_mock.cpp b/tests/extra_data_capacity_mock.cpp new file mode 100644 index 00000000..57afceb8 --- /dev/null +++ b/tests/extra_data_capacity_mock.cpp @@ -0,0 +1,45 @@ +#include +#include "icsnVC40.h" + +#ifdef _WIN32 +#define API extern "C" __declspec(dllexport) int __stdcall +#else +#define API extern "C" int +#endif + +static unsigned char payload[300]; +static int calls; + +static void receive(icsSpyMessage* msg) +{ + memset(msg, 0, sizeof(*msg)); + for (int i = 0; i < 300; ++i) + payload[i] = (unsigned char)i; + msg->Protocol = SPY_PROTOCOL_ETHERNET; + msg->NumberBytesHeader = 1; + msg->NumberBytesData = 44; + msg->ExtraDataPtr = payload; +} + +API icsneoGetDLLVersion() { return 1; } +API icsneoWaitForRxMessagesWithTimeOut(void*, unsigned int) { return 1; } +API icsneoGetMessages(void*, icsSpyMessage* msg, int* count, int* errors) +{ + receive(msg); + *count = 1; + *errors = 0; + return 1; +} +API icsneoScriptReadTxMessage(void*, unsigned int, icsSpyMessage* msg) +{ + receive(msg); + return 1; +} +API icsneoScriptReadRxMessage(void*, unsigned int, icsSpyMessage* msg, icsSpyMessage* mask) +{ + receive(msg); + receive(mask); + return 1; +} +API icsneoTxMessages(void*, icsSpyMessage*, int, int) { ++calls; return 1; } +API capacity_mock_calls() { return calls; } diff --git a/tests/test_extra_data_capacity.py b/tests/test_extra_data_capacity.py new file mode 100644 index 00000000..a657880b --- /dev/null +++ b/tests/test_extra_data_capacity.py @@ -0,0 +1,165 @@ +"""Payload bounds must not depend on mutable wire-format length fields.""" +import ctypes + +import ics +import pytest + + +MESSAGE_TYPES = [ics.SpyMessage, ics.SpyMessageJ1850] +PACKED_PROTOCOLS = [ics.SPY_PROTOCOL_ETHERNET, ics.SPY_PROTOCOL_A2B, ics.SPY_PROTOCOL_SPI, ics.SPY_PROTOCOL_WBMS] + + +@pytest.mark.parametrize("message_type", MESSAGE_TYPES) +@pytest.mark.parametrize("protocol", [ics.SPY_PROTOCOL_CANFD] + PACKED_PROTOCOLS) +def test_payload_length_cannot_exceed_allocation(message_type, protocol): + msg = message_type() + msg.Protocol = protocol + msg.ExtraDataPtr = (17,) + with pytest.raises(ValueError, match="capacity"): + msg.NumberBytesData = 64 + assert msg.NumberBytesData == 1 + assert msg.ExtraDataPtr == (17,) + if protocol in PACKED_PROTOCOLS: + with pytest.raises(ValueError, match="capacity"): + msg.NumberBytesHeader = 1 + assert msg.NumberBytesHeader == 0 + + +@pytest.mark.parametrize("message_type", MESSAGE_TYPES) +@pytest.mark.parametrize("protocol", PACKED_PROTOCOLS) +def test_protocol_change_cannot_reinterpret_header_as_extra_capacity(message_type, protocol): + msg = message_type() + msg.Protocol = ics.SPY_PROTOCOL_CANFD + msg.NumberBytesHeader = 1 + msg.ExtraDataPtr = (17,) + with pytest.raises(ValueError, match="capacity"): + msg.Protocol = protocol + assert msg.Protocol == ics.SPY_PROTOCOL_CANFD + assert msg.ExtraDataPtrEnabled == 1 + assert msg.ExtraDataPtr == (17,) + + +@pytest.mark.parametrize("message_type", MESSAGE_TYPES) +def test_data_and_header_updates_are_atomic(message_type): + msg = message_type() + msg.Protocol = ics.SPY_PROTOCOL_SPI + msg.ExtraDataPtr = (17,) + with pytest.raises(ValueError, match="capacity"): + msg.Data = (1, 2) + assert msg.Data == (0,) + if message_type is ics.SpyMessageJ1850: + with pytest.raises(ValueError, match="capacity"): + msg.Header = (1,) + assert msg.Header == () + assert msg.ExtraDataPtr == (17,) + + +@pytest.mark.parametrize("message_type", MESSAGE_TYPES) +def test_capacity_survives_shrink_replacement_and_cleanup_flag(message_type): + msg = message_type() + msg.Protocol = ics.SPY_PROTOCOL_SPI + payload = tuple(i & 255 for i in range(300)) + msg.ExtraDataPtr = payload + msg.NumberBytesHeader = 0 + assert msg.ExtraDataPtr == payload[:44] + msg.NumberBytesHeader = 1 + assert msg.ExtraDataPtr == payload + msg.ExtraDataPtr = (2, 3) + msg.noExtraDataPtrCleanup = True + try: + with pytest.raises(ValueError, match="capacity"): + msg.NumberBytesData = 3 + finally: + msg.noExtraDataPtrCleanup = False + with pytest.raises(TypeError): + msg.ExtraDataPtr = (1, "bad") + assert msg.ExtraDataPtr == (2, 3) + msg.ExtraDataPtr = () + with pytest.raises(ValueError, match="capacity"): + msg.NumberBytesData = 1 + assert msg.ExtraDataPtr is None + + +@pytest.mark.parametrize("message_type", MESSAGE_TYPES) +def test_direct_descriptor_write_still_cannot_read_past_payload(message_type): + msg = message_type() + msg.ExtraDataPtr = (17,) + # Member descriptors bypass tp_setattro; the getter must defend itself too. + message_type.NumberBytesData.__set__(msg, 64) + with pytest.raises(ValueError, match="capacity"): + _ = msg.ExtraDataPtr + + +@pytest.mark.parametrize("attribute", ["NumberBytesData", "Data", "Header", "Protocol"]) +def test_reentrant_integer_conversion_preserves_replacement_payload(attribute): + msg = ics.SpyMessageJ1850() + msg.Protocol = ics.SPY_PROTOCOL_CANFD + msg.ExtraDataPtr = (17,) + + class ReplacePayload: + def __index__(self): + msg.ExtraDataPtr = (2, 3) + if attribute == "Header": + msg.Protocol = ics.SPY_PROTOCOL_SPI + if attribute == "Protocol": + msg.NumberBytesHeader = 1 + return ics.SPY_PROTOCOL_SPI + return 3 + + value = ReplacePayload() + if attribute == "Data": + value = (value, 4, 5) + elif attribute == "Header": + value = (value,) + with pytest.raises(ValueError, match="capacity"): + setattr(msg, attribute, value) + assert msg.ExtraDataPtr == (2, 3) + assert msg.NumberBytesData == 2 + + +@pytest.mark.parametrize("attribute", ["NumberBytesData", "NumberBytesHeader", "Protocol"]) +@pytest.mark.parametrize("value", [-1, 259]) +def test_out_of_range_scalar_does_not_call_warning_hook(attribute, value, monkeypatch): + import warnings + + msg = ics.SpyMessage() + msg.ExtraDataPtr = (17,) + calls = [] + + def replace_payload(*args, **kwargs): + calls.append(True) + msg.ExtraDataPtr = (2, 3) + + monkeypatch.setattr(warnings, "showwarning", replace_payload) + with warnings.catch_warnings(): + warnings.simplefilter("always") + with pytest.raises(ValueError, match="0..255"): + setattr(msg, attribute, value) + assert calls == [] + assert msg.ExtraDataPtr == (17,) + + +@pytest.mark.parametrize("message_type", MESSAGE_TYPES) +@pytest.mark.parametrize("operation", ["transmit", "tx_script", "rx_script", "rx_mask"]) +def test_invalid_payload_is_rejected_before_native_library_lookup(message_type, operation): + device = ics.PyNeoDeviceEx() + device._auto_handle_close = False + # No device or native library is opened. The sentinel capsule only passes + # handle extraction; validation must reject the message before native use. + capsule_new = ctypes.pythonapi.PyCapsule_New + capsule_new.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_void_p] + capsule_new.restype = ctypes.py_object + device._handle = capsule_new(1, None, None) + msg = message_type() + msg.ExtraDataPtr = (17,) + message_type.NumberBytesData.__set__(msg, 64) + j1850 = message_type is ics.SpyMessageJ1850 + with pytest.raises(ValueError, match="capacity"): + if operation == "transmit": + ics.transmit_messages(device, (message_type(), msg)) + elif operation == "tx_script": + ics.coremini_write_tx_message(device, 0, msg, j1850) + elif operation == "rx_script": + ics.coremini_write_rx_message(device, 0, msg, message_type(), j1850) + else: + ics.coremini_write_rx_message(device, 0, message_type(), msg, j1850) diff --git a/tests/test_extra_data_capacity_native.py b/tests/test_extra_data_capacity_native.py new file mode 100644 index 00000000..6b48b718 --- /dev/null +++ b/tests/test_extra_data_capacity_native.py @@ -0,0 +1,92 @@ +"""Exercise DLL-owned payloads in a subprocess; never load a hardware library.""" +import os +from pathlib import Path +import subprocess +import sys + +import pytest + + +def test_received_payload_capacity_and_native_transmit(tmp_path): + # setuptools supplies portable compiler discovery, including MSVC. This + # integration test needs the same native toolchain as the extension build. + pytest.importorskip("setuptools") + from setuptools._distutils.ccompiler import new_compiler + from setuptools._distutils.sysconfig import customize_compiler + + compiler = new_compiler() + customize_compiler(compiler) + root = Path(__file__).resolve().parents[1] + objects = compiler.compile( + [str(root / "tests" / "extra_data_capacity_mock.cpp")], + output_dir=str(tmp_path), + include_dirs=[str(root / "include" / "ics")], + macros=[("EXTERNAL_PROJECT", "1")], + ) + library = tmp_path / ("capacity_mock.dll" if os.name == "nt" else "capacity_mock.so") + extra_args = [] + if os.name == "nt" and sys.maxsize <= 2**32: + # stdcall exports are decorated on x86, but the runtime resolves the + # vendor API by its undecorated names. + exports = {"icsneoGetDLLVersion": 0, "icsneoWaitForRxMessagesWithTimeOut": 8, + "icsneoGetMessages": 16, "icsneoScriptReadTxMessage": 12, + "icsneoScriptReadRxMessage": 16, "icsneoTxMessages": 16, "capacity_mock_calls": 0} + extra_args = [f"/EXPORT:{name}=_{name}@{size}" for name, size in exports.items()] + compiler.link_shared_object(objects, str(library), target_lang="c++", extra_postargs=extra_args) + script = r''' +import ctypes +import gc +import sys +import ics + +ics.override_library_name(sys.argv[1]) +dll = (ctypes.WinDLL if sys.platform == "win32" else ctypes.CDLL)(sys.argv[1]) +device = ics.PyNeoDeviceEx() +device._auto_handle_close = False +payload = tuple(i & 255 for i in range(300)) +for j1850 in (False, True): + for source in ("receive", "script", "rx_message", "rx_mask"): + if source == "receive": + messages, errors = ics.get_messages(device, j1850, 0) + assert errors == 0 + msg = messages[0] + elif source == "script": + msg = ics.coremini_read_tx_message(device, 0, j1850) + else: + pair = ics.coremini_read_rx_message(device, 0, j1850) + assert all(m.ExtraDataPtr == payload for m in pair) + msg = pair[source == "rx_mask"] + assert msg.ExtraDataPtr == payload + assert msg.noExtraDataPtrCleanup + msg.NumberBytesHeader = 0 + assert msg.ExtraDataPtr == payload[:44] + msg.NumberBytesHeader = 1 + try: + msg.NumberBytesData = 45 + except ValueError: + pass + else: + raise AssertionError("received capacity was enlarged") + before = dll.capacity_mock_calls() + ics.transmit_messages(device, msg) + assert dll.capacity_mock_calls() == before + 1 + type(msg).NumberBytesHeader.__set__(msg, 2) + for operation in (lambda: msg.ExtraDataPtr, lambda: ics.transmit_messages(device, msg)): + try: + operation() + except ValueError: + pass + else: + raise AssertionError("descriptor bypass exposed native payload") + assert dll.capacity_mock_calls() == before + 1 + # Replacement must not delete the DLL's static buffer, and must set a + # new bound independent of the received length. + msg.ExtraDataPtr = (9,) + assert msg.ExtraDataPtr == (9,) + assert not msg.noExtraDataPtrCleanup + del msg + gc.collect() +print("mock receive, script read, replacement and transmit bounds passed") +''' + result = subprocess.run([sys.executable, "-c", script, str(library)], text=True, capture_output=True) + assert result.returncode == 0, result.stdout + result.stderr From f0a25f9f8f03316476a3706e812c2fbf23a89561 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:44:32 -0400 Subject: [PATCH 6/8] test: compare capsule ownership portably --- tests/_reference_helpers.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/_reference_helpers.py b/tests/_reference_helpers.py index 28f5aea5..7130404a 100644 --- a/tests/_reference_helpers.py +++ b/tests/_reference_helpers.py @@ -34,15 +34,26 @@ def expect_error(action, 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 - capsule = device._handle - assert sys.getrefcount(capsule) == 3 # local, device, getrefcount argument + 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 sys.getrefcount(capsule) == 2 + assert_capsule_ownership(capsules) @pytest.mark.parametrize("kind", ["capsule", "noncapsule", "named_capsule"]) @@ -77,8 +88,7 @@ def reject(self, value): 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. - for capsule in rejected: - assert sys.getrefcount(capsule) == 3 # list, loop local, getrefcount + assert_capsule_ownership(rejected) @pytest.mark.parametrize("helper", ["construct", "isinstance"]) From 1d4a2c028100344c8a6bbdaddb83f42ad07f6283 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:47:42 -0400 Subject: [PATCH 7/8] fix: exclude subclass callbacks from rollback Write normalized native scalar fields directly and dispatch deletion before snapshots. Restore state before constructing errors so callback-driven payload replacement cannot resurrect freed pointers. --- src/object_spy_message.cpp | 36 ++++++++++++++------ tests/test_extra_data_capacity.py | 56 +++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 10 deletions(-) diff --git a/src/object_spy_message.cpp b/src/object_spy_message.cpp index 881d9426..c63d6fcc 100644 --- a/src/object_spy_message.cpp +++ b/src/object_spy_message.cpp @@ -177,14 +177,6 @@ static int spy_message_object_setattr_impl(PyObject* o, PyObject* name, PyObject return -1; obj->msg.NumberBytesHeader = static_cast(length); return 0; - } else if (PyUnicode_CompareWithASCIIString(name, "Protocol") == 0) { - // Ethernet behavior is backward to CAN and will crash if enabled. - long protocol = PyLong_AsLong(value); - if (protocol == -1 && PyErr_Occurred()) - PyErr_Clear(); // let PyObject_GenericSetAttr report the type error - else if (protocol == SPY_PROTOCOL_ETHERNET) - obj->msg.ExtraDataPtrEnabled = 0; - return PyObject_GenericSetAttr(o, name, value); } else if (PyUnicode_CompareWithASCIIString(name, "ExtraDataPtr") == 0) { if (!PyTuple_Check(value)) { PyErr_Format(PyExc_AttributeError, @@ -256,6 +248,10 @@ static int spy_message_object_setattr_impl(PyObject* o, PyObject* name, PyObject static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* value) { + // Subclass deletion descriptors may execute Python. Never include them in + // a native-message rollback transaction. + if (!value) + return PyObject_GenericSetAttr(o, name, value); // These assignments can reinterpret or enlarge the payload length. Roll back // the complete native message, including Data/Header bytes, on failure. bool scalar_length = PyUnicode_CompareWithASCIIString(name, "NumberBytesData") == 0 || @@ -296,10 +292,30 @@ static int spy_message_object_setattr(PyObject* o, PyObject* name, PyObject* val } spy_message_object* obj = (spy_message_object*)o; icsSpyMessage original = obj->msg; - int result = spy_message_object_setattr_impl(o, name, normalized ? normalized : value); + int result = 0; + if (scalar_length) { + // These native fields, like Data/Header, must not dispatch subclass + // descriptors inside the transaction: they can replace/free payloads. + unsigned char byte = (unsigned char)PyLong_AsLong(normalized); + if (PyUnicode_CompareWithASCIIString(name, "NumberBytesData") == 0) + obj->msg.NumberBytesData = byte; + else if (PyUnicode_CompareWithASCIIString(name, "NumberBytesHeader") == 0) + obj->msg.NumberBytesHeader = byte; + else { + obj->msg.Protocol = byte; + if (byte == SPY_PROTOCOL_ETHERNET) + obj->msg.ExtraDataPtrEnabled = 0; + } + } else { + result = spy_message_object_setattr_impl(o, name, normalized ? normalized : value); + } Py_XDECREF(normalized); - if (result == 0 && changes_length && !spy_message_validate_extra_data(obj)) { + if (result == 0 && changes_length && obj->msg.ExtraDataPtr && + spy_message_extra_data_length(obj->msg) > obj->extraDataCapacity) { + // Restore before allocating the exception: cyclic-GC finalizers may + // execute Python during allocation and must see the committed state. obj->msg = original; + PyErr_SetString(PyExc_ValueError, "ExtraDataPtr length exceeds its buffer capacity"); return -1; } return result; diff --git a/tests/test_extra_data_capacity.py b/tests/test_extra_data_capacity.py index a657880b..1a483a96 100644 --- a/tests/test_extra_data_capacity.py +++ b/tests/test_extra_data_capacity.py @@ -1,5 +1,8 @@ """Payload bounds must not depend on mutable wire-format length fields.""" import ctypes +import os +import subprocess +import sys import ics import pytest @@ -9,6 +12,59 @@ PACKED_PROTOCOLS = [ics.SPY_PROTOCOL_ETHERNET, ics.SPY_PROTOCOL_A2B, ics.SPY_PROTOCOL_SPI, ics.SPY_PROTOCOL_WBMS] +@pytest.mark.parametrize("message_type", MESSAGE_TYPES) +@pytest.mark.parametrize("attribute", ["NumberBytesData", "NumberBytesHeader", "Protocol"]) +def test_subclass_descriptors_cannot_replace_payload_during_rollback(message_type, attribute): + script = r''' +import ics +import sys + +base = getattr(ics, sys.argv[1]) +attribute = sys.argv[2] +calls = [] +def replace_payload(self, value): + calls.append(True) + self.ExtraDataPtr = (2, 3) + getattr(base, attribute).__set__(self, value) +def delete_payload(self): + self.ExtraDataPtr = (2, 3) + # A deletion callback may leave inconsistent fields; the getter must reject + # them, but rollback must never resurrect the freed original payload. + base.NumberBytesData.__set__(self, 3) +message_type = type("Message", (base,), {attribute: property(fset=replace_payload, fdel=delete_payload)}) +msg = message_type() +base.Protocol.__set__(msg, ics.SPY_PROTOCOL_SPI) +if attribute == "Protocol": + base.Protocol.__set__(msg, ics.SPY_PROTOCOL_CANFD) + base.NumberBytesHeader.__set__(msg, 1) +msg.ExtraDataPtr = (17,) +value = {"NumberBytesData": 3, "NumberBytesHeader": 1, "Protocol": ics.SPY_PROTOCOL_ETHERNET}[attribute] +try: + setattr(msg, attribute, value) +except ValueError: + pass +else: + raise AssertionError("unsafe native field accepted") +assert calls == [] +assert msg.ExtraDataPtr == (17,) +delattr(msg, attribute) +try: + msg.ExtraDataPtr +except ValueError: + pass +else: + raise AssertionError("inconsistent deletion result accepted") +base.NumberBytesData.__set__(msg, 2) +assert msg.ExtraDataPtr == (2, 3) +del msg +''' + env = os.environ.copy() + env["PYTHONPATH"] = os.pathsep.join(str(path) for path in sys.path) + result = subprocess.run([sys.executable, "-c", script, message_type.__name__, attribute], + env=env, capture_output=True, text=True, timeout=30) + assert result.returncode == 0, result.stdout + result.stderr + + @pytest.mark.parametrize("message_type", MESSAGE_TYPES) @pytest.mark.parametrize("protocol", [ics.SPY_PROTOCOL_CANFD] + PACKED_PROTOCOLS) def test_payload_length_cannot_exceed_allocation(message_type, protocol): From 599f264eb4f5b4f3cbc08a735b0b0bf46af442ef Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Wed, 16 Sep 2026 10:58:27 -0400 Subject: [PATCH 8/8] test: isolate transmit input ownership checks Shared None counts include device handle leaks and interpreter bookkeeping. Measure private rejected inputs, batch containers, and valid neighbors over repeated calls while retaining the full type-rejection matrix. --- tests/test_transmit_messages.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/tests/test_transmit_messages.py b/tests/test_transmit_messages.py index 6b498f9e..4af99c74 100644 --- a/tests/test_transmit_messages.py +++ b/tests/test_transmit_messages.py @@ -79,7 +79,6 @@ def test_transmit_message_types(transmit_library): (invalid, ics.SpyMessageJ1850()), (ics.SpyMessage(), invalid, ics.SpyMessageJ1850())): before = library.review_calls() - refs = sys.getrefcount(invalid) try: ics.transmit_messages(device, argument) except TypeError as error: @@ -87,7 +86,27 @@ def test_transmit_message_types(transmit_library): else: raise AssertionError(f"accepted invalid input: {argument!r}") assert library.review_calls() == before, "partially transmitted invalid batch" - assert sys.getrefcount(invalid) == refs, "invalid input leaked a reference" + +# Measure ownership separately with private objects. Shared singleton counts +# include unrelated interpreter activity and device._handle lookups (#243). +# Check the rejected scalar, batch container, and valid neighbors so neither +# a direct reference leak nor a retained temporary tuple can go unnoticed. +sentinel = object() +neighbors = (ics.SpyMessage(), ics.SpyMessageJ1850()) +for argument in (sentinel, (sentinel,), (neighbors[0], sentinel), + (sentinel, neighbors[1]), (*neighbors, sentinel)): + tracked = (sentinel, argument, *neighbors) + refs = tuple(sys.getrefcount(value) for value in tracked) + before = library.review_calls() + for _ in range(100): + try: + ics.transmit_messages(device, argument) + except TypeError: + pass + else: + raise AssertionError("accepted invalid owned input") + assert library.review_calls() == before, "partially transmitted invalid batch" + assert tuple(sys.getrefcount(value) for value in tracked) == refs, "rejected input retained references" standard = ics.SpyMessage() j1850 = ics.SpyMessageJ1850()