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 <noreply@anthropic.com>
This commit is contained in:
parent
08407e48e1
commit
348826a0f5
5 changed files with 203 additions and 130 deletions
|
|
@ -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<size_t>(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<UIEntity>& 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;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
175
tests/regression/issue_258_gridstate_resize_test.py
Normal file
175
tests/regression/issue_258_gridstate_resize_test.py
Normal file
|
|
@ -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)
|
||||
|
|
@ -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")
|
||||
Loading…
Add table
Add a link
Reference in a new issue