Fix 32/64-bit type mismatches, integer overflows, and SpyMessage memory corruption#233
Open
drebbe-intrepid wants to merge 4 commits into
Open
Fix 32/64-bit type mismatches, integer overflows, and SpyMessage memory corruption#233drebbe-intrepid wants to merge 4 commits into
drebbe-intrepid wants to merge 4 commits into
Conversation
…dlers
- Header setter wrote up to 8 bytes into the 4-byte J1850 Header array,
spilling into Data[0..3]; tuple lengths are now validated against the
real field sizes (ValueError instead of silent corruption)
- ExtraDataPtr setter truncated the tuple length through uint16_t and
through NumberBytesData for non length-packing protocols; lengths are
now range checked per protocol
- Broken null checks (!data && !PyLong_Check(data)) could dereference
NULL and leaked pending TypeErrors into later calls (surfacing as
SystemError); element conversion errors now propagate immediately
- getattr("ExtraDataPtr") returned Py_None without incref, underflowing
None's refcount on Python < 3.12
- ExtraDataPtr setter could delete[] DLL-owned buffers on received
messages (noExtraDataPtrCleanup) and the ExtraDataPtrEnabled path left
a dangling pointer behind after freeing (double free in dealloc)
- Removed T_OBJECT_EX member entries that exposed descriptors
reinterpreting raw struct memory as PyObject* (reachable via
type(msg).__dict__) and fixed cross-type offsetof in the J1850 table
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- find_devices: scalar new unsigned int(N) allocated one element instead
of an array (heap overflow with more than one device type, delete[] on
scalar new); device type masks like NEODEVICE_ALL now convert through
PyLong_AsUnsignedLongMask with error checks instead of overflowing
PyLong_AsLong on Windows
- get/set_device_settings: 'k' wrote a full unsigned long (8 bytes on
Linux/macOS x64) into the 4-byte EPlasmaIonVnetChannel_t enum,
corrupting the stack; parse with 'I' and cast
- get_accessory_firmware_version: 'i' wrote 4 bytes into a 1-byte char
index (now int with a 0..255 range check)
- uart_write: 'p' wrote a full int into a 1-byte bool (now int); return
bytesActuallySent via 'n' instead of truncating size_t through 'i'
- get_serial_number: serials >= 0x80000000 ("ZIK0ZK".."ZZZZZZ") came
back negative through Py_BuildValue("i"); use 'I'
- script_get_script_status: sizeof(parameters)/sizeof(¶meters[0])
divided by pointer size, reporting 127 instead of 255 entries on
Win64; elements returned via PyLong_FromUnsignedLong instead of a
signed truncating 'i' (also fixes a per-element reference leak)
- read_jupiter_firmware: 'i' wrote 4 of size_t's 8 bytes and rejected
sizes >= 2GB; parse/return with 'n'
- coremini load / read_bin: validate ftell() results (32-bit long on
Windows) before malloc
- get_rtc: return the __int64 offset via 'L' instead of casting through
unsigned long and 'i'
- open_device: check PyLong_AsUnsignedLong errors for integer and
base36 serial arguments (previously left pending exceptions and an
0xFFFFFFFF serial), free the int() conversion result
- reflash progress callback: pass unsigned long progress with 'k'
- FlashDevice2 (internal builds): avoid sign-extending the -1 network
sentinel to 64 bits on LP64
- byte-sized 'b' parse targets declared as int/unsigned int now use
unsigned char (single byte writes into 4-byte stack slots)
- get_bus_voltage / vnet channel getters: return unsigned long values
via 'k' instead of 'i'
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ics.DeviceSettingsNone was -1 on Windows (MSVC wraps the 0xFFFFFFFF enumerator to a signed int) but 4294967295 on Linux/macOS and in the generated Python enum, so settings.DeviceSettingType comparisons gave different answers per platform. - extract_icsneo40_defines.py: enum members whose header value is a hex literal above 0x7FFFFFFF are now emitted through PyLong_FromUnsignedLongLong(static_cast<unsigned int>(...)) so every platform exposes the positive value; intentionally negative enumerators (= -1) are unaffected - generate_icsneo40_structs.py: struct fields of enum types with an enumerator above 0x7FFFFFFF are emitted as ctypes.c_uint32 instead of c_int32 so the value round-trips (s_device_settings.DeviceSettingType) - generate_icsneo40_structs.py: descIdType is int16_t in icsnVC40.h, not uint16_t; DescriptionID fields are now signed and consistent with ics_spy_message_vsb - regenerate src/setup_module_auto_defines.cpp - tests: pin DeviceSettingsNone, DeviceSettingType round-trip and DescriptionID signedness; drop the hasattr() guards in test_defines so missing constants fail loudly, and pin the remaining high-bit defines and the serial range constants Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NeoDevice.SerialNumber is an int32_t, so serials "ZIK0ZK".."ZZZZZZ" read back negative and the property returned a negative decimal string. Mask to 32 bits before the range checks, and use < for the MIN_BASE36_SERIAL boundary so "A0000" (the first base36 serial) renders as base36 instead of decimal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
@pierreluctg I had this in progress for a little bit. I had AI audit the code base and it caught what you fixed in #232 before you submitted it. I just need to do a deep dive review of the code before I merge it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows up the audit that motivated #232/#231. Includes the
find_devicesnew unsigned int[N]fix from #232 and hardens the same loop(PyLong_AsUnsignedLongMask + error check so NEODEVICE_ALL / 0x80000000 masks
don't raise OverflowError on Windows).
SpyMessage attribute handlers (object_spy_message.cpp)
spilling into Data[0..3]; lengths now validated per field (ValueError)
range-checked per protocol
!data && !PyLong_Check(data)checks could deref NULL and leakedpending TypeErrors into later calls
underflow on Python < 3.12)
dangling pointer (double free in dealloc)
memory; fixed cross-type offsetof in the J1850 table
32/64-bit and overflow fixes (methods.cpp)
1-byte bool
widened to correct format chars
Cross-platform constant normalization (generators)
members > 0x7FFFFFFF now emitted as positive, struct field made c_uint32
Adds tests: test_spy_message.py, test_neo_device_ex.py, test_structures.py;
hardens test_defines.py. All 26 pass locally on win_amd64 / CPython 3.14.