[Bugfix] UniformCollection stores raw pointer without checking weak_ptr owner before access #272

New issue

Closed

opened 2026-03-07 23:23:00 +00:00 by john · 0 comments

john commented

2026-03-07 23:23:00 +00:00

Owner

Summary

PyUniformCollectionObject stores a raw UniformCollection* pointer to memory owned by a UIDrawable, and a std::weak_ptr<UIDrawable> owner for validity checking. However, the accessor methods only check if (!self->collection) (NULL check on the raw pointer) — they never check self->owner.lock(). If the owning UIDrawable is destroyed, collection becomes a dangling pointer that passes the NULL check.

Root Cause

PyUniformCollection.h:84-89:

typedef struct {
    PyObject_HEAD
    UniformCollection* collection;  // Owned by UIDrawable, not by this object
    std::weak_ptr<UIDrawable> owner;  // For checking validity
    PyObject* weakreflist;
} PyUniformCollectionObject;

All accessor methods check the raw pointer but not the weak_ptr:

PyUniformCollection.cpp:176 (mp_subscript):

if (!self->collection) {    // Only checks if pointer is NULL
    PyErr_SetString(PyExc_RuntimeError, "UniformCollection is not valid");
    return NULL;
}
// Proceeds to dereference self->collection — but it could be dangling

Same pattern in:

mp_length (line 169)
mp_ass_subscript (line 242)
repr (line 154)

The owner weak_ptr exists precisely for validity checking but is never used in any accessor method.

Reproduction

import mcrfpy

frame = mcrfpy.Frame(pos=(0,0), size=(100,100))
uniforms = frame.uniforms  # Gets UniformCollection with raw ptr

del frame  # UIDrawable destroyed, collection memory freed

uniforms["test"] = 1.0  # Passes NULL check but dereferences freed memory

Fix

Check owner.lock() before accessing the raw pointer in all methods:

PyObject* PyUniformCollectionType::mp_subscript(PyObject* obj, PyObject* key) {
    PyUniformCollectionObject* self = (PyUniformCollectionObject*)obj;
    
    if (!self->owner.lock() || !self->collection) {
        PyErr_SetString(PyExc_RuntimeError, "UniformCollection is not valid (owner destroyed)");
        return NULL;
    }
    // ... safe to use self->collection
}

Severity

Medium — use-after-free when UIDrawable is destroyed while a Python reference to its uniforms persists. Requires specific code pattern to trigger but leads to undefined behavior.

## Summary `PyUniformCollectionObject` stores a raw `UniformCollection*` pointer to memory owned by a `UIDrawable`, and a `std::weak_ptr<UIDrawable> owner` for validity checking. However, the accessor methods only check `if (!self->collection)` (NULL check on the raw pointer) — they never check `self->owner.lock()`. If the owning UIDrawable is destroyed, `collection` becomes a dangling pointer that passes the NULL check. ## Root Cause `PyUniformCollection.h:84-89`: ```cpp typedef struct { PyObject_HEAD UniformCollection* collection; // Owned by UIDrawable, not by this object std::weak_ptr<UIDrawable> owner; // For checking validity PyObject* weakreflist; } PyUniformCollectionObject; ``` All accessor methods check the raw pointer but not the weak_ptr: `PyUniformCollection.cpp:176` (mp_subscript): ```cpp if (!self->collection) { // Only checks if pointer is NULL PyErr_SetString(PyExc_RuntimeError, "UniformCollection is not valid"); return NULL; } // Proceeds to dereference self->collection — but it could be dangling ``` Same pattern in: - `mp_length` (line 169) - `mp_ass_subscript` (line 242) - `repr` (line 154) The `owner` weak_ptr exists precisely for validity checking but is never used in any accessor method. ## Reproduction ```python import mcrfpy frame = mcrfpy.Frame(pos=(0,0), size=(100,100)) uniforms = frame.uniforms # Gets UniformCollection with raw ptr del frame # UIDrawable destroyed, collection memory freed uniforms["test"] = 1.0 # Passes NULL check but dereferences freed memory ``` ## Fix Check `owner.lock()` before accessing the raw pointer in all methods: ```cpp PyObject* PyUniformCollectionType::mp_subscript(PyObject* obj, PyObject* key) { PyUniformCollectionObject* self = (PyUniformCollectionObject*)obj; if (!self->owner.lock() || !self->collection) { PyErr_SetString(PyExc_RuntimeError, "UniformCollection is not valid (owner destroyed)"); return NULL; } // ... safe to use self->collection } ``` ## Severity **Medium** — use-after-free when UIDrawable is destroyed while a Python reference to its uniforms persists. Requires specific code pattern to trigger but leads to undefined behavior.