From 348826a0f5399231fbe42c7b20c77131a9fcf027 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Sat, 7 Mar 2026 22:56:16 -0500 Subject: [PATCH] Fix gridstate heap overflows and spatial hash cleanup Add ensureGridstate() helper that unconditionally checks gridstate size against current grid dimensions and resizes if mismatched. Replace all lazy-init guards (size == 0) with ensureGridstate() calls. Previously, gridstate was only initialized when empty. When an entity moved to a differently-sized grid, gridstate kept the old size, causing heap buffer overflows when updateVisibility() or at() iterated using the new grid's dimensions. Also adds spatial_hash.remove() calls in set_grid() before removing entities from old grids, and replaces PyObject_GetAttrString type lookup with direct &mcrfpydef::PyUIGridType reference. Closes #258, closes #259, closes #260, closes #261, closes #262, closes #263, closes #274, closes #276, closes #278 Co-Authored-By: Claude Opus 4.6 --- src/UIEntity.cpp | 46 ++--- src/UIEntity.h | 1 + src/UIEntityCollection.cpp | 33 +--- .../issue_258_gridstate_resize_test.py | 175 ++++++++++++++++++ tests/test_gridstate_resize.py | 78 -------- 5 files changed, 203 insertions(+), 130 deletions(-) create mode 100644 tests/regression/issue_258_gridstate_resize_test.py delete mode 100644 tests/test_gridstate_resize.py diff --git a/src/UIEntity.cpp b/src/UIEntity.cpp index 033f1cf..55bb3ac 100644 --- a/src/UIEntity.cpp +++ b/src/UIEntity.cpp @@ -33,19 +33,24 @@ UIEntity::~UIEntity() { // Removed UIEntity(UIGrid&) constructor - using lazy initialization instead -void UIEntity::updateVisibility() +void UIEntity::ensureGridstate() { if (!grid) return; - - // Lazy initialize gridstate if needed - if (gridstate.size() == 0) { - gridstate.resize(grid->grid_w * grid->grid_h); - // Initialize all cells as not visible/discovered + size_t expected = static_cast(grid->grid_w) * grid->grid_h; + if (gridstate.size() != expected) { + gridstate.resize(expected); for (auto& state : gridstate) { state.visible = false; state.discovered = false; } } +} + +void UIEntity::updateVisibility() +{ + if (!grid) return; + + ensureGridstate(); // First, mark all cells as not visible for (auto& state : gridstate) { @@ -108,15 +113,7 @@ PyObject* UIEntity::at(PyUIEntityObject* self, PyObject* args, PyObject* kwds) { return NULL; } - // Lazy initialize gridstate if needed - if (self->data->gridstate.size() == 0) { - self->data->gridstate.resize(self->data->grid->grid_w * self->data->grid->grid_h); - // Initialize all cells as not visible/discovered - for (auto& state : self->data->gridstate) { - state.visible = false; - state.discovered = false; - } - } + self->data->ensureGridstate(); // Bounds check if (x < 0 || x >= self->data->grid->grid_w || y < 0 || y >= self->data->grid->grid_h) { @@ -662,6 +659,8 @@ int UIEntity::set_grid(PyUIEntityObject* self, PyObject* value, void* closure) // Handle None - remove from current grid if (value == Py_None) { if (self->data->grid) { + // Remove from spatial hash before removing from entity list + self->data->grid->spatial_hash.remove(self->data); // Remove from current grid's entity list auto& entities = self->data->grid->entities; auto it = std::find_if(entities->begin(), entities->end(), @@ -677,11 +676,7 @@ int UIEntity::set_grid(PyUIEntityObject* self, PyObject* value, void* closure) } // Value must be a Grid - PyTypeObject* grid_type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Grid"); - bool is_grid = grid_type && PyObject_IsInstance(value, (PyObject*)grid_type); - Py_XDECREF(grid_type); - - if (!is_grid) { + if (!PyObject_IsInstance(value, (PyObject*)&mcrfpydef::PyUIGridType)) { PyErr_SetString(PyExc_TypeError, "grid must be a Grid or None"); return -1; } @@ -690,6 +685,7 @@ int UIEntity::set_grid(PyUIEntityObject* self, PyObject* value, void* closure) // Remove from old grid first (if any) if (self->data->grid && self->data->grid != new_grid) { + self->data->grid->spatial_hash.remove(self->data); auto& old_entities = self->data->grid->entities; auto it = std::find_if(old_entities->begin(), old_entities->end(), [self](const std::shared_ptr& e) { @@ -705,14 +701,8 @@ int UIEntity::set_grid(PyUIEntityObject* self, PyObject* value, void* closure) new_grid->entities->push_back(self->data); self->data->grid = new_grid; - // Initialize gridstate if needed - if (self->data->gridstate.size() == 0) { - self->data->gridstate.resize(new_grid->grid_w * new_grid->grid_h); - for (auto& state : self->data->gridstate) { - state.visible = false; - state.discovered = false; - } - } + // Resize gridstate to match new grid dimensions + self->data->ensureGridstate(); } return 0; diff --git a/src/UIEntity.h b/src/UIEntity.h index f7143ce..4a926a4 100644 --- a/src/UIEntity.h +++ b/src/UIEntity.h @@ -73,6 +73,7 @@ public: ~UIEntity(); // Visibility methods + void ensureGridstate(); // Resize gridstate to match current grid dimensions void updateVisibility(); // Update gridstate from current FOV // Property system for animations diff --git a/src/UIEntityCollection.cpp b/src/UIEntityCollection.cpp index cf49e9c..a9207e7 100644 --- a/src/UIEntityCollection.cpp +++ b/src/UIEntityCollection.cpp @@ -190,6 +190,7 @@ int UIEntityCollection::setitem(PyUIEntityCollectionObject* self, Py_ssize_t ind // Replace the element and set grid reference *it = entity->data; entity->data->grid = self->grid; + entity->data->ensureGridstate(); // Add to spatial hash if (self->grid) { @@ -492,6 +493,7 @@ int UIEntityCollection::ass_subscript(PyUIEntityCollectionObject* self, PyObject for (const auto& entity : new_items) { self->data->insert(insert_pos, entity); entity->grid = self->grid; + entity->ensureGridstate(); if (self->grid) { self->grid->spatial_hash.insert(entity); } @@ -518,6 +520,7 @@ int UIEntityCollection::ass_subscript(PyUIEntityCollectionObject* self, PyObject *cur_it = new_items[new_idx++]; (*cur_it)->grid = self->grid; + (*cur_it)->ensureGridstate(); if (self->grid) { self->grid->spatial_hash.insert(*cur_it); @@ -578,14 +581,8 @@ PyObject* UIEntityCollection::append(PyUIEntityCollectionObject* self, PyObject* } } - // Initialize gridstate if not already done - if (entity->data->gridstate.size() == 0 && self->grid) { - entity->data->gridstate.resize(self->grid->grid_w * self->grid->grid_h); - for (auto& state : entity->data->gridstate) { - state.visible = false; - state.discovered = false; - } - } + // Ensure gridstate matches current grid dimensions + entity->data->ensureGridstate(); Py_RETURN_NONE; } @@ -683,14 +680,8 @@ PyObject* UIEntityCollection::extend(PyUIEntityCollectionObject* self, PyObject* self->grid->spatial_hash.insert(entity->data); } - // Initialize gridstate if needed - if (entity->data->gridstate.size() == 0 && self->grid) { - entity->data->gridstate.resize(self->grid->grid_w * self->grid->grid_h); - for (auto& state : entity->data->gridstate) { - state.visible = false; - state.discovered = false; - } - } + // Ensure gridstate matches current grid dimensions + entity->data->ensureGridstate(); Py_DECREF(entity); // Release the reference we held during validation } @@ -803,14 +794,8 @@ PyObject* UIEntityCollection::insert(PyUIEntityCollectionObject* self, PyObject* self->grid->spatial_hash.insert(entity->data); } - // Initialize gridstate if needed - if (entity->data->gridstate.size() == 0 && self->grid) { - entity->data->gridstate.resize(self->grid->grid_w * self->grid->grid_h); - for (auto& state : entity->data->gridstate) { - state.visible = false; - state.discovered = false; - } - } + // Ensure gridstate matches current grid dimensions + entity->data->ensureGridstate(); Py_RETURN_NONE; } diff --git a/tests/regression/issue_258_gridstate_resize_test.py b/tests/regression/issue_258_gridstate_resize_test.py new file mode 100644 index 0000000..a25ef34 --- /dev/null +++ b/tests/regression/issue_258_gridstate_resize_test.py @@ -0,0 +1,175 @@ +"""Regression test: entity gridstate must resize when moving between grids. + +Issues #258-#263, #274, #276, #278: UIEntity gridstate heap overflows. + +Bug: Gridstate was only initialized when empty (size == 0). When an entity +moved from a small grid to a larger grid via ANY transfer method, gridstate +kept the old size. Code then iterated using the new grid's dimensions, +writing past the vector's end. + +Fix: ensureGridstate() unconditionally checks gridstate.size() against +grid dimensions and resizes if they don't match. Applied to all transfer +methods: set_grid, append, extend, insert, setitem, slice assignment. + +Also tests #274: spatial_hash.remove() must be called when removing +entities from grids via set_grid(None) or set_grid(other_grid). +""" +import mcrfpy +import sys + +def test_set_grid(): + """entity.grid = new_grid resizes gridstate""" + small = mcrfpy.Grid(grid_size=(10, 10)) + large = mcrfpy.Grid(grid_size=(50, 50)) + entity = mcrfpy.Entity(grid_pos=(5, 5), grid=small) + + small.perspective = entity + small.fov_radius = 4 + entity.update_visibility() + + gs = entity.gridstate + assert len(gs) == 100, f"Expected 100, got {len(gs)}" + + entity.grid = large + gs = entity.gridstate + assert len(gs) == 2500, f"Expected 2500, got {len(gs)}" + + large.perspective = entity + large.fov_radius = 8 + entity.update_visibility() + print(" PASS: set_grid") + +def test_append(): + """grid.entities.append(entity) resizes gridstate""" + small = mcrfpy.Grid(grid_size=(10, 10)) + large = mcrfpy.Grid(grid_size=(40, 40)) + entity = mcrfpy.Entity(grid_pos=(3, 3), grid=small) + entity.update_visibility() + + gs = entity.gridstate + assert len(gs) == 100, f"Expected 100, got {len(gs)}" + + large.entities.append(entity) + gs = entity.gridstate + assert len(gs) == 1600, f"Expected 1600, got {len(gs)}" + print(" PASS: append") + +def test_extend(): + """grid.entities.extend([entity]) resizes gridstate""" + small = mcrfpy.Grid(grid_size=(10, 10)) + large = mcrfpy.Grid(grid_size=(30, 30)) + entity = mcrfpy.Entity(grid_pos=(3, 3), grid=small) + entity.update_visibility() + + large.entities.extend([entity]) + gs = entity.gridstate + assert len(gs) == 900, f"Expected 900, got {len(gs)}" + print(" PASS: extend") + +def test_insert(): + """grid.entities.insert(0, entity) resizes gridstate""" + small = mcrfpy.Grid(grid_size=(10, 10)) + large = mcrfpy.Grid(grid_size=(25, 25)) + entity = mcrfpy.Entity(grid_pos=(3, 3), grid=small) + entity.update_visibility() + + large.entities.insert(0, entity) + gs = entity.gridstate + assert len(gs) == 625, f"Expected 625, got {len(gs)}" + print(" PASS: insert") + +def test_setitem(): + """grid.entities[0] = entity resizes gridstate""" + small = mcrfpy.Grid(grid_size=(10, 10)) + large = mcrfpy.Grid(grid_size=(20, 20)) + entity = mcrfpy.Entity(grid_pos=(3, 3), grid=small) + entity.update_visibility() + + # Need a placeholder entity in large grid first + placeholder = mcrfpy.Entity(grid_pos=(0, 0), grid=large) + large.entities[0] = entity + gs = entity.gridstate + assert len(gs) == 400, f"Expected 400, got {len(gs)}" + print(" PASS: setitem") + +def test_slice_assign(): + """grid.entities[0:1] = [entity] resizes gridstate""" + small = mcrfpy.Grid(grid_size=(10, 10)) + large = mcrfpy.Grid(grid_size=(35, 35)) + entity = mcrfpy.Entity(grid_pos=(3, 3), grid=small) + entity.update_visibility() + + placeholder = mcrfpy.Entity(grid_pos=(0, 0), grid=large) + large.entities[0:1] = [entity] + gs = entity.gridstate + assert len(gs) == 1225, f"Expected 1225, got {len(gs)}" + print(" PASS: slice_assign") + +def test_update_visibility_after_transfer(): + """update_visibility works correctly after all transfer methods""" + grids = [mcrfpy.Grid(grid_size=(s, s)) for s in (5, 80, 3, 60, 10, 100)] + entity = mcrfpy.Entity(grid_pos=(2, 2), grid=grids[0]) + + for g in grids: + entity.grid = g + g.perspective = entity + g.fov_radius = 4 + entity.update_visibility() + gs = entity.gridstate + expected = g.grid_w * g.grid_h + assert len(gs) == expected, f"Expected {expected}, got {len(gs)}" + print(" PASS: update_visibility_after_transfer") + +def test_at_after_transfer(): + """entity.at(x, y) works correctly after grid transfer""" + small = mcrfpy.Grid(grid_size=(10, 10)) + large = mcrfpy.Grid(grid_size=(50, 50)) + entity = mcrfpy.Entity(grid_pos=(5, 5), grid=small) + entity.update_visibility() + + entity.grid = large + # Access a cell that would be out of bounds for the small grid + state = entity.at(30, 30) + assert state is not None + print(" PASS: at_after_transfer") + +def test_set_grid_none(): + """entity.grid = None properly removes entity (tests #274)""" + grid = mcrfpy.Grid(grid_size=(10, 10)) + entity = mcrfpy.Entity(grid_pos=(5, 5), grid=grid) + assert len(grid.entities) == 1 + entity.grid = None + assert len(grid.entities) == 0 + print(" PASS: set_grid_none") + +def test_stress(): + """Stress test: rapid grid transfers with heap churn""" + entity = mcrfpy.Entity(grid_pos=(2, 2)) + for i in range(20): + small_g = mcrfpy.Grid(grid_size=(5, 5)) + entity.grid = small_g + small_g.perspective = entity + entity.update_visibility() + + big_g = mcrfpy.Grid(grid_size=(80, 80)) + entity.grid = big_g + big_g.perspective = entity + entity.update_visibility() + + frames = [mcrfpy.Frame() for _ in range(10)] + del frames + print(" PASS: stress") + +print("Testing gridstate resize across transfer methods...") +test_set_grid() +test_append() +test_extend() +test_insert() +test_setitem() +test_slice_assign() +test_update_visibility_after_transfer() +test_at_after_transfer() +test_set_grid_none() +test_stress() +print("PASS: all gridstate resize tests passed") +sys.exit(0) diff --git a/tests/test_gridstate_resize.py b/tests/test_gridstate_resize.py deleted file mode 100644 index 73aea85..0000000 --- a/tests/test_gridstate_resize.py +++ /dev/null @@ -1,78 +0,0 @@ -"""Regression test: entity gridstate must resize when moving between grids. - -Bug: UIEntity::set_grid() only initialized gridstate when it was empty -(size == 0). When an entity moved from a small grid to a larger grid, -gridstate kept the old size. UIEntity::updateVisibility() then wrote -past the end of the vector using the new grid's dimensions, corrupting -adjacent heap memory. - -Trigger: any entity that calls update_visibility() after moving to a -larger grid. In Liber Noster this was the player entity using the -engine's perspective FOV system across zone transitions. - -This script should exit cleanly. Before the fix, it segfaulted or -produced incorrect gridstate lengths. -""" -import mcrfpy - -# Create a small grid and a large grid -small = mcrfpy.Grid(grid_size=(10, 10)) -large = mcrfpy.Grid(grid_size=(50, 50)) - -# Create an entity on the small grid -entity = mcrfpy.Entity(grid_pos=(5, 5), grid=small) - -# Force gridstate initialization by calling update_visibility -small.perspective = entity -small.fov_radius = 4 -entity.update_visibility() # gridstate sized to 10*10 = 100 - -# Verify gridstate matches small grid -gs = entity.gridstate -assert len(gs) == 100, f"Expected gridstate size 100 for 10x10 grid, got {len(gs)}" - -# Move entity to the larger grid -entity.grid = large - -# Gridstate must now match the large grid's dimensions -gs = entity.gridstate -assert len(gs) == 2500, f"Expected gridstate size 2500 for 50x50 grid, got {len(gs)}" - -# Set up perspective on the large grid -large.perspective = entity -large.fov_radius = 8 - -# This triggers updateVisibility() which iterates 50*50 = 2500 cells. -# Before the fix, gridstate was only 100 entries — heap buffer overflow. -entity.update_visibility() - -# Stress test: repeatedly move between grids of different sizes to -# exercise the resize path and pressure the heap allocator. -grids = [mcrfpy.Grid(grid_size=(s, s)) for s in (5, 80, 3, 60, 10, 100)] -for g in grids: - entity.grid = g - g.perspective = entity - g.fov_radius = 4 - entity.update_visibility() - gs = entity.gridstate - expected = g.grid_w * g.grid_h - assert len(gs) == expected, f"Expected {expected}, got {len(gs)} for {g.grid_w}x{g.grid_h}" - -# Also allocate other objects between transitions to fill freed heap -# regions — makes corruption more likely to manifest as a crash. -for i in range(20): - small_g = mcrfpy.Grid(grid_size=(5, 5)) - entity.grid = small_g - small_g.perspective = entity - entity.update_visibility() - - big_g = mcrfpy.Grid(grid_size=(80, 80)) - entity.grid = big_g - big_g.perspective = entity - entity.update_visibility() - - # Create and destroy interim objects to churn the heap - frames = [mcrfpy.Frame() for _ in range(10)] - del frames - -print("PASS: gridstate resized correctly across all transitions")