From c6563aa108fdc64aa7a7db16a7852e0df54d491d Mon Sep 17 00:00:00 2001 From: harelb Date: Fri, 3 Jul 2026 20:15:54 -0400 Subject: [PATCH 1/2] fix(places): use-after-free and NaN centroid in RegionGrowingTraversabilityClustering 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. --- ...gion_growing_traversability_clustering.cpp | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/places/region_growing_traversability_clustering.cpp b/src/places/region_growing_traversability_clustering.cpp index 92f801c7..8f91cd3d 100644 --- a/src/places/region_growing_traversability_clustering.cpp +++ b/src/places/region_growing_traversability_clustering.cpp @@ -230,15 +230,21 @@ void RegionGrowingTraversabilityClustering::mergeRegions( if (!region.is_active) { continue; } - std::list neighbors_to_remove; - for (const auto& [neighbor_id, num_connecting_voxels] : region.neighbors) { + // 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(). + for (auto n_it = region.neighbors.begin(); n_it != region.neighbors.end();) { + const NodeId neighbor_id = n_it->first; auto neighbor_it = regions_.find(neighbor_id); if (neighbor_it == regions_.end()) { - neighbors_to_remove.push_back(neighbor_id); + // Stale neighbor: its region was already merged away. Drop it in place. + n_it = region.neighbors.erase(n_it); continue; } Region& neighbor_region = neighbor_it->second; if (!neighbor_region.is_active) { + ++n_it; continue; } @@ -248,16 +254,17 @@ void RegionGrowingTraversabilityClustering::mergeRegions( const Eigen::Vector2i combined_max = region.max_coordinates.cwiseMax(neighbor_region.max_coordinates); if ((combined_max - combined_min).maxCoeff() <= max_region_size_) { - // Merge neighbor into this region. + // 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). region.merge(neighbor_region); regions_.erase(neighbor_it); graph.removeNode(neighbor_id); + n_it = region.neighbors.erase(n_it); merged = true; break; } - } - for (const auto neighbor_id : neighbors_to_remove) { - region.neighbors.erase(neighbor_id); + ++n_it; } if (merged) { break; @@ -457,7 +464,10 @@ void RegionGrowingTraversabilityClustering::Region::merge(const Region& other) { neighbors[n_id] += n_count; } - neighbors.erase(other.id); + // NOTE: other.id is intentionally NOT erased from `neighbors` here. Erasing it + // would invalidate the caller's iterator into this map; mergeRegions() removes + // the merged-away neighbor in place instead. (The insertions above are safe: + // std::map insertion never invalidates existing iterators.) computeBoundary(); } @@ -500,7 +510,11 @@ void RegionGrowingTraversabilityClustering::Region::computeBoundary() { centroid += voxel.cast(); } - centroid /= exterior_boundary.size(); + // 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(); + } } void RegionGrowingTraversabilityClustering::Region::computeNeighbors( From 23ec9d2df7487b9ef1d17051135d2c783e8d9908 Mon Sep 17 00:00:00 2001 From: harelb Date: Mon, 6 Jul 2026 15:55:23 -0400 Subject: [PATCH 2/2] address review: comment cleanup + NaN-default empty region - 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) --- ...region_growing_traversability_clustering.h | 7 ++-- ...gion_growing_traversability_clustering.cpp | 33 ++++++++----------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/include/hydra/places/region_growing_traversability_clustering.h b/include/hydra/places/region_growing_traversability_clustering.h index f5d07a7d..bf37b441 100644 --- a/include/hydra/places/region_growing_traversability_clustering.h +++ b/include/hydra/places/region_growing_traversability_clustering.h @@ -36,6 +36,7 @@ #include +#include #include #include "hydra/places/traversability_clustering.h" @@ -79,8 +80,10 @@ class RegionGrowingTraversabilityClustering : public TraversabilityClustering { // the current layer. bool is_active = true; - // Centroid of the region. - Eigen::Vector3f centroid; + // Centroid of the region. Defaults to NaN so a region with no exterior + // boundary voxels stays distinguishable from a valid place at the origin. + Eigen::Vector3f centroid = + Eigen::Vector3f::Constant(std::numeric_limits::quiet_NaN()); // Axis-aligned bounding box of the region in voxel coordinates. Eigen::Vector2i min_coordinates; diff --git a/src/places/region_growing_traversability_clustering.cpp b/src/places/region_growing_traversability_clustering.cpp index 8f91cd3d..b2f6c7b7 100644 --- a/src/places/region_growing_traversability_clustering.cpp +++ b/src/places/region_growing_traversability_clustering.cpp @@ -230,12 +230,8 @@ void RegionGrowingTraversabilityClustering::mergeRegions( if (!region.is_active) { continue; } - // 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(). for (auto n_it = region.neighbors.begin(); n_it != region.neighbors.end();) { - const NodeId neighbor_id = n_it->first; + const auto [neighbor_id, num_connecting_voxels] = *n_it; auto neighbor_it = regions_.find(neighbor_id); if (neighbor_it == regions_.end()) { // Stale neighbor: its region was already merged away. Drop it in place. @@ -254,9 +250,7 @@ void RegionGrowingTraversabilityClustering::mergeRegions( const Eigen::Vector2i combined_max = region.max_coordinates.cwiseMax(neighbor_region.max_coordinates); if ((combined_max - combined_min).maxCoeff() <= max_region_size_) { - // 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). + // Merge neighbor into this region and then cleanup. region.merge(neighbor_region); regions_.erase(neighbor_it); graph.removeNode(neighbor_id); @@ -464,10 +458,8 @@ void RegionGrowingTraversabilityClustering::Region::merge(const Region& other) { neighbors[n_id] += n_count; } - // NOTE: other.id is intentionally NOT erased from `neighbors` here. Erasing it - // would invalidate the caller's iterator into this map; mergeRegions() removes - // the merged-away neighbor in place instead. (The insertions above are safe: - // std::map insertion never invalidates existing iterators.) + // Leave other.id in `neighbors`; mergeRegions() erases it in place so the + // caller's iterator stays valid. computeBoundary(); } @@ -496,6 +488,9 @@ void RegionGrowingTraversabilityClustering::Region::computeBoundary() { // Find exterior and interior boundary voxels. exterior_boundary = growRegion(boundary, furthest); + if (exterior_boundary.empty()) { + LOG(WARNING) << "Empty region encountered!"; + } interior_boundary.clear(); interior_boundary.reserve(boundary.size() - exterior_boundary.size()); for (const auto& voxel : boundary) { @@ -504,15 +499,13 @@ void RegionGrowingTraversabilityClustering::Region::computeBoundary() { } } - // Compute the centroid. - centroid = Eigen::Vector3f::Zero(); - for (const auto& voxel : exterior_boundary) { - centroid += voxel.cast(); - } - - // 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. + // Compute the centroid. A boundary-less region initializes to NaN default (see + // Region::centroid) if (!exterior_boundary.empty()) { + centroid = Eigen::Vector3f::Zero(); + for (const auto& voxel : exterior_boundary) { + centroid += voxel.cast(); + } centroid /= exterior_boundary.size(); } }