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).
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).