[Bugfix] grid.at() returns GridPoint with dangling raw pointer into grid storage #265

New issue

Closed

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

john commented

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

Owner

Summary

UIGrid::py_at(x, y) and UIGrid::subscript() return a GridPoint Python object that holds a raw UIGridPoint* pointer directly into the grid's internal point storage. If the grid's storage is ever reallocated or the grid is destroyed while the GridPoint object is alive, the pointer dangles.

Root Cause

UIGrid.cpp:1362 (py_at):

obj->data = &(self->data->at(x, y));
obj->grid = self->data;

UIGrid.cpp:1403 (subscript):

obj->data = &(self->data->at(x, y));
obj->grid = self->data;

The PyUIGridPointObject struct stores:

UIGridPoint* data;                    // Raw pointer into grid storage
std::shared_ptr<UIGrid> grid;         // At least this is a shared_ptr

The grid member being a shared_ptr prevents the grid from being destroyed while a GridPoint exists, but doesn't protect against internal reallocation of the grid's point storage.

Reproduction

import mcrfpy

grid = mcrfpy.Grid(grid_size=(10, 10))
point = grid.at(5, 5)

# If grid's internal storage is reallocated (e.g., grid resize if implemented),
# point.data is now dangling
point.walkable = False  # Writes to potentially freed memory

Note: Currently the grid's points vector size is fixed at construction and never resized, so this is lower risk in practice. However, the grid field is a shared_ptr, so the grid won't be destroyed while GridPoint objects exist. The main risk is if grid resize is ever implemented.

Fix Options

Store coordinates instead of pointer: Change GridPoint to store (grid_shared_ptr, x, y) and recompute the pointer on each property access via grid->at(x, y)
Accept current risk: Document that grid storage is immutable after construction, making this a theoretical-only concern

Severity

Medium — currently low risk because grid size is fixed at construction, but the pattern is fragile. The shared_ptr<UIGrid> grid member already prevents the worst case (grid destruction). However, this is a hazard waiting for anyone who adds grid resize functionality.

## Summary `UIGrid::py_at(x, y)` and `UIGrid::subscript()` return a `GridPoint` Python object that holds a raw `UIGridPoint*` pointer directly into the grid's internal point storage. If the grid's storage is ever reallocated or the grid is destroyed while the `GridPoint` object is alive, the pointer dangles. ## Root Cause `UIGrid.cpp:1362` (py_at): ```cpp obj->data = &(self->data->at(x, y)); obj->grid = self->data; ``` `UIGrid.cpp:1403` (subscript): ```cpp obj->data = &(self->data->at(x, y)); obj->grid = self->data; ``` The `PyUIGridPointObject` struct stores: ```cpp UIGridPoint* data; // Raw pointer into grid storage std::shared_ptr<UIGrid> grid; // At least this is a shared_ptr ``` The `grid` member being a `shared_ptr` prevents the grid from being destroyed while a GridPoint exists, but doesn't protect against internal reallocation of the grid's point storage. ## Reproduction ```python import mcrfpy grid = mcrfpy.Grid(grid_size=(10, 10)) point = grid.at(5, 5) # If grid's internal storage is reallocated (e.g., grid resize if implemented), # point.data is now dangling point.walkable = False # Writes to potentially freed memory ``` **Note**: Currently the grid's `points` vector size is fixed at construction and never resized, so this is lower risk in practice. However, the `grid` field is a `shared_ptr`, so the grid won't be destroyed while GridPoint objects exist. The main risk is if grid resize is ever implemented. ## Fix Options 1. **Store coordinates instead of pointer**: Change `GridPoint` to store `(grid_shared_ptr, x, y)` and recompute the pointer on each property access via `grid->at(x, y)` 2. **Accept current risk**: Document that grid storage is immutable after construction, making this a theoretical-only concern ## Severity **Medium** — currently low risk because grid size is fixed at construction, but the pattern is fragile. The `shared_ptr<UIGrid> grid` member already prevents the worst case (grid destruction). However, this is a hazard waiting for anyone who adds grid resize functionality.