fix(places): use-after-free and NaN centroid in RegionGrowingTraversabilityClustering#152
Merged
nathanhhughes merged 2 commits intoJul 6, 2026
Conversation
harelb
added a commit
to harelb/Hydra
that referenced
this pull request
Jul 4, 2026
mergeRegions held a reference (neighbor_id) into region.neighbors, but region.merge() erases it, leaving a dangling reference read by the subsequent graph.removeNode() call (heap-use-after-free). Copy it by value first. Also guard computeBoundary against an empty exterior boundary, which produced a NaN centroid/place position. Upstream: Hydra PR MIT-SPARK#152.
…bilityClustering Two heap/correctness bugs in the region-growing traversability clustering, found with AddressSanitizer. 1. Use-after-free / iterator invalidation in mergeRegions(). The loop iterates `region.neighbors` while `region.merge()` calls `neighbors.erase(other.id)` on that same map, invalidating the loop iterator and freeing the node the loop's `neighbor_id` refers to (read immediately after by graph.removeNode). Fix the root cause rather than the symptom: Region::merge no longer erases from `neighbors` (it only inserts, which never invalidates iterators), and mergeRegions() now iterates with an explicit iterator and erases the stale and merged-away neighbors in place. 2. NaN centroid in Region::computeBoundary(). `centroid /= exterior_boundary.size()` divides a zero vector by 0 when a region has no exterior boundary, producing a NaN place position. Guard the division.
harelb
force-pushed
the
fix/rg-traversability-heap-and-nan
branch
from
July 6, 2026 16:51
e4933d7 to
c6563aa
Compare
nathanhhughes
approved these changes
Jul 6, 2026
nathanhhughes
left a comment
nathanhhughes
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, thanks for catching this! Some style things that would be helpful
Comment on lines
+233
to
+236
| // Use an explicit iterator so we can erase entries in place. Region::merge | ||
| // only inserts into region.neighbors (std::map insertion never invalidates | ||
| // iterators), so n_it stays valid across the merge; we remove the stale and | ||
| // merged-away neighbors here rather than inside merge(). |
Collaborator
There was a problem hiding this comment.
drop comment, this isn't useful where it is
| // iterators), so n_it stays valid across the merge; we remove the stale and | ||
| // merged-away neighbors here rather than inside merge(). | ||
| for (auto n_it = region.neighbors.begin(); n_it != region.neighbors.end();) { | ||
| const NodeId neighbor_id = n_it->first; |
Collaborator
There was a problem hiding this comment.
minor preference for structured binding like:
const auto [neighbor_id, num_connecting_voxels] = *n_it;
Comment on lines
+257
to
+259
| // Merge neighbor into this region, then remove it from regions_, the | ||
| // graph, and this region's neighbor map (in place, so no iterator is | ||
| // left dangling). |
Collaborator
There was a problem hiding this comment.
Something shorter like "merge neighbor into this region and then cleanup" is probably better
| } | ||
|
|
||
| neighbors.erase(other.id); | ||
| // NOTE: other.id is intentionally NOT erased from `neighbors` here. Erasing it |
Collaborator
There was a problem hiding this comment.
Simpler comment would be better
Comment on lines
+513
to
+517
| // Guard against an empty exterior boundary (e.g. a region with no boundary | ||
| // voxels): dividing a zero vector by 0 yields a NaN centroid/place position. | ||
| if (!exterior_boundary.empty()) { | ||
| centroid /= exterior_boundary.size(); | ||
| } |
Collaborator
There was a problem hiding this comment.
I think I would prefer this check to come earlier, i.e., after exterior_boundary = growRegion(boundary, farthest) I would put something like if (exterior_boundary.empty()) { LOG(WARNING) << "Empty region ecountered!"; } and have the region constructor default the position to something sane (like NaNs tbh). My understanding is that we shouldn't ever need to compute the centroid of a place that doesn't have exterior boundary voxels (and picking zero is a bad choice because you can't distinguish between a valid place at the origin and invalid places)
- drop/shorten comments per review; structured binding for neighbor iteration - warn on empty region right after growRegion, and default Region::centroid to NaN so a boundary-less place stays distinguishable from a valid origin place (rather than guarding the division to leave it at zero)
harelb
marked this pull request as ready for review
July 6, 2026 20:05
harelb
added a commit
to harelb/Hydra
that referenced
this pull request
Jul 7, 2026
Mirrors the upstream Hydra PR fixes onto the build branch: - deformation_interpolator: stamp cache entries with the computed timestamp_ns (not last_update_time_ns, which is 0 for objects) and reset to init_pos before transform() so repeated deformation of archived nodes stays idempotent (upstream PR MIT-SPARK#153). - region_growing_traversability_clustering: erase merged/stale neighbors in place with the loop iterator instead of erasing inside Region::merge, removing the dangling-reference hazard (upstream PR MIT-SPARK#152).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These were both discovered while I was working on some of the image keyframing code which does a lot more work in the heap.
Summary:
Two heap/correctness bugs in the region-growing traversability clustering, found with AddressSanitizer.
1. Use-after-free / iterator invalidation in
mergeRegionsmergeRegionsiteratesregion.neighborswhileregion.merge()callsneighbors.erase(other.id)on that same map. That erase invalidates the loop iterator and frees the node the loop’sneighbor_idrefers to, whichgraph.removeNode(neighbor_id)then reads. (ASan: heap-use-after-free via_Rb_treeerase; report available on request.)Fix (root cause, not the symptom):
Region::mergeno longer erases fromneighbors— it only inserts, andstd::mapinsertion never invalidates existing iterators.mergeRegionsnow walksregion.neighborswith an explicit iterator and erases the stale and merged-away neighbors in place (n_it = neighbors.erase(n_it)).2. NaN centroid in
Region::computeBoundarycentroid /= exterior_boundary.size()divides a zero vector by 0 when a region has no exterior boundary, producing a NaN place position. Fix: guard the division (leave the centroid at zero when the boundary is empty).Both fixes verified on infinite corridor bags — the map (agents/places/objects) which were previously crashing.