fix(topology): parent_faces initialization silently discards SetSize#253
Open
ahojukka5 wants to merge 2 commits into
Open
fix(topology): parent_faces initialization silently discards SetSize#253ahojukka5 wants to merge 2 commits into
ahojukka5 wants to merge 2 commits into
Conversation
MeshTopology::BuildParentFaces (build_parent_faces path) does:
parent_faces.SetSize (nfa);
parent_faces = { -1, { -1, -1, -1, -1 } };
The second line looks like a broadcast-initialize of every entry to a
default "no parent" sentinel, but Array::operator=(std::initializer_list<T>)
(core/array.hpp) is implemented as `*this = Array<T>(list); return *this;`
-- with a single-element list, this constructs a size-1 Array and assigns
it, silently REPLACING the array `SetSize(nfa)` had just allocated. The
array's true size after this line is 1, not nfa.
The loop immediately below only assigns parent_faces[i] for faces that go
through one of several conditional branches (a face on the coarse level, a
newly-created midpoint face, etc.) -- any face index that reaches none of
those branches is never explicitly written. Reading such an index via
GetParentFaces then returns data at an out-of-bounds offset into a size-1
array -- observed in practice as non-deterministic values in 3 of the 4
returned parent-face slots, varying between runs on identical input.
Fix: explicitly loop and initialize every one of the `nfa` entries to the
intended sentinel before the per-face population loop runs, instead of
relying on the broadcast-looking assignment.
There was a problem hiding this comment.
Pull request overview
Fixes a correctness issue in MeshTopology::BuildParentFaces where an initializer-list assignment unintentionally resizes parent_faces to length 1 after SetSize(nfa), leading to out-of-bounds reads via GetParentFaces.
Changes:
- Replace the misleading initializer-list assignment with explicit per-entry initialization to the intended “no parent” sentinel.
- Add an in-code note explaining why initializer-list assignment is unsafe here (due to
Array::operator=(std::initializer_list<T>)semantics).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…y parent_faces fix parent_edges had the identical Array::operator=(std::initializer_list<T>) pitfall as parent_faces: after SetSize(ned), the braced sentinel assignment silently collapsed the array to size 1, making GetParentEdges read out of bounds for any edge index > 0. Both initializations now assign a named, typed sentinel value instead of a braced literal, which binds to Array::operator=(const T&) and fills every existing entry in place without resizing or an explicit loop. Verified both call sites type-check under clang++ -fsyntax-only against the real Array<T> template in core/array.hpp.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MeshTopology::BuildParentFaces (build_parent_faces path) does:
The second line looks like a broadcast-initialize of every entry to a default "no parent" sentinel, but Array::operator=(std::initializer_list) (core/array.hpp) is implemented as
*this = Array<T>(list); return *this;-- with a single-element list, this constructs a size-1 Array and assigns it, silently REPLACING the arraySetSize(nfa)had just allocated. The array's true size after this line is 1, not nfa.The loop immediately below only assigns parent_faces[i] for faces that go through one of several conditional branches (a face on the coarse level, a newly-created midpoint face, etc.) -- any face index that reaches none of those branches is never explicitly written. Reading such an index via GetParentFaces then returns data at an out-of-bounds offset into a size-1 array -- observed in practice as non-deterministic values in 3 of the 4 returned parent-face slots, varying between runs on identical input.
Fix: explicitly loop and initialize every one of the
nfaentries to the intended sentinel before the per-face population loop runs, instead of relying on the broadcast-looking assignment.