BlueQuartzSoftware / BlueQuartzSoftware/simplnx
Surface meshers: legitimate Feature Ids can alias exterior/ghost sentinels, silently corrupting output
- Dominant language
- C++
- Stars
- 17
- Forks
- 13
- Avg merge
- 3d 4h
- Merged PRs (30d)
- 10
Description
## Summary
All three surface meshers encode "not a real feature" using values drawn from the same `int32` space as legitimate Feature Ids. A Feature Ids array containing one of those values is silently misinterpreted — in some cases producing a corrupt mesh with no error, and in one case invoking signed-integer overflow (undefined behaviour).
Four distinct collisions, all pre-existing. Found during an adversarial review of surface-meshing work; a targeted mitigation ships alongside that work (see below), but the underlying design is unchanged.
## The collisions
**1. `MMSurfaceNet::Padding == std::numeric_limits::max()` — SurfaceNets**
`MMSurfaceNet.h` uses `INT32_MAX` as the label for the synthetic ghost shell around the volume. `MMCellMap::label()` returns the raw Feature Id with no range check, so a legitimate Feature Id of `2147483647` is indistinguishable from "outside the volume". That voxel's cell flags, vertex placement and Node Types are all computed as if it were exterior. No warning.
**2. `maxGrainId = maxGrainId + 1` overflows — M3CSurfaceMeshing**
In `initialize_micro`, `maxGrainId` is derived from the data and then incremented, to serve as the internal renumbering of Feature Id 0. With a Feature Id of `INT32_MAX` present this is signed overflow — undefined behaviour, in practice wrapping negative. The wrapped value is then written into every Feature-0 voxel as "the renumbered background", where it satisfies `nSpin < 0`, which is M3C's test for "ghost/exterior". Feature 0 then folds into Face Label `-1` instead of `0`.
**3. Negative Feature Ids collide with M3C's ghost sign convention**
M3C marks ghost cells by the *sign* of `nSpin`. A legitimate Feature Id of, say, `-5` is therefore treated as exterior wherever that convention is consulted. `M3CSurfaceMeshingFilter` places no range restriction on the Feature Ids parameter.
**4. Feature Id `-1` collides with QuickSurfaceMesh's own exterior marker**
`QuickSurfaceMesh` hard-codes `-1` as the exterior Face Label and inserts `-1` into its per-node owner sets. A legitimate Feature Id of `-1` is deduplicated against the exterior marker inside those `std::set`s, undercounting distinct owners and so producing a wrong Node Type; and a wall face backed by that feature gets Face Labels `{-1, -1}`.
That last symptom is worth noting: `{-1, -1}` is exactly the "double exterior" corruption that was just fixed in SurfaceNets (where padding was mapped to `0` and then every `0` rewritten to `-1`). The same shape is reachable in QuickSurfaceMesh by a different route.
## Reachability
Feature Ids are conventionally non-negative and small, so none of this arises from ordinary segmentation output. But the filters accept an arbitrary `Int32Array` selected by the user, which can be imported or computed, so these are reachable through the normal parameter surface rather than being purely theoretical. Nothing in preflight rejects them.
## What ships as mitigation (not a fix)
The surface-meshing branch adds a validation pass in each mesher's `executeImpl` that rejects colliding values with a clear error naming the offending value:
- QuickSurfaceMesh: reject any Feature Id `< 0`
- SurfaceNets: reject `< 0` or `== INT32_MAX`
- M3CSurfaceMeshing: reject `< 0` or `== INT32_MAX`
That converts four silent-corruption paths into explicit errors. It is deliberately in `executeImpl` rather than preflight: a 512³ scan is ~134M reads, and preflight runs on every parameter change in the GUI.
## The actual fix
Stop letting a representable Feature Id alias a sentinel:
- **SurfaceNets**: widen the internal label type, or carry "is padding" as a separate flag rather than a reserved value.
- **M3C**: widen `maxGrainId` to `int64` (or saturate), and track "ghost" with a dedicated flag instead of the sign of `nSpin`.
- **QuickSurfaceMesh**: use a distinct exterior marker that cannot be a Feature Id, or track exterior membership separately from the owner set.
`MMSurfaceNet.*` and `MMCellMap.*` are third-party-derived (Sarah Frisken, Brigham and Women's Hospital), so changes there need care. Any of these will change output for the pathological inputs only, but the M3C and QuickSurfaceMesh changes touch core label handling and should be validated against the stored exemplars.
Contributor guide
Research direction
Start by reading the three meshers' executeImpl paths, then inspect MMSurfaceNet.* and MMCellMap.* to understand the existing sentinel handling. Reproduce the listed collisions with pathological Feature Ids and compare behavior against the stored exemplars. Done means legitimate Feature Ids no longer alias exterior or ghost state, without changing ordinary-output behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-graphics
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100