Skip to content

Commit 552249c

Browse files
Harden UnionFind bindings and mutex-watershed neighbor bounds
B2: validate node ids at the UnionFind binding boundary. UnionFind::find/ merge/merge_to index their parent vectors without a bounds check (intentional for the hot path), but the scalar and array bindings passed user-supplied ids straight through, so an out-of-range id was undefined behavior / a segfault from Python. Add check_node() and route the scalar find/merge/merge_to binds and the merge_edges/find_nodes helpers through it, throwing std::invalid_argument before releasing the GIL. B1: route the mutex-watershed and semantic-mutex-watershed neighbor computation through detail::valid_offset_target instead of an unchecked flat-index shift. This bounds-checks the neighbor per axis, so the kernel is memory-safe and rejects row/plane wrap-around independently of the caller's valid_edges mask. Drops the now-unused offset_strides precompute. Add regression tests for the new UnionFind validation and for the existing binding-layer point-coordinate validation in non_maximum_distance_suppression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7b67925 commit 552249c

6 files changed

Lines changed: 102 additions & 24 deletions

File tree

include/bioimage_cpp/non_maximum_distance_suppression.hxx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,9 @@ inline void non_maximum_distance_suppression(
8181
const auto strides = bioimage_cpp::detail::c_order_strides(distance_map.shape);
8282
const auto n = static_cast<std::size_t>(n_points);
8383

84-
// Precompute flat index and distance value at each point.
84+
// Precompute flat index and distance value at each point. Point coordinates
85+
// are validated against distance_map bounds in the binding layer before this
86+
// is called (see bindings/distance.cxx).
8587
std::vector<float> point_dist(n);
8688
for (std::size_t i = 0; i < n; ++i) {
8789
const auto *row =

include/bioimage_cpp/segmentation/mutex_watershed.hxx

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -78,13 +78,6 @@ void mutex_watershed_grid(
7878
));
7979
const auto spatial_strides = detail::c_order_strides(spatial_shape);
8080

81-
std::vector<std::ptrdiff_t> offset_strides(number_of_channels, 0);
82-
for (std::size_t channel = 0; channel < number_of_channels; ++channel) {
83-
for (std::size_t axis = 0; axis < spatial_ndim; ++axis) {
84-
offset_strides[channel] += offsets[channel][axis] * spatial_strides[axis];
85-
}
86-
}
87-
8881
const auto number_of_edges = number_of_nodes * number_of_channels;
8982
std::vector<WeightedGridEdge<T>> edge_order;
9083
edge_order.reserve(static_cast<std::size_t>(number_of_edges));
@@ -108,8 +101,13 @@ void mutex_watershed_grid(
108101
const auto channel = static_cast<std::size_t>(edge_id / number_of_nodes);
109102
const auto u = edge_id % number_of_nodes;
110103

111-
const auto v_signed = static_cast<std::int64_t>(u) + static_cast<std::int64_t>(offset_strides[channel]);
112-
const auto v = static_cast<std::uint64_t>(v_signed);
104+
// Bounds-check the neighbor per axis rather than relying solely on the
105+
// valid_edges mask: this keeps the kernel memory-safe and rejects edges
106+
// that would wrap across a row/plane boundary.
107+
std::uint64_t v = 0;
108+
if (!detail::valid_offset_target(u, offsets[channel], spatial_shape, spatial_strides, v)) {
109+
continue;
110+
}
113111
std::uint64_t root_u = sets.find(u);
114112
std::uint64_t root_v = sets.find(v);
115113
if (root_u == root_v) {

include/bioimage_cpp/segmentation/semantic_mutex_watershed.hxx

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,6 @@ void semantic_mutex_watershed_grid(
105105
));
106106
const auto spatial_strides = detail::c_order_strides(spatial_shape);
107107

108-
std::vector<std::ptrdiff_t> offset_strides(number_of_offsets, 0);
109-
for (std::size_t channel = 0; channel < number_of_offsets; ++channel) {
110-
for (std::size_t axis = 0; axis < spatial_ndim; ++axis) {
111-
offset_strides[channel] += offsets[channel][axis] * spatial_strides[axis];
112-
}
113-
}
114-
115108
struct WeightedGridEdge {
116109
T weight;
117110
std::uint64_t id;
@@ -155,9 +148,12 @@ void semantic_mutex_watershed_grid(
155148
continue;
156149
}
157150

158-
const auto v_signed = static_cast<std::int64_t>(u) +
159-
static_cast<std::int64_t>(offset_strides[channel]);
160-
const auto v = static_cast<std::uint64_t>(v_signed);
151+
// Bounds-check the neighbor per axis (see mutex_watershed_grid): keeps
152+
// the kernel memory-safe and rejects edges that wrap across a boundary.
153+
std::uint64_t v = 0;
154+
if (!detail::valid_offset_target(u, offsets[channel], spatial_shape, spatial_strides, v)) {
155+
continue;
156+
}
161157
const auto root_u = sets.find(u);
162158
const auto root_v = sets.find(v);
163159
if (root_u == root_v) {

src/bindings/util.cxx

Lines changed: 42 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,18 @@ using EdgeArray = nb::ndarray<nb::numpy, const std::uint64_t, nb::c_contig>;
1818
using NodeArray = nb::ndarray<nb::numpy, const std::uint64_t, nb::c_contig, nb::ndim<1>>;
1919
using OutputArray = nb::ndarray<nb::numpy, std::uint64_t, nb::c_contig>;
2020

21+
// UnionFind::find/merge/merge_to index their parent/rank vectors without a
22+
// bounds check (intentionally, for the hot path). Validate node ids at the
23+
// binding boundary so out-of-range ids raise a clear error instead of UB.
24+
void check_node(const util::UnionFind &uf, const std::uint64_t node, const char *name) {
25+
if (node >= uf.size()) {
26+
throw std::invalid_argument(
27+
std::string(name) + " out of range: got " + name + "="
28+
+ std::to_string(node) + ", size=" + std::to_string(uf.size())
29+
);
30+
}
31+
}
32+
2133
void merge_edges(
2234
util::UnionFind &uf,
2335
EdgeArray edges
@@ -39,6 +51,11 @@ void merge_edges(
3951
const auto n_edges = edges.shape(0);
4052
const auto *data = edges.data();
4153

54+
for (std::size_t i = 0; i < n_edges; ++i) {
55+
check_node(uf, data[2 * i], "edge endpoint");
56+
check_node(uf, data[2 * i + 1], "edge endpoint");
57+
}
58+
4259
{
4360
nb::gil_scoped_release release;
4461
for (std::size_t i = 0; i < n_edges; ++i) {
@@ -49,10 +66,14 @@ void merge_edges(
4966

5067
OutputArray find_nodes(util::UnionFind &uf, NodeArray nodes) {
5168
const auto n = nodes.shape(0);
69+
const auto *input = nodes.data();
70+
for (std::size_t i = 0; i < n; ++i) {
71+
check_node(uf, input[i], "node");
72+
}
73+
5274
auto *out = new std::uint64_t[n]();
5375
nb::capsule owner(out, [](void *p) noexcept { delete[] static_cast<std::uint64_t *>(p); });
5476

55-
const auto *input = nodes.data();
5677
{
5778
nb::gil_scoped_release release;
5879
for (std::size_t i = 0; i < n; ++i) {
@@ -64,6 +85,23 @@ OutputArray find_nodes(util::UnionFind &uf, NodeArray nodes) {
6485
return OutputArray(out, 1, shape, owner);
6586
}
6687

88+
std::uint64_t find_node(util::UnionFind &uf, const std::uint64_t node) {
89+
check_node(uf, node, "node");
90+
return uf.find(node);
91+
}
92+
93+
std::uint64_t merge_pair(util::UnionFind &uf, const std::uint64_t first, const std::uint64_t second) {
94+
check_node(uf, first, "first");
95+
check_node(uf, second, "second");
96+
return uf.merge(first, second);
97+
}
98+
99+
std::uint64_t merge_to_node(util::UnionFind &uf, const std::uint64_t stable, const std::uint64_t removed) {
100+
check_node(uf, stable, "stable");
101+
check_node(uf, removed, "removed");
102+
return uf.merge_to(stable, removed);
103+
}
104+
67105
OutputArray element_labeling(util::UnionFind &uf) {
68106
const auto n = uf.size();
69107
auto *out = new std::uint64_t[n]();
@@ -98,7 +136,7 @@ void bind_util(nb::module_ &m) {
98136
)
99137
.def(
100138
"find",
101-
&util::UnionFind::find,
139+
&find_node,
102140
nb::arg("node"),
103141
"Return the (path-compressed) root of `node`."
104142
)
@@ -110,7 +148,7 @@ void bind_util(nb::module_ &m) {
110148
)
111149
.def(
112150
"merge",
113-
&util::UnionFind::merge,
151+
&merge_pair,
114152
nb::arg("first"),
115153
nb::arg("second"),
116154
"Union the sets containing `first` and `second`. Returns the new root."
@@ -123,7 +161,7 @@ void bind_util(nb::module_ &m) {
123161
)
124162
.def(
125163
"merge_to",
126-
&util::UnionFind::merge_to,
164+
&merge_to_node,
127165
nb::arg("stable"),
128166
nb::arg("removed"),
129167
"Union the sets containing `stable` and `removed`, forcing "

tests/distance/test_non_maximum_distance_suppression.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,20 @@ def test_two_close_points_keeps_higher_value():
3333
assert out.tolist() == [[5, 5]]
3434

3535

36+
def test_out_of_range_point_coordinate_raises():
37+
dm = np.zeros((10, 10), dtype=np.float32)
38+
pts = np.array([[5, 5], [5, 12]], dtype=np.int64)
39+
with pytest.raises(ValueError, match="out of bounds"):
40+
nms(dm, pts)
41+
42+
43+
def test_negative_point_coordinate_raises():
44+
dm = np.zeros((10, 10), dtype=np.float32)
45+
pts = np.array([[-1, 5]], dtype=np.int64)
46+
with pytest.raises(ValueError, match="out of bounds"):
47+
nms(dm, pts)
48+
49+
3650
def test_two_far_points_both_survive():
3751
dm = np.zeros((20, 20), dtype=np.float32)
3852
dm[2, 2] = 1.0

tests/test_util_union_find.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,3 +141,33 @@ def test_bulk_merge_rejects_three_columns():
141141
uf = bic.utils.UnionFind(3)
142142
with pytest.raises(Exception, match=r"\(N, 2\)"):
143143
uf.merge(np.zeros((2, 3), dtype=np.uint64))
144+
145+
146+
def test_scalar_find_rejects_out_of_range_node():
147+
uf = bic.utils.UnionFind(3)
148+
with pytest.raises(ValueError, match="out of range"):
149+
uf.find(3)
150+
151+
152+
def test_scalar_merge_rejects_out_of_range_node():
153+
uf = bic.utils.UnionFind(3)
154+
with pytest.raises(ValueError, match="out of range"):
155+
uf.merge(0, 5)
156+
157+
158+
def test_merge_to_rejects_out_of_range_node():
159+
uf = bic.utils.UnionFind(3)
160+
with pytest.raises(ValueError, match="out of range"):
161+
uf.merge_to(7, 0)
162+
163+
164+
def test_bulk_find_rejects_out_of_range_node():
165+
uf = bic.utils.UnionFind(3)
166+
with pytest.raises(ValueError, match="out of range"):
167+
uf.find(np.array([0, 1, 9], dtype=np.uint64))
168+
169+
170+
def test_bulk_merge_rejects_out_of_range_node():
171+
uf = bic.utils.UnionFind(3)
172+
with pytest.raises(ValueError, match="out of range"):
173+
uf.merge(np.array([[0, 1], [2, 8]], dtype=np.uint64))

0 commit comments

Comments
 (0)