Skip to content

Grid::removeLocation invalidates its iterator after erase, causing undefined behavior #31

Description

@dmccoystephenson

A pre-existing defect was surfaced while adding direct test coverage for `Grid::removeLocation` under #28.

`src/grid.cpp`:
```cpp
void Grid::removeLocation(Location& location) {
for (auto i = locations.begin(); i != locations.end(); i++) {
if (i->getId() == location.getId()) {
locations.erase(i);
}
}
}
```

`std::vector::erase(i)` invalidates `i` (and every iterator from that point onward). The loop's own `i++` is then applied to an already-invalidated iterator, which is undefined behavior.

Confirmed empirically: a direct test constructing a small grid, adding a location, and calling `removeLocation` on it reliably aborts — under a plain build with `terminate called after throwing an instance of 'std::logic_error' — basic_string::_M_construct null not valid`, and under `-fsanitize=address` with a `std::length_error` from `basic_string::_M_create` — both symptoms of reading through the invalidated iterator.

Proposed fix: capture the return value of `erase`, e.g.

```cpp
void Grid::removeLocation(Location& location) {
for (auto i = locations.begin(); i != locations.end();) {
if (i->getId() == location.getId()) {
i = locations.erase(i);
} else {
++i;
}
}
}
```

A regression test was intentionally not added in the same PR that surfaces this — a test-coverage cycle should characterize existing behavior, not fix production code discovered incidentally, per this repo's dev-loop policy. This issue tracks the fix and its own regression test as a follow-up.

This issue body was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions