[Bugfix] PyObject_GetAttrString return values leaked across 60+ call sites #267

New issue

Closed

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

john commented

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

Owner

Summary

PyObject_GetAttrString() returns a new reference that must be Py_DECREF'd when no longer needed. Across the codebase, 60+ call sites store the result in a local variable, use it once (typically as a type for tp_alloc or PyObject_IsInstance), and never decrement the reference. Each call leaks a reference to a type object.

Root Cause

The pattern appears throughout UIEntity.cpp, UIGrid.cpp, UIEntityCollection.cpp, and other files:

// UIEntity.cpp:298 — leaks reference to Vector type
auto type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Vector");
auto obj = (PyVectorObject*)type->tp_alloc(type, 0);
// type is never Py_DECREF'd

// UIEntity.cpp:214 — leaks reference to Texture type
if (!PyObject_IsInstance(texture, PyObject_GetAttrString(McRFPy_API::mcrf_module, "Texture"))) {
// Return value used inline and immediately leaked

// UIGrid.cpp:1418 — leaks reference to Color type
auto type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Color");

Affected Files (non-exhaustive)

UIEntity.cpp — ~8 calls, ~6 leaked
UIGrid.cpp — ~26 calls, majority leaked
UIEntityCollection.cpp — several calls
Various getter/setter functions for Color, Vector, Texture properties

Impact

Each leaked call increments the type object's refcount permanently. While type objects are singletons (so no actual memory is freed), it:

Prevents clean interpreter shutdown
Masks real reference counting bugs (makes refcount debugging unreliable)
The inline PyObject_IsInstance(value, PyObject_GetAttrString(...)) pattern is particularly bad — it leaks on every property set call

Fix

Since these types are accessed repeatedly and are module-level singletons, the best fix is to use the inline PyTypeObject declarations directly instead of runtime attribute lookup:

// BEFORE (leaks):
auto type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Vector");

// AFTER (no leak, faster):
auto type = &mcrfpydef::PyVectorType;

This was already done for Entity types (&mcrfpydef::PyUIEntityType) but not consistently for Vector, Color, Texture, etc. The inline PyTypeObject declarations (documented in MEMORY.md) make this safe from any .cpp file.

For PyObject_IsInstance checks:

// BEFORE (leaks):
if (!PyObject_IsInstance(value, PyObject_GetAttrString(McRFPy_API::mcrf_module, "Texture")))

// AFTER (no leak):
if (!PyObject_IsInstance(value, (PyObject*)&mcrfpydef::PyTextureType))

Severity

High — memory/reference leak on every property access and object creation. No crash, but contributes to memory growth and prevents clean shutdown.

## Summary `PyObject_GetAttrString()` returns a **new reference** that must be `Py_DECREF`'d when no longer needed. Across the codebase, 60+ call sites store the result in a local variable, use it once (typically as a type for `tp_alloc` or `PyObject_IsInstance`), and never decrement the reference. Each call leaks a reference to a type object. ## Root Cause The pattern appears throughout UIEntity.cpp, UIGrid.cpp, UIEntityCollection.cpp, and other files: ```cpp // UIEntity.cpp:298 — leaks reference to Vector type auto type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Vector"); auto obj = (PyVectorObject*)type->tp_alloc(type, 0); // type is never Py_DECREF'd // UIEntity.cpp:214 — leaks reference to Texture type if (!PyObject_IsInstance(texture, PyObject_GetAttrString(McRFPy_API::mcrf_module, "Texture"))) { // Return value used inline and immediately leaked // UIGrid.cpp:1418 — leaks reference to Color type auto type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Color"); ``` ## Affected Files (non-exhaustive) - `UIEntity.cpp` — ~8 calls, ~6 leaked - `UIGrid.cpp` — ~26 calls, majority leaked - `UIEntityCollection.cpp` — several calls - Various getter/setter functions for Color, Vector, Texture properties ## Impact Each leaked call increments the type object's refcount permanently. While type objects are singletons (so no actual memory is freed), it: 1. Prevents clean interpreter shutdown 2. Masks real reference counting bugs (makes refcount debugging unreliable) 3. The inline `PyObject_IsInstance(value, PyObject_GetAttrString(...))` pattern is particularly bad — it leaks on every property set call ## Fix Since these types are accessed repeatedly and are module-level singletons, the best fix is to **use the `inline` PyTypeObject declarations directly** instead of runtime attribute lookup: ```cpp // BEFORE (leaks): auto type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Vector"); // AFTER (no leak, faster): auto type = &mcrfpydef::PyVectorType; ``` This was already done for Entity types (`&mcrfpydef::PyUIEntityType`) but not consistently for Vector, Color, Texture, etc. The `inline` PyTypeObject declarations (documented in MEMORY.md) make this safe from any .cpp file. For `PyObject_IsInstance` checks: ```cpp // BEFORE (leaks): if (!PyObject_IsInstance(value, PyObject_GetAttrString(McRFPy_API::mcrf_module, "Texture"))) // AFTER (no leak): if (!PyObject_IsInstance(value, (PyObject*)&mcrfpydef::PyTextureType)) ``` ## Severity **High** — memory/reference leak on every property access and object creation. No crash, but contributes to memory growth and prevents clean shutdown.