Repository navigation
BUG: Compute Triangle Face Ids accepts Face Labels with the wrong tuple count #1860
Description
Problem
Compute Triangle Face Ids does not check that the Face Labels array has one tuple per triangle of the selected Triangle Geometry. Preflight accepts a Face Labels array with fewer tuples than the geometry has faces. Execute then loops over every face of the geometry, so it reads past the end of the Face Labels array and writes past the end of the created Feature Face Ids array, which the filter sizes from the Face Labels array.
- Expected: preflight fails with an error that names both counts, as Extract Triple Lines and Write STL Files from Triangle Geometry do for the same kind of mismatch.
- Actual: preflight passes. Execute reads and writes out of bounds (undefined behavior: wrong output, memory corruption, or a crash).
Occurs at commit 419f9db (develop).
Steps to Reproduce
The verification test below builds this input in memory:
- Create a Triangle Geometry with 3 faces and a face Attribute Matrix that holds a valid
int32Face Labels array with 3 tuples and 2 components. - Create a top-level Data Group
OtherGroupthat holds anint32arrayFaceLabelswith 2 tuples and 2 components. - Run Compute Triangle Face Ids with Triangle Geometry = the geometry from step 1 and Face Labels =
OtherGroup/FaceLabels. - Preflight passes. (Execute would read Face Labels tuple 2 and write Feature Face Ids tuple 2, which do not exist.)
The same input in the GUI (not run; the unit test is the verified reproduction): Read STL File with Generate Triangle Face Labels on, using any STL file with 3 or more triangles; Create Data Group for OtherGroup; Create Data Array with Output Numeric Type = int32, Total Number of Components = 2, Set Tuple Dimensions [not required if creating inside an Attribute Matrix] on and Data Array Dimensions (Slowest to Fastest Dimensions) = 2, Created Array = OtherGroup/FaceLabels; then Compute Triangle Face Ids with that array as Face Labels.
Cause
The Face Labels parameter restricts only the type and the component shape:
Preflight checks only that the array exists, and then creates the Feature Face Ids array with the tuple shape of the Face Labels array. It never compares that tuple count with the number of faces:
const auto* faceLabelDataArray = dataStructure.getDataAs<Int32Array>(pFaceLabelsArrayPathValue);
if(nullptr == faceLabelDataArray)
{
return IFilter::MakePreflightErrorResult(-12901, ...);
}
...
auto createArrayAction = std::make_unique<CreateArrayAction>(nx::core::DataType::int32, faceLabelDataArray->getTupleShape(), std::vector<usize>{1}, createdArrayPath);The algorithm takes the loop count from the geometry, not from the Face Labels array:
usize totalPoints = triangleGeom.getNumberOfFaces();
...
for(usize t = 0; t < totalPoints; ++t)
{
...
int32 fl0 = surfaceMeshFaceLabels[t * 2];
int32 fl1 = surfaceMeshFaceLabels[t * 2 + 1];and writes surfaceMeshFeatureFaceIds[t] for each face (L177, L184). When the Face Labels array has fewer tuples than the geometry has faces, both accesses go past the end of their arrays. The in-memory DataStore does not check the index, so these are unchecked heap reads and writes. The out-of-bounds access is inferred from the code; the test stops at preflight and does not execute it.
Sibling filters reject this mismatch in preflight, for example Extract Triple Lines:
and Write STL Files from Triangle Geometry:
Verification Unit Test
Add to src/Plugins/SimplnxCore/test/SharedFeatureFaceTest.cpp (after the existing test case; it uses the constants in the file's SharedFeatureFaceFilterTest namespace):
TEST_CASE("SimplnxCore::SharedFeatureFaceFilter: Face Labels tuple count must match the triangle count", "[SimplnxCore][SharedFeatureFaceFilter]")
{
UnitTest::LoadPlugins();
// A valid Triangle Geometry with 5 vertices and 3 faces, and a matching 3-tuple Face Labels array
DataStructure dataStructure;
auto* triangleGeomPtr = TriangleGeom::Create(dataStructure, k_TriangleGeometryName);
REQUIRE(triangleGeomPtr != nullptr);
const std::vector<float32> vertices = {0.0F, 0.0F, 0.0F, 1.0F, 0.0F, 0.0F, 0.0F, 1.0F, 0.0F, 1.0F, 1.0F, 0.0F, 2.0F, 0.0F, 0.0F};
const std::vector<IGeometry::MeshIndexType> faces = {0, 1, 2, 1, 3, 2, 1, 4, 3};
const usize numVertices = vertices.size() / 3;
const usize numFaces = faces.size() / 3;
auto* vertexAttrMatPtr = AttributeMatrix::Create(dataStructure, INodeGeometry0D::k_VertexAttributeMatrixName, {numVertices}, triangleGeomPtr->getId());
REQUIRE(vertexAttrMatPtr != nullptr);
triangleGeomPtr->setVertexAttributeMatrix(*vertexAttrMatPtr);
auto* faceAttrMatPtr = AttributeMatrix::Create(dataStructure, k_FaceAttributeMatrixName, {numFaces}, triangleGeomPtr->getId());
REQUIRE(faceAttrMatPtr != nullptr);
triangleGeomPtr->setFaceAttributeMatrix(*faceAttrMatPtr);
auto* vertexListPtr = IGeometry::SharedVertexList::Create(dataStructure, "SharedVertexList", std::make_unique<DataStore<float32>>(std::vector<usize>{numVertices}, std::vector<usize>{3}, 0.0F),
triangleGeomPtr->getId());
REQUIRE(vertexListPtr != nullptr);
auto* faceListPtr = IGeometry::SharedFaceList::Create(dataStructure, "SharedTriList", std::make_unique<DataStore<IGeometry::MeshIndexType>>(std::vector<usize>{numFaces}, std::vector<usize>{3}, 0),
triangleGeomPtr->getId());
REQUIRE(faceListPtr != nullptr);
auto& vertexListRef = vertexListPtr->getDataStoreRef();
for(usize index = 0; index < vertices.size(); index++)
{
vertexListRef[index] = vertices[index];
}
auto& faceListRef = faceListPtr->getDataStoreRef();
for(usize index = 0; index < faces.size(); index++)
{
faceListRef[index] = faces[index];
}
triangleGeomPtr->setVertices(*vertexListPtr);
triangleGeomPtr->setFaceList(*faceListPtr);
auto* faceLabelsPtr = Int32Array::Create(dataStructure, k_FaceLabelsName, std::make_unique<DataStore<int32>>(std::vector<usize>{numFaces}, std::vector<usize>{2}, 0), faceAttrMatPtr->getId());
REQUIRE(faceLabelsPtr != nullptr);
const std::vector<int32> faceLabels = {1, 2, 1, 2, 2, 3};
auto& faceLabelsRef = faceLabelsPtr->getDataStoreRef();
for(usize index = 0; index < faceLabels.size(); index++)
{
faceLabelsRef[index] = faceLabels[index];
}
// A valid int32 array with 2 components but only 2 tuples, in a group outside the geometry
const DataPath otherGroupPath({"OtherGroup"});
const DataPath shortFaceLabelsPath = otherGroupPath.createChildPath(k_FaceLabelsName);
auto* otherGroupPtr = DataGroup::Create(dataStructure, otherGroupPath.getTargetName());
REQUIRE(otherGroupPtr != nullptr);
auto* shortFaceLabelsPtr =
Int32Array::Create(dataStructure, k_FaceLabelsName, std::make_unique<DataStore<int32>>(std::vector<usize>{numFaces - 1}, std::vector<usize>{2}, 1), otherGroupPtr->getId());
REQUIRE(shortFaceLabelsPtr != nullptr);
SharedFeatureFaceFilter filter;
Arguments args;
args.insertOrAssign(SharedFeatureFaceFilter::k_RandomizeFeatures_Key, std::make_any<bool>(false));
args.insertOrAssign(SharedFeatureFaceFilter::k_TriGeometryDataPath_Key, std::make_any<GeometrySelectionParameter::ValueType>(k_GeometryPath));
args.insertOrAssign(SharedFeatureFaceFilter::k_FeatureFaceIdsArrayName_Key, std::make_any<DataObjectNameParameter::ValueType>(k_FeatureFaceIdsArrayName));
args.insertOrAssign(SharedFeatureFaceFilter::k_GrainBoundaryAttributeMatrixName_Key, std::make_any<DataObjectNameParameter::ValueType>(k_GrainBoundaryAttributeMatrixName));
args.insertOrAssign(SharedFeatureFaceFilter::k_FeatureNumTrianglesArrayName_Key, std::make_any<DataObjectNameParameter::ValueType>(k_FeatureNumTrianglesArrayName));
args.insertOrAssign(SharedFeatureFaceFilter::k_FeatureFaceLabelsArrayName_Key, std::make_any<DataObjectNameParameter::ValueType>(k_FeatureFaceLabelsArrayName));
// Control: the matching Face Labels array passes preflight
args.insertOrAssign(SharedFeatureFaceFilter::k_FaceLabelsArrayPath_Key, std::make_any<DataPath>(k_FaceLabelsPath));
auto validPreflightResult = filter.preflight(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_VALID(validPreflightResult.outputActions);
// 2 Face Labels tuples for 3 triangles must be rejected before execute reads past the array
args.insertOrAssign(SharedFeatureFaceFilter::k_FaceLabelsArrayPath_Key, std::make_any<DataPath>(shortFaceLabelsPath));
auto preflightResult = filter.preflight(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_INVALID(preflightResult.outputActions);
}Run only this test from the build directory:
ctest -R "SharedFeatureFaceFilter: Face Labels tuple count" --output-on-failureFailure at commit 419f9db (the control preflight passes, the mismatched preflight is accepted):
src/Plugins/SimplnxCore/test/SharedFeatureFaceTest.cpp:182: FAILED:
REQUIRE( simplnxCheckedResult.invalid() )
with expansion:
false
test cases: 1 | 0 passed | 1 failed
assertions: 11 | 10 passed | 1 failed
Proposed Fix
In SharedFeatureFaceFilter::preflightImpl() (src/Plugins/SimplnxCore/src/SimplnxCore/Filters/SharedFeatureFaceFilter.cpp), get the Triangle Geometry and return an error when the Face Labels tuple count is not the face count, before the output actions are created:
+ const auto* triangleGeom = dataStructure.getDataAs<TriangleGeom>(triangleGeometryPath);
+ if(nullptr == triangleGeom)
+ {
+ return IFilter::MakePreflightErrorResult(-12902, fmt::format("Triangle Geometry does not exist at path '{}'", triangleGeometryPath.toString()));
+ }
+
// Create the Face Data/FeatureIds Array
{
const auto* faceLabelDataArray = dataStructure.getDataAs<Int32Array>(pFaceLabelsArrayPathValue);
if(nullptr == faceLabelDataArray)
{
return IFilter::MakePreflightErrorResult(-12901, ...);
}
+ if(faceLabelDataArray->getNumberOfTuples() != triangleGeom->getNumberOfFaces())
+ {
+ return IFilter::MakePreflightErrorResult(-12903, fmt::format("Face Labels array '{}' has {} tuples but Triangle Geometry '{}' has {} faces. They must match.", pFaceLabelsArrayPathValue.toString(),
+ faceLabelDataArray->getNumberOfTuples(), triangleGeometryPath.toString(), triangleGeom->getNumberOfFaces()));
+ }The filter needs #include "simplnx/DataStructure/Geometry/TriangleGeom.hpp". The error codes are suggestions; -12901 is the only code the filter uses now. After the fix, the test can also check the new error code. The algorithm can then rely on the two counts being equal.
Notes
- Found while reviewing BUG: Read the shared face-count map without inserting in Compute Triangle Face Ids #1858 (the fix for BUG: Avoid concurrent map insertion in Compute Triangle Face Ids #1772).
- Closest existing issue checked: BUG: Enforce array-kind contracts in ArraySelectionParameter #1833.
- The verification test was built and run on macOS (Apple clang, Release) at commit 419f9db.
Activity
imikejackson commentedon Oct 21, 2026
@joeykleingers This is either being fixed in a current PR or has aready been fixed in a merge. I distinctly remember fixing this somewhere.