Fix UniformCollection owner validity check (closes #272)

UniformCollection accessor methods checked the raw collection pointer
but never checked if the owning object was still alive. Changed owner
field from weak_ptr<UIDrawable> to weak_ptr<void> (type-erased) so
both UIDrawable and UIEntity owners can be tracked. Set owner in both
get_uniforms() paths. All accessors now check owner.lock() before
dereferencing the raw collection pointer.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
John McCardle 2026-03-08 17:07:14 -04:00
commit 34c84ce50a
4 changed files with 37 additions and 14 deletions

View file

@ -151,7 +151,7 @@ PyObject* PyUniformCollectionType::repr(PyObject* obj) {
PyUniformCollectionObject* self = (PyUniformCollectionObject*)obj;
std::ostringstream ss;
ss << "<UniformCollection";
if (self->collection) {
if (self->owner.lock() && self->collection) {
auto names = self->collection->getNames();
ss << " [";
for (size_t i = 0; i < names.size(); ++i) {
@ -166,15 +166,15 @@ PyObject* PyUniformCollectionType::repr(PyObject* obj) {
Py_ssize_t PyUniformCollectionType::mp_length(PyObject* obj) {
PyUniformCollectionObject* self = (PyUniformCollectionObject*)obj;
if (!self->collection) return 0;
if (!self->owner.lock() || !self->collection) return 0;
return static_cast<Py_ssize_t>(self->collection->getNames().size());
}
PyObject* PyUniformCollectionType::mp_subscript(PyObject* obj, PyObject* key) {
PyUniformCollectionObject* self = (PyUniformCollectionObject*)obj;
if (!self->collection) {
PyErr_SetString(PyExc_RuntimeError, "UniformCollection is not valid");
if (!self->owner.lock() || !self->collection) {
PyErr_SetString(PyExc_RuntimeError, "UniformCollection is not valid (owner destroyed)");
return NULL;
}
@ -239,8 +239,8 @@ PyObject* PyUniformCollectionType::mp_subscript(PyObject* obj, PyObject* key) {
int PyUniformCollectionType::mp_ass_subscript(PyObject* obj, PyObject* key, PyObject* value) {
PyUniformCollectionObject* self = (PyUniformCollectionObject*)obj;
if (!self->collection) {
PyErr_SetString(PyExc_RuntimeError, "UniformCollection is not valid");
if (!self->owner.lock() || !self->collection) {
PyErr_SetString(PyExc_RuntimeError, "UniformCollection is not valid (owner destroyed)");
return -1;
}
@ -330,7 +330,7 @@ int PyUniformCollectionType::mp_ass_subscript(PyObject* obj, PyObject* key, PyOb
int PyUniformCollectionType::sq_contains(PyObject* obj, PyObject* key) {
PyUniformCollectionObject* self = (PyUniformCollectionObject*)obj;
if (!self->collection) return 0;
if (!self->owner.lock() || !self->collection) return 0;
if (!PyUnicode_Check(key)) return 0;
@ -339,7 +339,7 @@ int PyUniformCollectionType::sq_contains(PyObject* obj, PyObject* key) {
}
PyObject* PyUniformCollectionType::keys(PyUniformCollectionObject* self, PyObject* Py_UNUSED(ignored)) {
if (!self->collection) {
if (!self->owner.lock() || !self->collection) {
return PyList_New(0);
}
@ -352,7 +352,7 @@ PyObject* PyUniformCollectionType::keys(PyUniformCollectionObject* self, PyObjec
}
PyObject* PyUniformCollectionType::values(PyUniformCollectionObject* self, PyObject* Py_UNUSED(ignored)) {
if (!self->collection) {
if (!self->owner.lock() || !self->collection) {
return PyList_New(0);
}
@ -372,7 +372,7 @@ PyObject* PyUniformCollectionType::values(PyUniformCollectionObject* self, PyObj
}
PyObject* PyUniformCollectionType::items(PyUniformCollectionObject* self, PyObject* Py_UNUSED(ignored)) {
if (!self->collection) {
if (!self->owner.lock() || !self->collection) {
return PyList_New(0);
}
@ -395,7 +395,7 @@ PyObject* PyUniformCollectionType::items(PyUniformCollectionObject* self, PyObje
}
PyObject* PyUniformCollectionType::clear(PyUniformCollectionObject* self, PyObject* Py_UNUSED(ignored)) {
if (self->collection) {
if (self->owner.lock() && self->collection) {
auto names = self->collection->getNames();
for (const auto& name : names) {
self->collection->remove(name);

View file

@ -83,8 +83,8 @@ private:
// Python object structure for UniformCollection
typedef struct {
PyObject_HEAD
UniformCollection* collection; // Owned by UIDrawable, not by this object
std::weak_ptr<UIDrawable> owner; // For checking validity
UniformCollection* collection; // Owned by UIDrawable/UIEntity, not by this object
std::weak_ptr<void> owner; // Type-erased weak ref for validity checking
PyObject* weakreflist;
} PyUniformCollectionObject;

View file

@ -41,6 +41,28 @@ static UIDrawable* extractDrawable(PyObject* self, PyObjectsEnum objtype) {
}
}
// Helper to extract shared_ptr<UIDrawable> for ownership tracking
static std::shared_ptr<UIDrawable> extractDrawableShared(PyObject* self, PyObjectsEnum objtype) {
switch (objtype) {
case PyObjectsEnum::UIFRAME:
return ((PyUIFrameObject*)self)->data;
case PyObjectsEnum::UICAPTION:
return ((PyUICaptionObject*)self)->data;
case PyObjectsEnum::UISPRITE:
return ((PyUISpriteObject*)self)->data;
case PyObjectsEnum::UIGRID:
return ((PyUIGridObject*)self)->data;
case PyObjectsEnum::UILINE:
return ((PyUILineObject*)self)->data;
case PyObjectsEnum::UICIRCLE:
return ((PyUICircleObject*)self)->data;
case PyObjectsEnum::UIARC:
return ((PyUIArcObject*)self)->data;
default:
return nullptr;
}
}
UIDrawable::UIDrawable() : position(0.0f, 0.0f) { click_callable = NULL; }
UIDrawable::UIDrawable(const UIDrawable& other)
@ -1271,7 +1293,7 @@ PyObject* UIDrawable::get_uniforms(PyObject* self, void* closure) {
collection->collection = drawable->uniforms.get();
collection->weakreflist = NULL;
// Note: owner weak_ptr could be set here if we had access to shared_ptr
collection->owner = extractDrawableShared(self, objtype);
return (PyObject*)collection;
}

View file

@ -140,6 +140,7 @@ static PyObject* UIEntity_get_uniforms(PyUIEntityObject* self, void* closure)
// The collection is owned by the sprite, we just provide a view
uniforms_obj->collection = self->data->sprite.uniforms.get();
uniforms_obj->owner = self->data; // Entity shared_ptr keeps collection alive
uniforms_obj->weakreflist = NULL;
return (PyObject*)uniforms_obj;