Skip to content

Prevent ExtraDataPtr reads beyond payload capacity #261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
drebbe-intrepid wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Prevent ExtraDataPtr reads beyond payload capacity #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b3b0101
fix: handle SpyMessage attribute deletion
drebbe-intrepid Sep 16, 2026
e56291f
fix: validate transmit message types
drebbe-intrepid Sep 16, 2026
17e08af
fix: release helper and attribute references
drebbe-intrepid Sep 16, 2026
78642dd
test: export mock APIs on 32-bit Windows
drebbe-intrepid Sep 16, 2026
471d658
fix: bound message payload reads by capacity
drebbe-intrepid Sep 16, 2026
f0a25f9
test: compare capsule ownership portably
drebbe-intrepid Sep 16, 2026
1d4a2c0
fix: exclude subclass callbacks from rollback
drebbe-intrepid Sep 16, 2026
7f5bdf0
fix: integrate reviewed message dependencies
drebbe-intrepid Sep 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/object_spy_message.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -47,6 +49,7 @@ typedef struct
{
PyObject_HEAD icsSpyMessageJ1850 msg;
bool noExtraDataPtrCleanup;
size_t extraDataCapacity;
} spy_message_j1850_object;
#pragma pack(pop)

Expand All @@ -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_
81 changes: 58 additions & 23 deletions src/methods.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@
#include <sstream>
#include <string>

// Own a Python reference while the GIL is held, including on early returns.
struct PyObjectDecref
{
void operator()(PyObject* object) const { Py_XDECREF(object); }
};
using PyObjectRef = std::unique_ptr<PyObject, PyObjectDecref>;

// This class allows RAII of the python GIL. This is a C++ replacement of
// Py_BEGIN_ALLOW_THREADS / Py_END_ALLOW_THREADS
class PyAllowThreads
Expand Down Expand Up @@ -794,12 +801,12 @@ bool _convertListOrTupleToArray(PyObject* obj, std::vector<PyObject*>* results)
PyObject* _getPythonModuleObject(const char* module_name, const char* module_object_name)
{
// Before we do anything, we need to grab the python s_device_settings ctype.Structure.
PyObject* module = PyImport_ImportModule(module_name);
PyObjectRef module(PyImport_ImportModule(module_name));
if (!module) {
return set_ics_exception(exception_runtime_error(), "_getPythonModuleObject(): Failed to import module");
}
// Grab the module Dictionary
PyObject* module_dict = PyModule_GetDict(module);
PyObject* module_dict = PyModule_GetDict(module.get());
if (!module_dict) {
return set_ics_exception(exception_runtime_error(),
"_getPythonModuleObject(): Failed to grab module dict from module");
Expand All @@ -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");
Expand Down Expand Up @@ -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;
}
Expand All @@ -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 {
Expand Down Expand Up @@ -1771,6 +1778,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);
Expand All @@ -1782,6 +1800,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) {
Expand All @@ -1791,20 +1822,12 @@ PyObject* meth_transmit_messages(PyObject* self, PyObject* args)
ice::Function<int __stdcall(void*, icsSpyMessage*, int, int)> icsneoTxMessages(lib, "icsneoTxMessages");
const Py_ssize_t TUPLE_COUNT = PyTuple_Size(tuple);
icsSpyMessage** msgs = new icsSpyMessage*[static_cast<size_t>(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<Py_ssize_t>(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) {
Expand Down Expand Up @@ -1901,6 +1924,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);
Expand Down Expand Up @@ -2726,6 +2750,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());
Expand Down Expand Up @@ -2777,7 +2802,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");
Expand All @@ -2804,6 +2829,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());
Expand Down Expand Up @@ -2844,6 +2871,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) {
Expand Down Expand Up @@ -2883,6 +2912,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) {
Expand Down
Loading
Loading