Repository navigation
Fix heap buffer overflow for high valence subdivision vertices - #632
stefanatwork wants to merge 10 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>
|
|
||
| do | ||
| { | ||
| /* stop when the ring buffers are exhausted to avoid writing out of bounds */ |
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!?
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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep m128i() and m128d() available when compiling with Emscripten. SSE2 equality, shuffle, and masked integer loads call the explicit accessors regardless of platform. Continue excluding the implicit integer and double conversion operators for Emscripten, and leave the MSVC ARM64 path unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| bool valid = edge->validPatchTopology(); | ||
| const HalfEdge* geometryEdge = &mesh->topology[0].halfEdges[mesh->faceStartEdge[f]]; | ||
| for (size_t t=0; t<mesh->numTimeSteps && valid; t++) { | ||
| valid = valid && geometryEdge->valid(mesh->vertices[t]); |
There was a problem hiding this comment.
I would really prefer to change here as little as possible. This call already checks topology, thus this separate topology with and without vertex check separation can just removed.
There was a problem hiding this comment.
Ok, I get now why you wanted to check only topology before. The code here is not correct, it uses the mesh->vertices[t] to potentially validate the attribute buffer topology.
Change valid() function to get pointer to buffer and let it skip the vertex check when that pointer is NULL. This can be done by keeping current valid function similar as now, just add the skip for NULL ptr.
Now this valid check going over all time steps should only get done when this == &mesh->topology[0], and there is code that does these checks already. When this->mesh->topology[0] then just pass NULL to valid which will only validate the topology.
Sorry, that is partly reverting, but this topology only check can be done less invasive.
| __forceinline bool validRing(const BufferView<Vec3fa>& vertices) const | ||
| private: | ||
| template <bool checkVertices = true> | ||
| __forceinline bool validRing(const BufferView<Vec3fa>* vertices) const |
There was a problem hiding this comment.
This function can just stay as it was and check topology and vertices always.
|
|
||
| /*! tests if this patch and all its rings are within the supported size | ||
| * limits. Patches that are not, cannot get evaluated. */ | ||
| __forceinline bool validPatchTopology() const |
There was a problem hiding this comment.
We do not need this, just always check topology and vertices, much smaller change. I really prefer not to change too much on this more complicated subdiv code.
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.