BlueQuartzSoftware / BlueQuartzSoftware/simplnx

Surface meshers: legitimate Feature Ids can alias exterior/ghost sentinels, silently corrupting output

Open
#1,705 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.