Fix heap buffer overflow for high valence subdivision vertices - #632
Open
stefanatwork wants to merge 2 commits into
Open
stefanatwork wants to merge 2 commits into
stefanatwork wants to merge 2 commits into
Conversation
The one-ring construction in CatmullClark1RingT::init() and GeneralCatmullClark1RingT::init() wrote into the fixed capacity ring buffers (MAX_RING_FACE_VALENCE / MAX_RING_EDGE_VALENCE) without any runtime bound, only asserts guarded these writes. A vertex shared by more than MAX_RING_FACE_VALENCE faces (or a ring with more than MAX_RING_EDGE_VALENCE edges) thus corrupted the heap in release builds. The ring construction now stops when the ring buffers are exhausted and clamps the evaluation start indices accordingly, so no out of bounds write can happen for any topology. In addition patches whose rings exceed the supported size limits can now get detected in constant time through a flag that is calculated at commit time for every topology. rtcInterpolate and rtcInterpolateN, which did not perform any validity check before, use this flag to reject such patches and return zeros. The ray tracing path already rejected these patches through SubdivMesh::valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
svenwoop
requested changes
Sep 23, 2026
| PatchType patch_type; //!< stores type of subdiv patch | ||
| VertexType vertex_type; //!< stores type of the start vertex | ||
| char align[2]; | ||
| char valid_sizes; //!< stores if the patch and all its rings are within the supported size limits |
| /*! tests if the ring around the start vertex is within the supported size | ||
| * limits. In contrast to validRing this test only depends on the topology | ||
| * and not on the vertex positions. */ | ||
| __forceinline bool validRingSizes() const |
Collaborator
There was a problem hiding this comment.
I think we do not need that function at all, there is some SubdivMesh::valid() function that is used in the builder to detect valid faces. Same function should also be used to filter out invalid faces for interpolate.
|
|
||
| do | ||
| { | ||
| /* stop when the ring buffers are exhausted to avoid writing out of bounds */ |
Collaborator
There was a problem hiding this comment.
Do you understand all these changes in this file? As we now validate the size of the ring upfront, there should be no need for these checks at all!?
| foreach_unique(valid1,primID,[&](const vbool4& valid1, const unsigned int primID) | ||
| { | ||
| /* patches that exceed the supported valence limits cannot get evaluated */ | ||
| if (unlikely(!topo->getHalfEdge(primID)->hasValidSizes())) |
Collaborator
There was a problem hiding this comment.
Just to be on the save side, pull out topo->getHalfEdge(primID) into variable and use it here and in code below. want to avoid compiler to compute this twice.
svenwoop
requested changes
Sep 23, 2026
| /* we have to calculate patch_type last! */ | ||
| HalfEdge::PatchType patch_type = edge->patchType(); | ||
| for (size_t i=0; i<mesh->faceVertices[f]; i++) | ||
| const char valid_sizes = edge->validPatchSizes() ? 1 : 0; |
Collaborator
There was a problem hiding this comment.
Should use the mesh->valid(i) function to tag a valid face/edge. This same function is also used by the builder to filter out invalid faces, we should stay consistent here.
Fold the high-valence subdivision patch check into the existing Topology::valid() path instead of carrying a separate hasValidSizes() query at interpolation sites. The cached half-edge flag is now a bool and interpolation uses the same validity model as the builders, while still checking the selected attribute topology when vertex attributes are bound to a non-default topology. With invalid patches filtered before PatchEval, remove the defensive ring truncation code from the Catmull-Clark ring constructors and keep assertions for the invariants that commit-time validation guarantees. This avoids constructing partially truncated rings and keeps invalid patch handling centralized in the topology validity path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
svenwoop
requested changes
Sep 23, 2026
| const HalfEdge* halfEdge = topo->getHalfEdge(primID); | ||
|
|
||
| /* invalid patches cannot get evaluated */ | ||
| if (unlikely(!this->valid(primID) || !topo->valid(primID))) |
Collaborator
There was a problem hiding this comment.
I think here should just be topo->valid(primID)? This->valid() uses the main topology which is for geometry. But even if that is invalid, if the attribute topology is still ok all is fine.
| return faceValence <= MAX_RING_FACE_VALENCE && edgeValence <= MAX_RING_EDGE_VALENCE; | ||
| } | ||
|
|
||
| /*! tests if the ring around the start vertex is within the supported size |
Collaborator
There was a problem hiding this comment.
We do not need that function, one can just use topo->valid() ?
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
2 participants
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.
The one-ring construction in CatmullClark1RingT::init() and GeneralCatmullClark1RingT::init() wrote into the fixed capacity ring buffers (MAX_RING_FACE_VALENCE / MAX_RING_EDGE_VALENCE) without any runtime bound, only asserts guarded these writes. A vertex shared by more than MAX_RING_FACE_VALENCE faces (or a ring with more than MAX_RING_EDGE_VALENCE edges) thus corrupted the heap in release builds.
The ring construction now stops when the ring buffers are exhausted and clamps the evaluation start indices accordingly, so no out of bounds write can happen for any topology.
In addition patches whose rings exceed the supported size limits can now get detected in constant time through a flag that is calculated at commit time for every topology. rtcInterpolate and rtcInterpolateN, which did not perform any validity check before, use this flag to reject such patches and return zeros. The ray tracing path already rejected these patches through SubdivMesh::valid.