diff --git a/src/PyDrawable.cpp b/src/PyDrawable.cpp index 61a8151..849409f 100644 --- a/src/PyDrawable.cpp +++ b/src/PyDrawable.cpp @@ -10,10 +10,11 @@ static PyObject* PyDrawable_get_click(PyDrawableObject* self, void* closure) Py_RETURN_NONE; PyObject* ptr = self->data->click_callable->borrow(); - if (ptr && ptr != Py_None) + if (ptr && ptr != Py_None) { + Py_INCREF(ptr); // Return new reference, not borrowed return ptr; - else - Py_RETURN_NONE; + } + Py_RETURN_NONE; } // Click property setter diff --git a/src/UIDrawable.cpp b/src/UIDrawable.cpp index a1fe1cd..a3aff1a 100644 --- a/src/UIDrawable.cpp +++ b/src/UIDrawable.cpp @@ -259,10 +259,11 @@ PyObject* UIDrawable::get_click(PyObject* self, void* closure) { PyErr_SetString(PyExc_TypeError, "no idea how you did that; invalid UIDrawable derived instance for _get_click"); return NULL; } - if (ptr && ptr != Py_None) + if (ptr && ptr != Py_None) { + Py_INCREF(ptr); // Return new reference, not borrowed return ptr; - else - return Py_None; + } + Py_RETURN_NONE; } int UIDrawable::set_click(PyObject* self, PyObject* value, void* closure) { @@ -1615,10 +1616,11 @@ PyObject* UIDrawable::get_on_enter(PyObject* self, void* closure) { PyErr_SetString(PyExc_TypeError, "Invalid UIDrawable derived instance for on_enter"); return NULL; } - if (ptr && ptr != Py_None) + if (ptr && ptr != Py_None) { + Py_INCREF(ptr); // Return new reference, not borrowed return ptr; - else - Py_RETURN_NONE; + } + Py_RETURN_NONE; } int UIDrawable::set_on_enter(PyObject* self, PyObject* value, void* closure) { @@ -1698,10 +1700,11 @@ PyObject* UIDrawable::get_on_exit(PyObject* self, void* closure) { PyErr_SetString(PyExc_TypeError, "Invalid UIDrawable derived instance for on_exit"); return NULL; } - if (ptr && ptr != Py_None) + if (ptr && ptr != Py_None) { + Py_INCREF(ptr); // Return new reference, not borrowed return ptr; - else - Py_RETURN_NONE; + } + Py_RETURN_NONE; } int UIDrawable::set_on_exit(PyObject* self, PyObject* value, void* closure) { @@ -1790,10 +1793,11 @@ PyObject* UIDrawable::get_on_move(PyObject* self, void* closure) { PyErr_SetString(PyExc_TypeError, "Invalid UIDrawable derived instance for on_move"); return NULL; } - if (ptr && ptr != Py_None) + if (ptr && ptr != Py_None) { + Py_INCREF(ptr); // Return new reference, not borrowed return ptr; - else - Py_RETURN_NONE; + } + Py_RETURN_NONE; } int UIDrawable::set_on_move(PyObject* self, PyObject* value, void* closure) { diff --git a/tests/regression/issue_callback_refcount_test.py b/tests/regression/issue_callback_refcount_test.py new file mode 100644 index 0000000..da4b563 --- /dev/null +++ b/tests/regression/issue_callback_refcount_test.py @@ -0,0 +1,157 @@ +#!/usr/bin/env python3 +"""Test for callback property reference counting fix. + +This test verifies that accessing callback properties (on_click, on_enter, etc.) +returns correctly reference-counted objects, preventing use-after-free bugs. + +The bug: Callback getters were returning borrowed references instead of new +references, causing objects to be freed prematurely when Python DECREFs them. +""" +import mcrfpy +import sys +import gc + +def test_callback_refcount(): + """Test that callback getters return new references.""" + errors = [] + + # Create a scene + scene = mcrfpy.Scene("test_callback_refcount") + + # Test Frame + frame = mcrfpy.Frame(pos=(0, 0), size=(100, 100)) + + # Set a callback + def my_callback(pos, button, action): + pass + + frame.on_click = my_callback + + # Read the callback back multiple times + # If borrowing incorrectly, this could cause use-after-free + for i in range(10): + cb = frame.on_click + if cb is None: + errors.append(f"on_click returned None on iteration {i}") + break + if not callable(cb): + errors.append(f"on_click returned non-callable on iteration {i}: {type(cb)}") + break + # Explicitly delete to trigger any refcount issues + del cb + gc.collect() + + # Final check - should still return the callback + final_cb = frame.on_click + if final_cb is None: + errors.append("on_click returned None after repeated access") + elif not callable(final_cb): + errors.append(f"on_click returned non-callable after repeated access: {type(final_cb)}") + + # Test on_enter, on_exit, on_move + frame.on_enter = lambda pos, button, action: None + frame.on_exit = lambda pos, button, action: None + frame.on_move = lambda pos, button, action: None + + for name in ['on_enter', 'on_exit', 'on_move']: + for i in range(5): + cb = getattr(frame, name) + if cb is None: + errors.append(f"{name} returned None on iteration {i}") + break + del cb + gc.collect() + + return errors + + +def test_grid_cell_callbacks(): + """Test Grid cell callback getters (these were already correct).""" + errors = [] + + grid = mcrfpy.Grid(pos=(0, 0), size=(100, 100), grid_size=(5, 5), + texture=mcrfpy.default_texture, zoom=1.0) + + grid.on_cell_enter = lambda pos: None + grid.on_cell_exit = lambda pos: None + grid.on_cell_click = lambda pos: None + + for name in ['on_cell_enter', 'on_cell_exit', 'on_cell_click']: + for i in range(5): + cb = getattr(grid, name) + if cb is None: + errors.append(f"{name} returned None on iteration {i}") + break + del cb + gc.collect() + + return errors + + +def test_subclass_callback(): + """Test callback access on Python subclasses.""" + errors = [] + + class MyFrame(mcrfpy.Frame): + pass + + obj = MyFrame(pos=(0, 0), size=(100, 100)) + + # Set callback via property + obj.on_click = lambda pos, button, action: print("clicked") + + # Read back multiple times + for i in range(5): + cb = obj.on_click + if cb is None: + errors.append(f"Subclass on_click returned None on iteration {i}") + break + if not callable(cb): + errors.append(f"Subclass on_click returned non-callable: {type(cb)}") + break + del cb + gc.collect() + + return errors + + +def run_tests(): + """Run all callback refcount tests.""" + all_errors = [] + + print("Testing callback property refcount...") + errors = test_callback_refcount() + if errors: + all_errors.extend(errors) + print(f" FAIL: {len(errors)} errors") + else: + print(" PASS: on_click, on_enter, on_exit, on_move") + + print("Testing Grid cell callbacks...") + errors = test_grid_cell_callbacks() + if errors: + all_errors.extend(errors) + print(f" FAIL: {len(errors)} errors") + else: + print(" PASS: on_cell_enter, on_cell_exit, on_cell_click") + + print("Testing subclass callbacks...") + errors = test_subclass_callback() + if errors: + all_errors.extend(errors) + print(f" FAIL: {len(errors)} errors") + else: + print(" PASS: MyFrame(Frame) subclass") + + if all_errors: + print(f"\nFAILED with {len(all_errors)} errors:") + for e in all_errors: + print(f" - {e}") + sys.exit(1) + else: + print("\nAll callback refcount tests PASSED") + sys.exit(0) + + +if __name__ == "__main__": + run_tests()