From 2d0eb3589d78cac4bc111f8146225bf7cea86dab Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:03:52 -0400 Subject: [PATCH 01/21] fix: scope Linux dependency build cleanup --- build_libicsneo.py | 9 ++-- tests/test_build_libicsneo.py | 80 +++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 tests/test_build_libicsneo.py diff --git a/build_libicsneo.py b/build_libicsneo.py index 93d7b16a..1ef6907f 100644 --- a/build_libicsneo.py +++ b/build_libicsneo.py @@ -30,7 +30,7 @@ # icspb bootstraps protobuf from source at configure time. Keep the # bootstrap OUTSIDE the libicsneo build dir so it survives the per-python -# `git clean` in _build_libicsneo_linux and is reused across all +# build-directory cleanup in _build_libicsneo_linux and is reused across all # cibuildwheel builds in a job (it self-partitions by -, # so sharing one root across archs is safe). Building protobuf once per # arch instead of once per python matters most under QEMU aarch64. @@ -125,8 +125,11 @@ def _cmake_ninja_args(): def _build_libicsneo_linux(): print("Cleaning libicsneo...") - subprocess.check_output(["git", "clean", "-xdf"], cwd="libicsneo") - subprocess.check_output(["mkdir", "-p", "libicsneo/build"]) + # Source archives have no parent Git checkout. Reset only the CMake + # build tree, preserving the source checkout and protobuf bootstrap. + if os.path.exists(LIBICSNEO_BUILD): + shutil.rmtree(LIBICSNEO_BUILD) + os.makedirs(LIBICSNEO_BUILD, exist_ok=True) print("cmake libicsneo...") subprocess.check_output( diff --git a/tests/test_build_libicsneo.py b/tests/test_build_libicsneo.py new file mode 100644 index 00000000..d4cf142c --- /dev/null +++ b/tests/test_build_libicsneo.py @@ -0,0 +1,80 @@ +"""Build orchestration checks without downloads, compilers, or hardware.""" + +import importlib.util +import os +from pathlib import Path +import shutil +import subprocess +import tempfile +import unittest +from unittest import mock + + +class TestLinuxBuild(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name).resolve() + # Model an unpacked source archive: the build script has no parent .git. + script = self.root / "build_libicsneo.py" + shutil.copyfile(Path(__file__).resolve().parents[1] / script.name, script) + previous = Path.cwd() + os.chdir(self.root) + self.addCleanup(os.chdir, previous) + spec = importlib.util.spec_from_file_location("archive_build", script) + self.module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(self.module) + self.build = Path(self.module.LIBICSNEO_BUILD) + self.source = Path(self.module.LIBICSNEO_SOURCE) + self.source.mkdir(parents=True) + subprocess.run(["git", "init", "--quiet", str(self.source)], check=True) + self.real_check_output = subprocess.check_output + + def run_build(self, stale=None): + commands = [] + + def command(args, **kwargs): + if args[0] != "cmake": + return self.real_check_output(args, **kwargs) + self.assertTrue(self.build.is_dir()) + if stale is not None: + self.assertFalse(stale.exists()) + commands.append(args) + return b"" + + with mock.patch.object(self.module.subprocess, "check_output", side_effect=command): + self.module._build_libicsneo_linux() + self.assertEqual(len(commands), 2) + self.assertEqual(Path(commands[0][commands[0].index("-S") + 1]), self.source) + self.assertEqual(commands[0][commands[0].index("-B") + 1], self.module.LIBICSNEO_BUILD) + self.assertEqual(commands[1][1:3], ["--build", self.module.LIBICSNEO_BUILD]) + self.assertFalse((self.root / "libicsneo" / "build").exists()) + + def test_fresh_archive_build(self): + self.run_build() + + def test_rebuild_cleans_only_build_directory(self): + self.build.mkdir() + stale = self.build / "CMakeCache.txt" + stale.write_text("stale build environment") + preserved = [ + self.source / "untracked-source.txt", + Path(self.module.LIBICSNEO_INSTALL) / "installed.txt", + Path(self.module.ICSPB_BOOTSTRAP_DIR) / "protobuf.txt", + self.root / "libicsneo" / "other-version" / "keep.txt", + ] + for path in preserved: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("keep") + self.run_build(stale) + for path in preserved: + self.assertEqual(path.read_text(), "keep") + self.assertTrue((self.source / ".git").is_dir()) + + def test_cleanup_failure_stops_before_cmake(self): + self.build.mkdir() + with mock.patch.object(self.module.shutil, "rmtree", side_effect=PermissionError("busy")): + with mock.patch.object(self.module.subprocess, "check_output") as command: + with self.assertRaises(PermissionError): + self.module._build_libicsneo_linux() + command.assert_not_called() From 1084535dfbb0884c5d94bc3c09dbf1152432401d Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:04:42 -0400 Subject: [PATCH 02/21] fix: include serial in device equality --- src/ics/py_neo_device_ex.py | 5 +++- tests/test_neo_device_ex.py | 59 +++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/src/ics/py_neo_device_ex.py b/src/ics/py_neo_device_ex.py index e21c0270..59214417 100644 --- a/src/ics/py_neo_device_ex.py +++ b/src/ics/py_neo_device_ex.py @@ -26,8 +26,11 @@ def __repr__(self): return f"" def __eq__(self, other) -> bool: + if not isinstance(other, PyNeoDeviceEx): + return NotImplemented return \ self.DeviceType == other.DeviceType and \ + self.SerialNumber == other.SerialNumber and \ self.Handle == other.Handle and \ self.NumberOfClients == other.NumberOfClients and \ self.MaxAllowedClients == other.MaxAllowedClients and \ @@ -514,4 +517,4 @@ def request_set_neovi_miscio(self, *args, **kwargs): def get_firmware_variant(self, *args, **kwargs): "see ics.get_firmware_variant for details on arguments." - return ics.get_firmware_variant(self, *args, **kwargs) \ No newline at end of file + return ics.get_firmware_variant(self, *args, **kwargs) diff --git a/tests/test_neo_device_ex.py b/tests/test_neo_device_ex.py index bd969250..c08815e5 100644 --- a/tests/test_neo_device_ex.py +++ b/tests/test_neo_device_ex.py @@ -39,5 +39,64 @@ def test_serial_number_out_of_range_raises(): _make_device(ics.MAX_SERIAL + 1).serial_number +@pytest.mark.parametrize("serial", [100, ics.MAX_SERIAL]) +def test_equal_device_snapshots(serial): + first, second = _make_device(serial), _make_device(serial) + assert first is not second + assert first == second + assert second == first + assert not (first != second) + assert second in [first] + + +@pytest.mark.parametrize("serials", [(100, 200), (0x7FFFFFFF, 0x80000000)]) +def test_different_serials_are_distinct_devices(serials): + first, second = map(_make_device, serials) + assert first != second + assert second != first + assert not (first == second) + assert second not in [first] + devices = [first] + if second not in devices: + devices.append(second) + assert len(devices) == 2 + + +@pytest.mark.parametrize("field", ["DeviceType", "Handle", "NumberOfClients", "MaxAllowedClients"]) +def test_device_snapshot_fields_still_affect_equality(field): + first, second = _make_device(100), _make_device(100) + setattr(second.neoDevice, field, 1) + assert first != second + + +@pytest.mark.parametrize("field", [ + "FirmwareMajor", "FirmwareMinor", "Status", "Options", "pAvailWIFINetwork", + "isEthernetDevice", "hardwareRev", "revReserved", "tcpPort", +]) +def test_extended_snapshot_fields_still_affect_equality(field): + first, second = _make_device(100), _make_device(100) + setattr(second, field, 1) + assert first != second + + +@pytest.mark.parametrize("other", [None, object(), 100, "100"]) +def test_unrelated_objects_compare_unequal(other): + device = _make_device(100) + assert device.__eq__(other) is NotImplemented + assert not (device == other) + assert not (other == device) + assert device != other + assert other != device + + +def test_equality_allows_reflected_comparison(): + class AcceptsDevice: + def __eq__(self, other): + return isinstance(other, ics.PyNeoDeviceEx) + + device = _make_device(100) + assert device == AcceptsDevice() + + if __name__ == "__main__": pytest.main(args=[__file__, "--verbose", "-s"]) From b3b0101ef6cdc2fe55db3cd2747b1c9eae09c632 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:06:32 -0400 Subject: [PATCH 03/21] 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 f95804adfc115d54d62d26da7b5518fe63a97f9c Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:32:07 -0400 Subject: [PATCH 04/21] fix: honor device settings type override --- src/methods.cpp | 7 ++ tests/test_device_settings.py | 128 ++++++++++++++++++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 tests/test_device_settings.py diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..1d0dfce9 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -2200,6 +2200,11 @@ PyObject* meth_get_device_settings(PyObject* self, PyObject* args) if (!PyArg_ParseTuple(args, arg_parse("O|lI:", __FUNCTION__), &obj, &device_type_override, &vnet_slot_arg)) { return NULL; } + if (device_type_override != -1 && + (device_type_override < 0 || device_type_override >= DeviceSettingsTypeMax)) { + PyErr_SetString(PyExc_ValueError, "device_type must be -1 or a valid EDeviceSettingsType"); + return NULL; + } EPlasmaIonVnetChannel_t vnet_slot = static_cast(vnet_slot_arg); // Before we do anything, we need to grab the python s_device_settings ctype.Structure. @@ -2243,6 +2248,8 @@ PyObject* meth_get_device_settings(PyObject* self, PyObject* args) Py_DECREF(settings); return set_ics_exception(exception_runtime_error(), "icsneoGetDeviceSettingsType() Failed"); } + } else { + *setting_type = static_cast(device_type_override); } // int _stdcall icsneoGetDeviceSettings(void* hObject, SDeviceSettings* pSettings, int iNumBytes, // EPlasmaIonVnetChannel_t vnetSlot) diff --git a/tests/test_device_settings.py b/tests/test_device_settings.py new file mode 100644 index 00000000..e2978cb7 --- /dev/null +++ b/tests/test_device_settings.py @@ -0,0 +1,128 @@ +"""Hardware-free checks of the settings type passed across the native ABI.""" + +import os +from pathlib import Path +import shutil +import subprocess +import sys + +import pytest + + +@pytest.fixture(scope="module") +def settings_library(tmp_path_factory): + directory = tmp_path_factory.mktemp("settings-library") + source = directory / "settings.c" + source.write_text(r""" +#ifdef _WIN32 +#define API __declspec(dllexport) +#define CALL __stdcall +#else +#define API +#define CALL +#endif +static int received = -99, discoveries, requests, slot, size; +API int CALL icsneoGetDeviceSettingsType(void* handle, int vnet, int* type) { + ++discoveries; + slot = vnet; + *type = 27; + return vnet != 98; +} +API int CALL icsneoGetDeviceSettings(void* handle, int* settings, int bytes, int vnet) { + ++requests; + received = *settings; + slot = vnet; + size = bytes; + return vnet != 99; +} +API int observed(int field) { + switch (field) { + case 0: return received; + case 1: return discoveries; + case 2: return requests; + case 3: return slot; + default: return size; + } +} +""") + if sys.platform == "win32": + compiler = shutil.which("clang-cl") + linker = shutil.which("lld-link") + if not compiler or not linker: + pytest.skip("LLVM clang-cl and lld-link are required for the mock DLL") + library = directory / "settings.dll" + obj = directory / "settings.obj" + target = "i686" if sys.maxsize <= 2**32 else "x86_64" + subprocess.run([compiler, f"--target={target}-pc-windows-msvc", "/nologo", "/c", "/GS-", "/Zl", + str(source), f"/Fo{obj}"], check=True, capture_output=True) + subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", str(obj)], + check=True, capture_output=True) + else: + compiler = shutil.which("cc") + if not compiler: + pytest.skip("A C compiler is required for the mock library") + library = directory / ("settings.dylib" if sys.platform == "darwin" else "settings.so") + subprocess.run([compiler, "-dynamiclib" if sys.platform == "darwin" else "-shared", "-fPIC", + str(source), "-o", str(library)], check=True, capture_output=True) + return library + + +@pytest.mark.parametrize("scenario", ["override", "first", "last", "discovery", "sentinel", + "invalid", "overflow", "discovery_failure", "settings_failure"]) +def test_device_settings_type(settings_library, scenario): + # Isolate the process-global library override from all other tests. + script = r""" +import ctypes +import sys +import ics + +library, scenario = sys.argv[1:] +ics.override_library_name(library) +mock = ctypes.CDLL(library) +mock.observed.argtypes = [ctypes.c_int] +mock.observed.restype = ctypes.c_int +device = ics.PyNeoDeviceEx() +device._auto_handle_close = False +if scenario in ('invalid', 'overflow'): + values = (-2, ics.DeviceSettingsTypeMax, ics.DeviceSettingsTypeMax + 1, 0xFFFFFFFF) + if scenario == 'overflow': + values = (2**100, -(2**100)) + for value in values: + try: + ics.get_device_settings(device, value) + except (ValueError, OverflowError): + pass + else: + raise AssertionError(f'accepted invalid override {value}') + assert mock.observed(1) == mock.observed(2) == 0 +elif scenario in ('discovery_failure', 'settings_failure'): + discovery = scenario == 'discovery_failure' + name = 'icsneoGetDeviceSettingsType()' if discovery else 'icsneoGetDeviceSettings()' + try: + ics.get_device_settings(device, -1 if discovery else ics.DeviceFire3SettingsType, + 98 if discovery else 99) + except ics.RuntimeError as error: + assert name in str(error) + else: + raise AssertionError('native failure was ignored') + assert mock.observed(1) == int(discovery) + assert mock.observed(2) == int(not discovery) +else: + expected = {'override': ics.DeviceFire3SettingsType, 'first': 0, + 'last': ics.DeviceSettingsTypeMax - 1}.get(scenario, ics.DeviceRADJupiterSettingsType) + if scenario == 'discovery': + result = ics.get_device_settings(device) + else: + result = ics.get_device_settings(device, -1 if scenario == 'sentinel' else expected, 2) + assert mock.observed(0) == expected + assert result.DeviceSettingType == expected + assert mock.observed(1) == int(scenario in ('discovery', 'sentinel')) + assert mock.observed(2) == 1 + assert mock.observed(3) == (ics.PlasmaIonVnetChannelMain if scenario == 'discovery' else 2) + assert mock.observed(4) == ctypes.sizeof(result) +""" + env = os.environ.copy() + env["PYTHONPATH"] = os.pathsep.join(str(Path(path).resolve()) for path in sys.path) + result = subprocess.run([sys.executable, "-c", script, str(settings_library), scenario], + env=env, capture_output=True, text=True) + assert result.returncode == 0, result.stdout + result.stderr From babf43e09b768e3612f796462e7968726053cf1d Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:32:14 -0400 Subject: [PATCH 05/21] fix: preserve device handle ownership on open Reject already-open devices before a second native open. Correct capsule update status and release newly opened handles if Python assignment fails. --- src/methods.cpp | 29 +++++- tests/test_device_handle_lifecycle.py | 130 ++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 3 deletions(-) create mode 100644 tests/test_device_handle_lifecycle.py diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..4c1824c2 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -972,20 +972,27 @@ bool PyNeoDeviceEx_SetHandle(PyObject* object, void* handle) if (!PyCapsule_CheckExact(_handle) && handle) { PyObject* capsule = PyCapsule_New(handle, NULL, __destroy_PyNeoDeviceEx_Handle); if (!capsule) { + Py_DECREF(_handle); return false; } - if (PyObject_SetAttrString(object, "_handle", capsule) != 0) { + int result = PyObject_SetAttrString(object, "_handle", capsule); + Py_DECREF(capsule); + if (result != 0) { + Py_DECREF(_handle); return false; } } else if (handle) { - if (!PyCapsule_SetPointer(_handle, handle)) { - return NULL; + if (PyCapsule_SetPointer(_handle, handle) != 0) { + Py_DECREF(_handle); + return false; } } else { if (PyObject_SetAttrString(object, "_handle", Py_None) != 0) { + Py_DECREF(_handle); return false; } } + Py_DECREF(_handle); return true; } @@ -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/tests/test_device_handle_lifecycle.py b/tests/test_device_handle_lifecycle.py new file mode 100644 index 00000000..a1d22329 --- /dev/null +++ b/tests/test_device_handle_lifecycle.py @@ -0,0 +1,130 @@ +"""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" + subprocess.run([compiler, "/nologo", "/c", "/GS-", "/Zl", str(source), f"/Fo{obj}"], check=True) + subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", str(obj)], check=True) + 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) + 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, + ) + 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) +""") + From e56291f008009fc57362d3063bb986d3519915c8 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:32:21 -0400 Subject: [PATCH 06/21] 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 07/21] 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 55411f879f4a66476dd45782bfb453f015683d27 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:33:31 -0400 Subject: [PATCH 08/21] fix: correct radio message argument parsing --- src/methods.cpp | 16 ++++----- tests/test_radio_message.py | 65 +++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 tests/test_radio_message.py diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..dceaa6cd 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -2445,13 +2445,13 @@ PyObject* meth_write_sdcard( PyObject* meth_create_neovi_radio_message(PyObject* self, PyObject* args, PyObject* keywords) { (void)self; - int relay1 = 0; - int relay2 = 0; - int relay3 = 0; - int relay4 = 0; - int relay5 = 0; - int led5 = 0; - int led6 = 0; + unsigned char relay1 = 0; + unsigned char relay2 = 0; + unsigned char relay3 = 0; + unsigned char relay4 = 0; + unsigned char relay5 = 0; + unsigned char led5 = 0; + unsigned char led6 = 0; int msb = 0; int lsb = 0; int analog = 0; @@ -2461,7 +2461,7 @@ PyObject* meth_create_neovi_radio_message(PyObject* self, PyObject* args, PyObje #endif char* kwords[] = { "Relay1", "Relay2", "Relay3", "Relay4", "Relay5", "LED5", "LED6", "MSB_report_rate", "LSB_report_rate", "analog_change_report_rate", - "relay_timeout" }; + "relay_timeout", NULL }; // Accepts keywords: Relay1-Relay5 (boolean), LED5 (boolean), LED6 (boolean), MSB_report_rate (int), // LSB_report_rate (int), analog_change_report_rate (int), relay_timeout (int). if (!PyArg_ParseTupleAndKeywords(args, diff --git a/tests/test_radio_message.py b/tests/test_radio_message.py new file mode 100644 index 00000000..ce6c1148 --- /dev/null +++ b/tests/test_radio_message.py @@ -0,0 +1,65 @@ +"""Hardware-independent radio message parsing regressions. + +Run each call in a child process so native parser crashes become test failures. +""" + +import os +import subprocess +import sys + +import pytest + + +@pytest.mark.parametrize( + "code", + [ + "assert ics.create_neovi_radio_message() == (0, 0, 0, 0, 0)", + """assert ics.create_neovi_radio_message( + Relay1=1, Relay2=0, Relay3=2, Relay4=0, Relay5=255, + LED5=0, LED6=1, MSB_report_rate=0x123, LSB_report_rate=0x45, + analog_change_report_rate=0x67, relay_timeout=0x89, + ) == (0x55, 0x23, 0x45, 0x67, 0x89)""", + "assert ics.create_neovi_radio_message(0, 1, 0, 1, 0, 1, 0, 255, 128, 1, 2) == (0x2A, 255, 128, 1, 2)", + "assert ics.create_neovi_radio_message(relay_timeout=255) == (0, 0, 0, 0, 255)", + """try: + ics.create_neovi_radio_message(unknown=1) +except TypeError: + pass +else: + raise AssertionError('unknown keyword was accepted')""", + *[ + f"""try: + ics.create_neovi_radio_message(**{{{name!r}: {value!r}}}) +except {error}: + pass +else: + raise AssertionError('invalid argument was accepted')""" + for name in ("Relay1", "Relay2", "Relay3", "Relay4", "Relay5", "LED5", "LED6") + for value, error in ((-1, "OverflowError"), (256, "OverflowError"), ("bad", "TypeError")) + ], + ], + ids=["defaults", "all-keywords", "positional", "last-keyword", "unknown-keyword"] + + [ + f"{name}-{case}" + for name in ("Relay1", "Relay2", "Relay3", "Relay4", "Relay5", "LED5", "LED6") + for case in ("negative", "overflow", "non-integer") + ], +) +def test_create_neovi_radio_message_subprocess(code): + env = os.environ.copy() + # Preserve the test runner's import path, including an in-place extension. + env["PYTHONPATH"] = os.pathsep.join(sys.path) + prelude = "import ics\n" + if sys.platform == "win32": + # Suppress Windows Error Reporting dialogs when testing a broken build. + prelude = "import ctypes\nctypes.windll.kernel32.SetErrorMode(3)\n" + prelude + result = subprocess.run( + [sys.executable, "-c", prelude + code], + env=env, + capture_output=True, + text=True, + timeout=30, + ) + assert result.returncode == 0, ( + f"child exited with {result.returncode}\nstdout: {result.stdout}\nstderr: {result.stderr}" + ) From 13414b7c0926047375e76b7a5e8f21afa6745aae Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:33:33 -0400 Subject: [PATCH 09/21] fix: release synchronous buffer exports --- src/methods.cpp | 49 ++++++++-- tests/buffer_exports_mock.c | 41 ++++++++ tests/test_buffer_exports.py | 184 +++++++++++++++++++++++++++++++++++ 3 files changed, 264 insertions(+), 10 deletions(-) create mode 100644 tests/buffer_exports_mock.c create mode 100644 tests/test_buffer_exports.py diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..2a13d08e 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -42,6 +42,20 @@ class PyAllowThreads } }; +// Own an already acquired export. Construct only after successful argument +// parsing: PyArg_ParseTuple releases any acquired buffers itself on failure. +// Declare before PyAllowThreads so unwinding restores the GIL before release. +class PyBufferRelease +{ + Py_buffer& buffer; + + public: + explicit PyBufferRelease(Py_buffer& value) : buffer(value) {} + ~PyBufferRelease() { PyBuffer_Release(&buffer); } + PyBufferRelease(const PyBufferRelease&) = delete; + PyBufferRelease& operator=(const PyBufferRelease&) = delete; +}; + extern PyTypeObject spy_message_object_type; // __func__, __FUNCTION__ and __PRETTY_FUNCTION__ are not preprocessor macros. // but MSVC doesn't follow c standard and treats __FUNCTION__ as a string literal macro... @@ -3153,17 +3167,20 @@ PyObject* meth_get_hw_firmware_info(PyObject* self, PyObject* args) if (!info) { return NULL; } + std::unique_ptr result(info, Py_DecRef); Py_buffer info_buffer = {}; - PyObject_GetBuffer(info, &info_buffer, PyBUF_CONTIG); + if (PyObject_GetBuffer(info, &info_buffer, PyBUF_CONTIG) < 0) { + return NULL; + } + PyBufferRelease buffer_release(info_buffer); auto gil = PyAllowThreads(); if (!icsneoGetHWFirmwareInfo(handle, (stAPIFirmwareInfo*)info_buffer.buf)) { gil.restore(); - PyBuffer_Release(&info_buffer); return set_ics_exception(exception_runtime_error(), "icsneoGetHWFirmwareInfo() Failed"); } gil.restore(); - return info; + return result.release(); } catch (ice::Exception& ex) { return set_ics_exception(exception_runtime_error(), (char*)ex.what()); } @@ -3359,15 +3376,19 @@ PyObject* meth_get_dll_firmware_info(PyObject* self, PyObject* args) if (!info) { return NULL; } + std::unique_ptr result(info, Py_DecRef); Py_buffer info_buffer = {}; - PyObject_GetBuffer(info, &info_buffer, PyBUF_CONTIG); + if (PyObject_GetBuffer(info, &info_buffer, PyBUF_CONTIG) < 0) { + return NULL; + } + PyBufferRelease buffer_release(info_buffer); auto gil = PyAllowThreads(); if (!icsneoGetDLLFirmwareInfo(handle, (stAPIFirmwareInfo*)info_buffer.buf)) { gil.restore(); return set_ics_exception(exception_runtime_error(), "icsneoGetDLLFirmwareInfo() Failed"); } gil.restore(); - return info; + return result.release(); } catch (ice::Exception& ex) { return set_ics_exception(exception_runtime_error(), (char*)ex.what()); } @@ -3977,8 +3998,12 @@ PyObject* meth_get_device_status(PyObject* self, PyObject* args) if (!device_status) { return NULL; } + std::unique_ptr result(device_status, Py_DecRef); Py_buffer device_status_buffer = {}; - PyObject_GetBuffer(device_status, &device_status_buffer, PyBUF_CONTIG); + if (PyObject_GetBuffer(device_status, &device_status_buffer, PyBUF_CONTIG) < 0) { + return NULL; + } + PyBufferRelease buffer_release(device_status_buffer); size_t device_status_size = static_cast(device_status_buffer.len); ice::Function icsneoGetDeviceStatus(lib, @@ -3986,18 +4011,16 @@ PyObject* meth_get_device_status(PyObject* self, PyObject* args) auto gil = PyAllowThreads(); if (!icsneoGetDeviceStatus(handle, (icsDeviceStatus*)device_status_buffer.buf, &device_status_size)) { gil.restore(); - PyBuffer_Release(&device_status_buffer); return set_ics_exception(exception_runtime_error(), "icsneoGetDeviceStatus() Failed"); } if (throw_exception_on_size_mismatch) { if (device_status_size != (size_t)device_status_buffer.len) { gil.restore(); - PyBuffer_Release(&device_status_buffer); return set_ics_exception(exception_runtime_error(), "icsneoGetDeviceStatus() API mismatch detected!"); } } gil.restore(); - return device_status; + return result.release(); } catch (ice::Exception& ex) { return set_ics_exception(exception_runtime_error(), (char*)ex.what()); } @@ -4276,13 +4299,17 @@ PyObject* meth_flash_accessory_firmware(PyObject* self, PyObject* args) lib, "icsneoFlashAccessoryFirmware"); Py_buffer parms_buffer = {}; - PyObject_GetBuffer(parms, &parms_buffer, PyBUF_CONTIG_RO); + if (PyObject_GetBuffer(parms, &parms_buffer, PyBUF_CONTIG_RO) < 0) { + return NULL; + } + PyBufferRelease parms_release(parms_buffer); auto gil = PyAllowThreads(); if (!icsneoFlashAccessoryFirmware(handle, (FlashAccessoryFirmwareParams*)parms_buffer.buf, &function_error)) { gil.restore(); return set_ics_exception(exception_runtime_error(), "icsneoFlashAccessoryFirmware() Failed"); } + gil.restore(); // check the return value to make sure we are good if (check_success && function_error != AccessoryOperationSuccess) { std::stringstream ss; @@ -5029,6 +5056,7 @@ PyObject* meth_uart_write(PyObject* self, PyObject* args) return NULL; } + PyBufferRelease data_release(data); // Get the device handle if (!PyNeoDeviceEx_CheckExact(obj)) { return set_ics_exception(exception_runtime_error(), "Argument must be of type " MODULE_NAME ".PyNeoDeviceEx"); @@ -5210,6 +5238,7 @@ PyObject* meth_generic_api_send_command(PyObject* self, PyObject* args) args, arg_parse("Obbby*:", __FUNCTION__), &obj, &apiIndex, &instanceIndex, &functionIndex, &data)) { return NULL; } + PyBufferRelease data_release(data); // Get the device handle if (!PyNeoDeviceEx_CheckExact(obj)) { return set_ics_exception(exception_runtime_error(), "Argument must be of type " MODULE_NAME ".PyNeoDeviceEx"); diff --git a/tests/buffer_exports_mock.c b/tests/buffer_exports_mock.c new file mode 100644 index 00000000..2541fb98 --- /dev/null +++ b/tests/buffer_exports_mock.c @@ -0,0 +1,41 @@ +#include + +#ifdef _WIN32 +#define API __declspec(dllexport) +#define CALL __stdcall +#else +#define API __attribute__((visibility("default"))) +#define CALL +#endif + +static int mode; +API void set_mode(int value) { mode = value; } + +#ifndef OMIT_BUFFER_APIS +/* No hardware calls or file access: mode 1 fails, mode 2 reports a mismatch. */ +API int CALL icsneoUartWrite(void* h, int port, const void* data, size_t len, + size_t* sent, unsigned char* flags) +{ + *sent = mode == 2 ? 0 : len; + return mode != 1; +} +API int CALL icsneoGenericAPISendCommand(void* h, unsigned char a, unsigned char i, + unsigned char f, void* data, unsigned int len, + unsigned char* error) +{ + *error = 7; + return mode != 1; +} +API int CALL icsneoGetDeviceStatus(void* h, void* status, size_t* size) +{ + if (mode == 2) *size = 0; + return mode != 1; +} +API int CALL icsneoGetHWFirmwareInfo(void* h, void* info) { return mode != 1; } +API int CALL icsneoGetDLLFirmwareInfo(void* h, void* info) { return mode != 1; } +API int CALL icsneoFlashAccessoryFirmware(void* h, void* params, int* error) +{ + *error = mode == 2 ? 0 : 1; + return mode != 1; +} +#endif diff --git a/tests/test_buffer_exports.py b/tests/test_buffer_exports.py new file mode 100644 index 00000000..235df8c7 --- /dev/null +++ b/tests/test_buffer_exports.py @@ -0,0 +1,184 @@ +"""Exercise native buffer ownership in subprocesses using a hardware-free library.""" + +import os +from pathlib import Path +import shutil +import subprocess +import sys +import textwrap + +import pytest + + +@pytest.fixture(scope="module") +def mock_libraries(tmp_path_factory): + directory = tmp_path_factory.mktemp("buffer_exports") + source = Path(__file__).with_name("buffer_exports_mock.c") + libraries = [] + for missing in (False, True): + name = "missing" if missing else "mock" + defines = ["-DOMIT_BUFFER_APIS"] if missing else [] + if sys.platform == "win32": + compiler = shutil.which("clang-cl") + linker = shutil.which("lld-link") + if not compiler or not linker: + pytest.skip("LLVM clang-cl and lld-link are required for the mock DLL") + library = directory / (name + ".dll") + obj = directory / (name + ".obj") + subprocess.run([compiler, "/nologo", "/c", "/GS-", "/Zl", *defines, + str(source), "/Fo" + str(obj)], check=True, capture_output=True) + subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", + "/out:" + str(library), str(obj)], check=True, capture_output=True) + else: + compiler = shutil.which("cc") + if not compiler: + pytest.skip("A C compiler is required for the mock library") + library = directory / (name + (".dylib" if sys.platform == "darwin" else ".so")) + subprocess.run([compiler, "-shared", "-fPIC", *defines, str(source), "-o", str(library)], + check=True, capture_output=True) + libraries.append(library) + return libraries + + +def run_case(library, code): + # Keep the process-global library override out of other tests. Use the same + # import paths as pytest, including a locally built extension when selected. + env = dict(os.environ, PYTHONPATH=os.pathsep.join(map(str, sys.path))) + setup = f""" +import ctypes, gc, importlib, weakref +import ics +library = {str(library)!r} +mock = ctypes.CDLL(library) +mock.set_mode.argtypes = [ctypes.c_int] +mock.set_mode.restype = None +ics.override_library_name(library) +device = ics.PyNeoDeviceEx() +device._auto_handle_close = False +""" + result = subprocess.run([sys.executable, "-c", setup + textwrap.dedent(code)], + env=env, capture_output=True, text=True, timeout=30) + assert result.returncode == 0, result.stdout + result.stderr + + +@pytest.mark.parametrize("method,prefix,expected", [ + ("uart_write", "device, 0", 3), + ("generic_api_send_command", "device, 0, 0, 0", 7), + ("flash_accessory_firmware", "device", 1), +]) +@pytest.mark.parametrize("path", ["success", "failure", "missing", "invalid_device"]) +def test_input_exports_released(mock_libraries, method, prefix, expected, path): + library = mock_libraries[path == "missing"] + if path == "invalid_device": + prefix = prefix.replace("device", "None") + run_case(library, f""" + mock.set_mode({int(path == 'failure')}) + data = bytearray(b'abc') + try: + result = ics.{method}({prefix}, data) + except ics.RuntimeError: + assert {path != 'success'!r} + else: + assert {path == 'success'!r} + assert result == {expected} + data.extend(b'd') + assert data == b'abcd' + """) + + +@pytest.mark.parametrize("method,args", [ + ("uart_write", "device, 0, data"), + ("flash_accessory_firmware", "device, data"), +]) +def test_checked_error_releases_input(mock_libraries, method, args): + run_case(mock_libraries[0], f""" + mock.set_mode(2) + data = bytearray(b'abc') + try: + ics.{method}({args}) + except ics.RuntimeError: + pass + else: + raise AssertionError('expected checked error') + data.extend(b'd') + """) + + +@pytest.mark.parametrize("method,module_name", [ + ("get_device_status", "ics_device_status"), + ("get_hw_firmware_info", "st_api_firmware_info"), + ("get_dll_firmware_info", "st_api_firmware_info"), +]) +@pytest.mark.parametrize("path", ["success", "failure", "missing", "no_buffer"]) +def test_result_objects_collectable(mock_libraries, method, module_name, path): + run_case(mock_libraries[path == "missing"], f""" + module = importlib.import_module('ics.structures.{module_name}') + original = module.{module_name} + refs = [] + class NoBuffer: + pass + def create(): + obj = NoBuffer() if {path == 'no_buffer'!r} else original() + refs.append(weakref.ref(obj)) + return obj + module.{module_name} = create + mock.set_mode({int(path == 'failure')}) + try: + result = ics.{method}(device) + except (ics.RuntimeError, TypeError): + assert {path != 'success'!r} + else: + assert {path == 'success'!r} + assert isinstance(result, original) + del result + gc.collect() + assert all(ref() is None for ref in refs) + if {path != 'missing'!r}: + assert len(refs) == 1 + """) + + +def test_status_size_mismatch_collectable(mock_libraries): + run_case(mock_libraries[0], """ + module = importlib.import_module('ics.structures.ics_device_status') + original = module.ics_device_status + refs = [] + def create(): + obj = original() + refs.append(weakref.ref(obj)) + return obj + module.ics_device_status = create + mock.set_mode(2) + try: + ics.get_device_status(device, True) + except ics.RuntimeError: + pass + else: + raise AssertionError('expected size mismatch') + gc.collect() + assert refs[0]() is None + """) + + +def test_accessory_buffer_acquisition_failure(mock_libraries): + run_case(mock_libraries[0], """ + try: + ics.flash_accessory_firmware(device, object()) + except TypeError: + pass + else: + raise AssertionError('expected buffer acquisition failure') + """) + + +def test_uart_late_parse_failure_releases_once(mock_libraries): + run_case(mock_libraries[0], """ + data = bytearray(b'abc') + try: + ics.uart_write(device, 0, data, object()) + except TypeError: + pass + else: + raise AssertionError('expected argument conversion failure') + data.extend(b'd') + """) + From 7b74272a35bf5380fdf121583d472bd710032149 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:33:48 -0400 Subject: [PATCH 10/21] fix: retain registered reflash callbacks --- include/methods.h | 5 +- src/methods.cpp | 66 +++++-- tests/test_reflash_callback.py | 302 +++++++++++++++++++++++++++++++++ 3 files changed, 362 insertions(+), 11 deletions(-) create mode 100644 tests/test_reflash_callback.py diff --git a/include/methods.h b/include/methods.h index 605c1279..01cf495d 100644 --- a/include/methods.h +++ b/include/methods.h @@ -436,7 +436,10 @@ extern "C" MODULE_NAME \ ".set_reflash_callback(callback)\n" \ "\n" \ - "Sets the reflash display callback.\n" \ + "Sets the reflash display callback. The callback is retained until replaced or disabled.\n" \ + "An object with a callable reflash_callback method is also accepted.\n" \ + "Pass None to disable callbacks, or omit the argument to print progress to stdout.\n" \ + "Callback exceptions are reported through sys.unraisablehook.\n" \ "\n" \ "Args:\n" \ "\tcallback (:class:`function`): Must be a callable Python function (`def callback(msg, progress)`)\n\n" \ diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..01dfb14c 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -2139,18 +2139,45 @@ PyObject* meth_flash_devices(PyObject* self, PyObject* args) } #endif // _USE_INTERNAL_HEADER_ -PyObject* msg_reflash_callback = NULL; +// Owned reference, accessed with the GIL held. NULL selects stdout output. +static PyObject* msg_reflash_callback = NULL; static void message_reflash_callback(const wchar_t* message, unsigned long progress) { // We need to relock the GIL here otherwise we crash PyGILState_STATE state = PyGILState_Ensure(); - if (!msg_reflash_callback) { - PySys_WriteStdout("%ls -%ld\n", message, progress); - } else if (PyObject_HasAttrString(msg_reflash_callback, "reflash_callback")) { - PyObject_CallMethod(msg_reflash_callback, "reflash_callback", "u,k", message, progress); + // Keep the current handler alive even if it unregisters/replaces itself. + PyObject* callback = msg_reflash_callback; + Py_XINCREF(callback); + if (!callback) { + PyObject* text = PyUnicode_FromWideChar(message, -1); + if (text) { + PySys_FormatStdout("%U -%lu\n", text, progress); + Py_DECREF(text); + } else { + PyErr_WriteUnraisable(Py_None); + } } else { - PyObject_CallFunction(msg_reflash_callback, "u,k", message, progress); + PyObject* callable = PyObject_GetAttrString(callback, "reflash_callback"); + if (!callable && PyErr_ExceptionMatches(PyExc_AttributeError)) { + PyErr_Clear(); + callable = callback; + Py_INCREF(callable); + } + if (callable) { + PyObject* text = PyUnicode_FromWideChar(message, -1); + if (text) { + PyObject* result = PyObject_CallFunction(callable, "Ok", text, progress); + Py_XDECREF(result); + Py_DECREF(text); + } + Py_DECREF(callable); + } + // Native progress notifications have no Python caller to receive errors. + if (PyErr_Occurred()) { + PyErr_WriteUnraisable(callback); + } } + Py_XDECREF(callback); // Unlock the GIL here again... PyGILState_Release(state); } @@ -2163,10 +2190,22 @@ PyObject* meth_set_reflash_callback(PyObject* self, PyObject* args) if (!PyArg_ParseTuple(args, arg_parse("|O:", __FUNCTION__), &callback)) { return NULL; } - if (!callback) { - msg_reflash_callback = NULL; - } else { - msg_reflash_callback = callback; + if (callback && callback != Py_None) { + PyObject* callable = PyObject_GetAttrString(callback, "reflash_callback"); + if (!callable) { + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { + return NULL; + } + PyErr_Clear(); + callable = callback; + Py_INCREF(callable); + } + int is_callable = PyCallable_Check(callable); + Py_DECREF(callable); + if (!is_callable) { + PyErr_SetString(PyExc_TypeError, "callback must be callable or have a callable reflash_callback method"); + return NULL; + } } try { ice::Library* lib = dll_get_library(); @@ -2176,6 +2215,12 @@ PyObject* meth_set_reflash_callback(PyObject* self, PyObject* args) } ice::Function icsneoSetReflashCallback( lib, "icsneoSetReflashCallback"); + // Resolve the library symbol before changing ownership. Publish before + // calling native code, which may immediately deliver a progress event. + PyObject* replacement = callback == Py_None ? NULL : callback; + Py_XINCREF(replacement); + PyObject* previous = msg_reflash_callback; + msg_reflash_callback = replacement; auto gil = PyAllowThreads(); if (callback == Py_None) { icsneoSetReflashCallback(NULL); @@ -2183,6 +2228,7 @@ PyObject* meth_set_reflash_callback(PyObject* self, PyObject* args) icsneoSetReflashCallback(&message_reflash_callback); } gil.restore(); + Py_XDECREF(previous); Py_RETURN_NONE; } catch (ice::Exception& ex) { return set_ics_exception(exception_runtime_error(), (char*)ex.what()); diff --git a/tests/test_reflash_callback.py b/tests/test_reflash_callback.py new file mode 100644 index 00000000..c828fc17 --- /dev/null +++ b/tests/test_reflash_callback.py @@ -0,0 +1,302 @@ +"""Hardware-free callback tests; each case owns an isolated native-library registration.""" + +import os +from pathlib import Path +import shutil +import subprocess +import sys + +import pytest + + +@pytest.fixture(scope="module") +def callback_libraries(tmp_path_factory): + directory = tmp_path_factory.mktemp("reflash_callback") + source = directory / "callback.c" + source.write_text(r""" +#ifdef _WIN32 +#define API __declspec(dllexport) +#define CALL __stdcall +typedef unsigned short wchar_t; +#else +#include +#define API __attribute__((visibility("default"))) +#define CALL +#endif +typedef void (*callback_t)(const wchar_t*, unsigned long); +static callback_t callback; +static int fire_on_registration; +API void fire_callback(void); +API void set_fire_on_registration(int value) { fire_on_registration = value; } +#ifndef OMIT_SETTER +API void CALL icsneoSetReflashCallback(callback_t value) { + callback = value; + if (fire_on_registration) fire_callback(); +} +#endif +API void fire_callback(void) { + static const wchar_t message[] = { 'p', 'r', 'o', 'g', 'r', 'e', 's', 's', 0x2713, 0 }; + if (callback) callback(message, 42); +} +""") + libraries = [] + for missing in (False, True): + name = "missing" if missing else "callback" + if sys.platform == "win32": + compiler = shutil.which("clang-cl") + linker = shutil.which("lld-link") + if not compiler or not linker: + pytest.skip("callback mock requires clang-cl and lld-link") + library = directory / (name + ".dll") + obj = directory / (name + ".obj") + target = "i686-pc-windows-msvc" if sys.maxsize <= 2**32 else "x86_64-pc-windows-msvc" + command = [compiler, "--target=" + target, "/nologo", "/c", "/GS-", "/Zl", str(source), "/Fo" + str(obj)] + if missing: + command.append("/DOMIT_SETTER") + subprocess.run(command, check=True, capture_output=True) + exports = ( + ["/export:fire_callback=_fire_callback", "/export:set_fire_on_registration=_set_fire_on_registration"] + if sys.maxsize <= 2**32 + else [] + ) + if sys.maxsize <= 2**32 and not missing: + exports.append("/export:icsneoSetReflashCallback=_icsneoSetReflashCallback@4") + subprocess.run( + [linker, "/dll", "/noentry", "/nodefaultlib", "/out:" + str(library), str(obj), *exports], + check=True, + capture_output=True, + ) + else: + compiler = shutil.which("cc") + if not compiler: + pytest.skip("callback mock requires a C compiler") + library = directory / (name + (".dylib" if sys.platform == "darwin" else ".so")) + command = [ + compiler, + "-dynamiclib" if sys.platform == "darwin" else "-shared", + "-fPIC", + str(source), + "-o", + str(library), + ] + if missing: + command.append("-DOMIT_SETTER") + subprocess.run(command, check=True, capture_output=True) + libraries.append(library) + return libraries + + +PREAMBLE = """ +import ctypes, gc, sys, weakref +import ics +ics.override_library_name(sys.argv[1]) +native = ctypes.CDLL(sys.argv[1]) +native.fire_callback.argtypes = [] +native.fire_callback.restype = None +events = [] +class Handler: + def __call__(self, message, progress): + events.append((message, progress)) +def collect(): + gc.collect() +""" + + +CASES = { + "lifetime": """ +handler = Handler() +ref = weakref.ref(handler) +ics.set_reflash_callback(handler) +del handler +collect() +assert ref() is not None, 'registered callback was collected' +native.fire_callback() +assert events == [('progress\\u2713', 42)] +ics.set_reflash_callback(None) +collect() +assert ref() is None +native.fire_callback() +assert len(events) == 1 +""", + "replacement": """ +first, second = Handler(), Handler() +old, new = weakref.ref(first), weakref.ref(second) +ics.set_reflash_callback(first) +ics.set_reflash_callback(first) +del first +ics.set_reflash_callback(second) +del second +collect() +assert old() is None and new() is not None +ics.set_reflash_callback(None) +collect() +assert new() is None +""", + "method": """ +class MethodHandler: + def reflash_callback(self, message, progress): + events.append((message, progress)) +handler = MethodHandler() +ref = weakref.ref(handler) +ics.set_reflash_callback(handler) +del handler +collect() +assert ref() is not None +native.fire_callback() +assert events == [('progress\\u2713', 42)] +ics.set_reflash_callback(None) +collect() +assert ref() is None +""", + "invalid": """ +handler = Handler() +ics.set_reflash_callback(handler) +class Invalid: + reflash_callback = 1 +for value in (123, object(), Invalid()): + try: + ics.set_reflash_callback(value) + except TypeError: + pass + else: + raise AssertionError('accepted invalid callback') +native.fire_callback() +assert len(events) == 1 +ics.set_reflash_callback(None) +""", + "return_value": """ +refs = [] +def callback(message, progress): + result = Handler() + refs.append(weakref.ref(result)) + return result +ics.set_reflash_callback(callback) +for _ in range(10): + native.fire_callback() +collect() +assert all(ref() is None for ref in refs) +ics.set_reflash_callback(None) +""", + "exception": """ +errors = [] +sys.unraisablehook = lambda error: errors.append((error.exc_type, str(error.exc_value))) +def callback(message, progress): + raise ValueError('callback failed') +ics.set_reflash_callback(callback) +native.fire_callback() +native.fire_callback() +assert errors == [(ValueError, 'callback failed')] * 2 +ics.set_reflash_callback(Handler()) +native.fire_callback() +assert len(events) == 1 +ics.set_reflash_callback(None) +""", + "reentrant": """ +refs = [] +errors = [] +sys.unraisablehook = lambda error: errors.append(error.exc_type) +class Reentrant: + def __call__(self, message, progress): + ics.set_reflash_callback(None) + collect() + assert refs[0]() is self + raise ValueError('after unregister') +handler = Reentrant() +refs.append(weakref.ref(handler)) +ics.set_reflash_callback(handler) +del handler +native.fire_callback() +collect() +assert errors == [ValueError] +assert refs[0]() is None +""", + "stdout": """ +import contextlib, io +handler = Handler() +ref = weakref.ref(handler) +ics.set_reflash_callback(handler) +del handler +ics.set_reflash_callback() +collect() +assert ref() is None +output = io.StringIO() +with contextlib.redirect_stdout(output): + native.fire_callback() +assert output.getvalue() == 'progress\\u2713 -42\\n' +ics.set_reflash_callback(None) +""", + "missing_symbol": """ +handler = Handler() +ref = weakref.ref(handler) +ics.set_reflash_callback(handler) +del handler +ics.override_library_name(sys.argv[2]) +candidate = Handler() +candidate_ref = weakref.ref(candidate) +try: + ics.set_reflash_callback(candidate) +except ics.RuntimeError: + pass +else: + raise AssertionError('missing symbol accepted') +del candidate +collect() +assert ref() is not None and candidate_ref() is None +native.fire_callback() +assert len(events) == 1 +ics.override_library_name(sys.argv[1]) +ics.set_reflash_callback(None) +collect() +assert ref() is None +""", + "attribute_error": """ +class Broken: + @property + def reflash_callback(self): + raise ValueError('attribute failed') +ics.set_reflash_callback(Handler()) +try: + ics.set_reflash_callback(Broken()) +except ValueError as error: + assert str(error) == 'attribute failed' +else: + raise AssertionError('attribute error suppressed') +native.fire_callback() +assert len(events) == 1 +ics.set_reflash_callback(None) +""", + "thread": """ +import threading +ics.set_reflash_callback(Handler()) +worker = threading.Thread(target=native.fire_callback) +worker.start() +worker.join(timeout=5) +assert not worker.is_alive() +assert events == [('progress\\u2713', 42)] +ics.set_reflash_callback(None) +""", + "immediate": """ +native.set_fire_on_registration.argtypes = [ctypes.c_int] +native.set_fire_on_registration.restype = None +native.set_fire_on_registration(1) +ics.set_reflash_callback(Handler()) +assert events == [('progress\\u2713', 42)] +ics.set_reflash_callback(None) +assert len(events) == 1 +""", +} + + +@pytest.mark.parametrize("case", CASES) +def test_reflash_callback(callback_libraries, case): + env = os.environ.copy() + # Preserve the selected checkout/build when launching an isolated interpreter. + env["PYTHONPATH"] = os.pathsep.join(str(Path(path).resolve()) for path in sys.path) + result = subprocess.run( + [sys.executable, "-c", PREAMBLE + CASES[case], *map(str, callback_libraries)], + env=env, + capture_output=True, + text=True, + timeout=30, + ) + assert result.returncode == 0, result.stdout + result.stderr From d213d8ddff7fa8ec2fcdf0f80b20bc2b283a0421 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:33:50 -0400 Subject: [PATCH 11/21] fix: release script load buffers on every exit --- src/methods.cpp | 54 +++++++++++---- tests/test_script_load.py | 136 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 178 insertions(+), 12 deletions(-) create mode 100644 tests/test_script_load.py diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..e5bb3cd2 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -1547,7 +1547,7 @@ PyObject* meth_coremini_load(PyObject* self, PyObject* args) return NULL; } long fsize; - unsigned char* data = NULL; + std::unique_ptr data(nullptr, &free); int data_size = 0; #if PY_MAJOR_VERSION >= 3 if (PyUnicode_CheckExact(arg_data)) { @@ -1575,15 +1575,26 @@ PyObject* meth_coremini_load(PyObject* self, PyObject* args) fclose(f); return set_ics_exception(exception_runtime_error(), "CoreMini script file size is invalid"); } - data = (unsigned char*)malloc(sizeof(char) * fsize); - data_size = (int)fread(data, 1, static_cast(fsize), f); + data.reset(static_cast(malloc(fsize ? static_cast(fsize) : 1))); + if (!data) { + fclose(f); + return PyErr_NoMemory(); + } + data_size = (int)fread(data.get(), 1, static_cast(fsize), f); fclose(f); if (fsize != data_size) { return set_ics_exception(exception_runtime_error(), "CoreMini binary file size mismatch"); } } else if (PyTuple_CheckExact(arg_data)) { Py_ssize_t tuple_size = PyTuple_Size(arg_data); - data = (unsigned char*)malloc(sizeof(char) * tuple_size); + if (tuple_size > INT_MAX) { + PyErr_SetString(PyExc_OverflowError, "Script tuple is too large"); + return NULL; + } + data.reset(static_cast(malloc(tuple_size ? static_cast(tuple_size) : 1))); + if (!data) { + return PyErr_NoMemory(); + } // Move tuple data into array for (int i = 0; i < tuple_size; ++i) { PyObject* value = PyTuple_GET_ITEM(arg_data, i); @@ -1591,7 +1602,11 @@ PyObject* meth_coremini_load(PyObject* self, PyObject* args) return set_ics_exception(exception_runtime_error(), "Failed to convert tuple data. Tuple data must be integer type"); } - data[i] = (unsigned char)PyLong_AsLong(PyTuple_GET_ITEM(arg_data, i)); + long byte = PyLong_AsLong(value); + if (byte == -1 && PyErr_Occurred()) { + return NULL; + } + data.get()[i] = static_cast(byte); } fsize = static_cast(tuple_size); data_size = fsize; @@ -1607,7 +1622,7 @@ PyObject* meth_coremini_load(PyObject* self, PyObject* args) ice::Function icsneoScriptLoad( lib, "icsneoScriptLoad"); auto gil = PyAllowThreads(); - if (!icsneoScriptLoad(handle, data, static_cast(data_size), location)) { + if (!icsneoScriptLoad(handle, data.get(), static_cast(data_size), location)) { gil.restore(); return set_ics_exception(exception_runtime_error(), "icsneoScriptLoad() Failed"); } @@ -3496,7 +3511,7 @@ PyObject* meth_load_readbin(PyObject* self, PyObject* args) return NULL; } long fsize; - unsigned char* data = NULL; + std::unique_ptr data(nullptr, &free); int data_size = 0; #if PY_MAJOR_VERSION >= 3 if (PyUnicode_CheckExact(arg_data)) { @@ -3524,15 +3539,26 @@ PyObject* meth_load_readbin(PyObject* self, PyObject* args) fclose(f); return set_ics_exception(exception_runtime_error(), "Readbin file size is invalid"); } - data = (unsigned char*)malloc(sizeof(char) * fsize); - data_size = (int)fread(data, 1, static_cast(fsize), f); + data.reset(static_cast(malloc(fsize ? static_cast(fsize) : 1))); + if (!data) { + fclose(f); + return PyErr_NoMemory(); + } + data_size = (int)fread(data.get(), 1, static_cast(fsize), f); fclose(f); if (fsize != data_size) { return set_ics_exception(exception_runtime_error(), "Readbin file size mismatch"); } } else if (PyTuple_CheckExact(arg_data)) { Py_ssize_t tuple_size = PyTuple_Size(arg_data); - data = (unsigned char*)malloc(sizeof(char) * tuple_size); + if (tuple_size > INT_MAX) { + PyErr_SetString(PyExc_OverflowError, "Script tuple is too large"); + return NULL; + } + data.reset(static_cast(malloc(tuple_size ? static_cast(tuple_size) : 1))); + if (!data) { + return PyErr_NoMemory(); + } // Move tuple data into array for (int i = 0; i < tuple_size; ++i) { PyObject* value = PyTuple_GET_ITEM(arg_data, i); @@ -3540,7 +3566,11 @@ PyObject* meth_load_readbin(PyObject* self, PyObject* args) return set_ics_exception(exception_runtime_error(), "Failed to convert tuple data. Tuple data must be integer type"); } - data[i] = (unsigned char)PyLong_AsLong(PyTuple_GET_ITEM(arg_data, i)); + long byte = PyLong_AsLong(value); + if (byte == -1 && PyErr_Occurred()) { + return NULL; + } + data.get()[i] = static_cast(byte); } fsize = static_cast(tuple_size); data_size = fsize; @@ -3556,7 +3586,7 @@ PyObject* meth_load_readbin(PyObject* self, PyObject* args) ice::Function icsneoScriptLoadReadBin( lib, "icsneoScriptLoadReadBin"); auto gil = PyAllowThreads(); - if (!icsneoScriptLoadReadBin(handle, data, static_cast(data_size), location)) { + if (!icsneoScriptLoadReadBin(handle, data.get(), static_cast(data_size), location)) { gil.restore(); return set_ics_exception(exception_runtime_error(), "icsneoScriptLoadReadBin() Failed"); } diff --git a/tests/test_script_load.py b/tests/test_script_load.py new file mode 100644 index 00000000..2e72feb3 --- /dev/null +++ b/tests/test_script_load.py @@ -0,0 +1,136 @@ +"""Hardware-free script loading tests; each mock runs in a separate process.""" + +import os +from pathlib import Path +import shutil +import subprocess +import sys + +import ics +import pytest + + +@pytest.fixture(scope="module") +def script_library(tmp_path_factory): + root = tmp_path_factory.mktemp("script-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 +API int CALL icsneoScriptLoad(void* h, const unsigned char* data, unsigned long size, int location) { + unsigned long i; + if (location == -1) return 0; + for (i = 0; i < size; ++i) if (data[i] != (unsigned char)i) return 0; + return 1; +} +API int CALL icsneoScriptLoadReadBin(void* h, const unsigned char* data, unsigned long size, int location) { + return icsneoScriptLoad(h, data, size, location); +} +""") + if sys.platform == "win32": + compiler, linker = shutil.which("clang-cl"), shutil.which("lld-link") + if not compiler or not linker: + pytest.skip("LLVM is required for the mock DLL") + library = root / "mock.dll" + subprocess.run([compiler, "/nologo", "/c", "/GS-", "/Zl", str(source), + f"/Fo{root / 'mock.obj'}"], check=True, capture_output=True) + subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", + str(root / "mock.obj")], check=True, capture_output=True) + else: + compiler = shutil.which("cc") + if not compiler: + pytest.skip("C compiler is required for the mock library") + library = root / "mock.so" + subprocess.run([compiler, "-shared", "-fPIC", str(source), "-o", str(library)], check=True) + return library + + +@pytest.mark.parametrize("loader", ["coremini_load", "load_readbin"]) +@pytest.mark.parametrize("kind", ["file", "tuple"]) +@pytest.mark.parametrize("failure", [False, True]) +def test_script_load(script_library, tmp_path, loader, kind, failure): + if not hasattr(ics, loader): + pytest.skip("load_readbin requires the internal header build") + # Preserve the exact extension under test without changing the parent process's DLL. + env = os.environ.copy() + env["PYTHONPATH"] = str(Path(ics.__file__).resolve().parent.parent) + script = r''' +import ctypes +import sys +from pathlib import Path +import ics + +library, loader, kind, failure, root = sys.argv[1:] +ics.override_library_name(library) +load = getattr(ics, loader) +device = ics.PyNeoDeviceEx() +device._auto_handle_close = False +payload = bytes(range(256)) * 4096 +path = Path(root) / "payload.bin" +path.write_bytes(payload) +data = str(path) if kind == "file" else tuple(payload) + +def call(): + if failure == "True": + try: + load(device, data, -1) + except ics.RuntimeError: + pass + else: + raise AssertionError("native failure was ignored") + else: + assert load(device, data, 0) is None + +def private_bytes(): + # Windows private commit includes malloc storage even when it is not resident. + class Counters(ctypes.Structure): + _fields_ = [("cb", ctypes.c_ulong), ("PageFaultCount", ctypes.c_ulong)] + [ + (name, ctypes.c_size_t) for name in ( + "PeakWorkingSetSize", "WorkingSetSize", "QuotaPeakPagedPoolUsage", + "QuotaPagedPoolUsage", "QuotaPeakNonPagedPoolUsage", "QuotaNonPagedPoolUsage", + "PagefileUsage", "PeakPagefileUsage", "PrivateUsage")] + counters = Counters() + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + kernel.GetCurrentProcess.restype = ctypes.c_void_p + psapi = ctypes.WinDLL("psapi", use_last_error=True) + psapi.GetProcessMemoryInfo.argtypes = [ctypes.c_void_p, ctypes.c_void_p, ctypes.c_ulong] + assert psapi.GetProcessMemoryInfo(kernel.GetCurrentProcess(), ctypes.byref(counters), ctypes.sizeof(counters)) + return counters.PrivateUsage + +for _ in range(4): + call() +before = private_bytes() if sys.platform == "win32" else None +for _ in range(32): + call() +if before is not None: + growth = private_bytes() - before + print("private byte growth:", growth) + assert growth < 8 * 1024 * 1024, growth + +# Failed conversions must preserve their exception and never call the native API. +for value, error in [(object(), ics.RuntimeError), (1 << 100, OverflowError)]: + try: + load(device, (0, value), 0) + except error: + pass + else: + raise AssertionError("invalid tuple accepted") +empty = Path(root) / "empty.bin" +empty.write_bytes(b"") +assert load(device, str(empty), 0) is None +assert load(device, (), 0) is None +try: + load(device, str(Path(root) / "missing.bin"), 0) +except ics.RuntimeError: + pass +else: + raise AssertionError("missing file accepted") +''' + result = subprocess.run([sys.executable, "-c", script, str(script_library), loader, kind, + str(failure), str(tmp_path)], env=env, capture_output=True, text=True) + assert result.returncode == 0, result.stdout + result.stderr From 8aa997afc5df533f5b17bd5ea294365151d8f106 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:34:38 -0400 Subject: [PATCH 12/21] test: fix x86 settings mock exports --- tests/test_device_settings.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/test_device_settings.py b/tests/test_device_settings.py index e2978cb7..6835a806 100644 --- a/tests/test_device_settings.py +++ b/tests/test_device_settings.py @@ -54,16 +54,21 @@ def settings_library(tmp_path_factory): obj = directory / "settings.obj" target = "i686" if sys.maxsize <= 2**32 else "x86_64" subprocess.run([compiler, f"--target={target}-pc-windows-msvc", "/nologo", "/c", "/GS-", "/Zl", - str(source), f"/Fo{obj}"], check=True, capture_output=True) - subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", str(obj)], - check=True, capture_output=True) + str(source), f"/Fo{obj}"], check=True, capture_output=True, timeout=60) + # The wrapper resolves undecorated API names, also on 32-bit Windows. + exports = [] if target == "x86_64" else [ + "/export:icsneoGetDeviceSettings=_icsneoGetDeviceSettings@16", + "/export:icsneoGetDeviceSettingsType=_icsneoGetDeviceSettingsType@12", + ] + subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", str(obj), *exports], + check=True, capture_output=True, timeout=60) else: compiler = shutil.which("cc") if not compiler: pytest.skip("A C compiler is required for the mock library") library = directory / ("settings.dylib" if sys.platform == "darwin" else "settings.so") subprocess.run([compiler, "-dynamiclib" if sys.platform == "darwin" else "-shared", "-fPIC", - str(source), "-o", str(library)], check=True, capture_output=True) + str(source), "-o", str(library)], check=True, capture_output=True, timeout=60) return library @@ -124,5 +129,5 @@ def test_device_settings_type(settings_library, scenario): env = os.environ.copy() env["PYTHONPATH"] = os.pathsep.join(str(Path(path).resolve()) for path in sys.path) result = subprocess.run([sys.executable, "-c", script, str(settings_library), scenario], - env=env, capture_output=True, text=True) + env=env, capture_output=True, text=True, timeout=30) assert result.returncode == 0, result.stdout + result.stderr From 8aac41b06139194376bbdda8b2ac338d1f7d2321 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:35:04 -0400 Subject: [PATCH 13/21] test: support x86 device handle mocks --- tests/test_device_handle_lifecycle.py | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/tests/test_device_handle_lifecycle.py b/tests/test_device_handle_lifecycle.py index a1d22329..df5cfaa9 100644 --- a/tests/test_device_handle_lifecycle.py +++ b/tests/test_device_handle_lifecycle.py @@ -44,14 +44,26 @@ def handle_library(tmp_path_factory): pytest.skip("handle mock requires clang-cl and lld-link") library = root / "mock.dll" obj = root / "mock.obj" - subprocess.run([compiler, "/nologo", "/c", "/GS-", "/Zl", str(source), f"/Fo{obj}"], check=True) - subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", str(obj)], check=True) + 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) + subprocess.run([compiler, "-shared", "-fPIC", str(source), "-o", str(library)], check=True, timeout=60) return library @@ -77,7 +89,7 @@ def run_case(library, code): def counts(): return tuple(mock.counts(i) for i in range(3)) """ + code, str(library)], - env=env, capture_output=True, text=True, + env=env, capture_output=True, text=True, timeout=30, ) 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 14/21] 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 15/21] 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 70a7c6aa20814e7da918b6ed77bd3c5adc1439c7 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:38:00 -0400 Subject: [PATCH 16/21] fix(build): avoid locking existing extension --- .github/workflows/tests.yml | 11 ++++- generate_icsneo40_structs.py | 45 ++++++++++++------- pyproject.toml | 4 +- tests/test_generator_validation.py | 69 ++++++++++++++++++++++++++++++ uv.lock | 29 ++++++++++++- 5 files changed, 140 insertions(+), 18 deletions(-) create mode 100644 tests/test_generator_validation.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 4a585c0f..635e1198 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -49,6 +49,15 @@ jobs: run: | uv sync --group test + - name: Rebuild extension in place twice (Windows) + if: runner.os == 'Windows' + run: | + uv pip install setuptools wheel + uv run --no-sync python setup.py build_ext --inplace --force + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + uv run --no-sync python setup.py build_ext --inplace --force + if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + - name: Run tests run: | - uv run pytest tests/ --verbose \ No newline at end of file + uv run pytest tests/ --verbose diff --git a/generate_icsneo40_structs.py b/generate_icsneo40_structs.py index 4a466dea..288b36d2 100644 --- a/generate_icsneo40_structs.py +++ b/generate_icsneo40_structs.py @@ -737,24 +737,39 @@ def generate(filename="include/ics/icsnVC40.h"): f.write(f' "ics.structures.{fname}",\n') f.write("]\n\n") - # Verify We can at least import all of the modules - quick check to make sure parser worked. - ics_module_path = GEN_ICS_DIR.parent.resolve() - # Add the module to the eval sys.path. - eval("""sys.path.insert(0, f"{ics_module_path}")""") - for file_name in file_names: - if file_name.startswith("__"): - continue - import_line = "from ics.structures import {}".format( - re.sub(r'(\.py)', '', file_name)) - try: - print(f"Importing / Verifying {output_dir / file_name}...{' '*20}", end="\r") - exec(import_line) - except Exception as ex: - print(f"""\nERROR: {ex} IMPORT LINE: '{import_line}'""") - raise ex + validate_generated_modules(file_names) print("\nDone.") +def validate_generated_modules(file_names): + """Check imports without keeping an existing native extension loaded in setup.""" + import json + + # Importing ics also loads ics.ics when it exists. Windows cannot replace a + # loaded .pyd, so wait for a separate interpreter to exit before building. + # Pass paths and names as data, including paths containing spaces/backslashes. + validation_code = """ +import importlib +import json +import sys + +package_path, file_names = json.load(sys.stdin) +sys.path.insert(0, package_path) +for file_name in file_names: + if file_name.startswith("__"): + continue + module_name = "ics.structures." + file_name.removesuffix(".py") + print(f"Importing / Verifying {module_name}...", flush=True) + importlib.import_module(module_name) +""" + run( + [sys.executable, "-c", validation_code], + input=json.dumps([str(GEN_ICS_DIR.parent.resolve()), file_names]), + text=True, + check=True, + ) + + def _write_c_object(f, c_object): # Write the header if c_object.data_type == DataType.Struct: diff --git a/pyproject.toml b/pyproject.toml index faea49e5..29dd3e4d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,10 +31,12 @@ Issues = "https://github.com/intrepidcs/python_ics/issues" [dependency-groups] test = [ "pytest>=8.4.2", + "dunamai", + "setuptools", ] [tool.setuptools.dynamic] -version = { attr = "ics.__version__" } +version = { attr = "ics.__version.__version__" } [build-system] requires = [ diff --git a/tests/test_generator_validation.py b/tests/test_generator_validation.py new file mode 100644 index 00000000..efea858d --- /dev/null +++ b/tests/test_generator_validation.py @@ -0,0 +1,69 @@ +"""Build-time import validation must not load ics into the build process.""" +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +import generate_icsneo40_structs as generator + + +@pytest.fixture +def generated_package(tmp_path, monkeypatch): + package = tmp_path / "generated package" / "ics" + structures = package / "structures" + structures.mkdir(parents=True) + (package / "__init__.py").write_text("", encoding="utf-8") + (structures / "__init__.py").write_text("", encoding="utf-8") + monkeypatch.setattr(generator, "GEN_ICS_DIR", package) + return structures + + +def test_validation_imports_in_child_and_leaves_parent_unchanged(generated_package): + marker = generated_package / "import-pid.txt" + (generated_package / "sample.py").write_text( + "import os\nfrom pathlib import Path\n" + f"Path({str(marker)!r}).write_text(str(os.getpid()))\n", + encoding="utf-8", + ) + original_path = sys.path[:] + original_modules = {k: v for k, v in sys.modules.items() if k == "ics" or k.startswith("ics.")} + + generator.validate_generated_modules(["sample.py"]) + + assert int(marker.read_text()) != os.getpid() + assert sys.path == original_path + assert {k: v for k, v in sys.modules.items() if k == "ics" or k.startswith("ics.")} == original_modules + + +@pytest.mark.parametrize("source", ["raise RuntimeError('invalid structure')\n", "invalid syntax !\n", None]) +def test_validation_propagates_import_errors(generated_package, source): + if source is not None: + (generated_package / "broken.py").write_text(source, encoding="utf-8") + + with pytest.raises(subprocess.CalledProcessError): + generator.validate_generated_modules(["broken.py"]) + + +def test_validation_reads_regenerated_modules(generated_package): + module = generated_package / "sample.py" + module.write_text("value = 1\n", encoding="utf-8") + generator.validate_generated_modules(["sample.py"]) + module.write_text("raise RuntimeError('regenerated invalid structure')\n", encoding="utf-8") + + with pytest.raises(subprocess.CalledProcessError): + generator.validate_generated_modules(["sample.py"]) + + +def test_build_version_is_read_without_importing_package(generated_package): + from setuptools.config.expand import read_attr + from setuptools.config.pyprojecttoml import load_file + + package = generated_package.parent + (package / "__init__.py").write_text("raise RuntimeError('package must not be imported')\n") + (package / "__version.py").write_text('__version__ = "1.2.3"\n') + config = load_file(Path(__file__).resolve().parents[1] / "pyproject.toml") + version_attr = config["tool"]["setuptools"]["dynamic"]["version"]["attr"] + + assert read_attr(version_attr, package_dir={"ics": str(package)}) == "1.2.3" diff --git a/uv.lock b/uv.lock index ae0894e8..0ec21654 100644 --- a/uv.lock +++ b/uv.lock @@ -11,6 +11,18 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/d1/d6/3965ed04c63042e047cb6a3e6ed1a63a35087b6a609aa3a15ed8ac56c221/colorama-0.4.6-py2.py3-none-any.whl", hash = "sha256:4f1d9991f5acc0ca119f9d443620b77f9d6b33703e51011c16baf57afb285fc6", size = 25335, upload-time = "2022-10-25T02:36:20.889Z" }, ] +[[package]] +name = "dunamai" +version = "1.26.2" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "packaging" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/12/18/020d3b27a10450ddb11429f637404e8ea67ecf4d9fd999d4f1d553f25506/dunamai-1.26.2.tar.gz", hash = "sha256:84ea45eddf9bb4b40df7610b1b22a03137365e6257dbf9d7b72128fdccca564c", size = 46536, upload-time = "2026-08-02T03:32:50.276Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/f0/31/2aaabe7d03f395c7b6d955f09a4d1440a2c49920abea42fde88cacc2ef97/dunamai-1.26.2-py3-none-any.whl", hash = "sha256:4234be3a90c3ec13ecf75d94a2248068e5a3018e8112892fcd9f3a1f287c9c32", size = 27478, upload-time = "2026-08-02T03:32:49.348Z" }, +] + [[package]] name = "exceptiongroup" version = "1.3.1" @@ -83,13 +95,28 @@ source = { editable = "." } [package.dev-dependencies] test = [ + { name = "dunamai" }, { name = "pytest" }, + { name = "setuptools" }, ] [package.metadata] [package.metadata.requires-dev] -test = [{ name = "pytest", specifier = ">=8.4.2" }] +test = [ + { name = "dunamai" }, + { name = "pytest", specifier = ">=8.4.2" }, + { name = "setuptools" }, +] + +[[package]] +name = "setuptools" +version = "84.0.0" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/6d/44/f5da03a8ef95d369145c5bb53050e7877c9f3d312e128605fd9504829143/setuptools-84.0.0.tar.gz", hash = "sha256:f4695c21257f0d9b537ec2692c941d02ee143b7cc1276941349a546573b2ef73", size = 1168449, upload-time = "2026-08-08T18:27:58.365Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/95/9c/c510029fc6ef33a6275cd2c5d3cecd6613dfd6aa401d57c54f1c18852ccf/setuptools-84.0.0-py3-none-any.whl", hash = "sha256:51a52592b3b99e102b609654876bd65f19f999935166d1352678931132b0c670", size = 818216, upload-time = "2026-08-08T18:27:56.719Z" }, +] [[package]] name = "tomli" From 6e71040f1bb1918bd0958a7da7f7282cdede24b1 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:38:44 -0400 Subject: [PATCH 17/21] test: support x86 buffer export mocks --- tests/buffer_exports_mock.c | 10 ++++++++++ tests/test_buffer_exports.py | 3 ++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/buffer_exports_mock.c b/tests/buffer_exports_mock.c index 2541fb98..304d9d69 100644 --- a/tests/buffer_exports_mock.c +++ b/tests/buffer_exports_mock.c @@ -12,6 +12,16 @@ static int mode; API void set_mode(int value) { mode = value; } #ifndef OMIT_BUFFER_APIS +/* The loader requests undecorated API names, including on 32-bit Windows. */ +#if defined(_WIN32) && defined(_M_IX86) +#pragma comment(linker, "/export:icsneoUartWrite=_icsneoUartWrite@24") +#pragma comment(linker, "/export:icsneoGenericAPISendCommand=_icsneoGenericAPISendCommand@28") +#pragma comment(linker, "/export:icsneoGetDeviceStatus=_icsneoGetDeviceStatus@12") +#pragma comment(linker, "/export:icsneoGetHWFirmwareInfo=_icsneoGetHWFirmwareInfo@8") +#pragma comment(linker, "/export:icsneoGetDLLFirmwareInfo=_icsneoGetDLLFirmwareInfo@8") +#pragma comment(linker, "/export:icsneoFlashAccessoryFirmware=_icsneoFlashAccessoryFirmware@12") +#endif + /* No hardware calls or file access: mode 1 fails, mode 2 reports a mismatch. */ API int CALL icsneoUartWrite(void* h, int port, const void* data, size_t len, size_t* sent, unsigned char* flags) diff --git a/tests/test_buffer_exports.py b/tests/test_buffer_exports.py index 235df8c7..a007e089 100644 --- a/tests/test_buffer_exports.py +++ b/tests/test_buffer_exports.py @@ -25,7 +25,8 @@ def mock_libraries(tmp_path_factory): pytest.skip("LLVM clang-cl and lld-link are required for the mock DLL") library = directory / (name + ".dll") obj = directory / (name + ".obj") - subprocess.run([compiler, "/nologo", "/c", "/GS-", "/Zl", *defines, + target = "x86_64-pc-windows-msvc" if sys.maxsize > 2**32 else "i686-pc-windows-msvc" + subprocess.run([compiler, "--target=" + target, "/nologo", "/c", "/GS-", "/Zl", *defines, str(source), "/Fo" + str(obj)], check=True, capture_output=True) subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", "/out:" + str(library), str(obj)], check=True, capture_output=True) From 12bca98ce07d879fb735714ac666a38e379cf9e7 Mon Sep 17 00:00:00 2001 From: David Rebbe Date: Tue, 15 Sep 2026 23:40:33 -0400 Subject: [PATCH 18/21] test: match script mock to Windows architecture --- tests/test_script_load.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/test_script_load.py b/tests/test_script_load.py index 2e72feb3..4f145bbc 100644 --- a/tests/test_script_load.py +++ b/tests/test_script_load.py @@ -3,6 +3,7 @@ import os from pathlib import Path import shutil +import struct import subprocess import sys @@ -37,16 +38,20 @@ def script_library(tmp_path_factory): if not compiler or not linker: pytest.skip("LLVM is required for the mock DLL") library = root / "mock.dll" - subprocess.run([compiler, "/nologo", "/c", "/GS-", "/Zl", str(source), - f"/Fo{root / 'mock.obj'}"], check=True, capture_output=True) + is_32bit = struct.calcsize("P") == 4 + target = "i686-pc-windows-msvc" if is_32bit else "x86_64-pc-windows-msvc" + subprocess.run([compiler, f"--target={target}", "/nologo", "/c", "/GS-", "/Zl", str(source), + f"/Fo{root / 'mock.obj'}"], check=True, capture_output=True, timeout=60) + aliases = (["/export:icsneoScriptLoad=_icsneoScriptLoad@16", + "/export:icsneoScriptLoadReadBin=_icsneoScriptLoadReadBin@16"] if is_32bit else []) subprocess.run([linker, "/dll", "/noentry", "/nodefaultlib", f"/out:{library}", - str(root / "mock.obj")], check=True, capture_output=True) + str(root / "mock.obj"), *aliases], check=True, capture_output=True, timeout=60) else: compiler = shutil.which("cc") if not compiler: pytest.skip("C compiler is required for the mock library") library = root / "mock.so" - subprocess.run([compiler, "-shared", "-fPIC", str(source), "-o", str(library)], check=True) + subprocess.run([compiler, "-shared", "-fPIC", str(source), "-o", str(library)], check=True, timeout=60) return library @@ -132,5 +137,5 @@ class Counters(ctypes.Structure): raise AssertionError("missing file accepted") ''' result = subprocess.run([sys.executable, "-c", script, str(script_library), loader, kind, - str(failure), str(tmp_path)], env=env, capture_output=True, text=True) + str(failure), str(tmp_path)], env=env, capture_output=True, text=True, timeout=60) 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 19/21] 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 20/21] 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 21/21] 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()