Fix heap buffer overflow for high valence subdivision vertices by stefanatwork · Pull Request #632 · RenderKit/embree

stefanatwork · 2026-09-22T13:10:14Z

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.

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 · 2026-09-23T05:21:26Z

    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


this should be a bool

and just call it valid

The validity used by the builder and interpolation is exposed as Topology::valid(). validPatchTopology() is kept as the construction-time topology scan that initializes this bit, rather than a separate hot-path check.

Changed: valid_patch is now a bool, with the half-edge padding adjusted to preserve its size.

svenwoop · 2026-09-23T05:30:55Z

+    /*! 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


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.

Agreed that the builder and interpolation should consume the established validity API rather than repeat size checks. The topology-size traversal is retained only during topology construction to precompute the per-topology validity bit; builders see it through SubdivMesh::valid(), and interpolation now checks topo->valid().

svenwoop · 2026-09-23T05:33:43Z


      do
      {
+        /* stop when the ring buffers are exhausted to avoid writing out of bounds */


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!?

Agreed. I removed these ring-capacity assertions: supported ring sizes are established during topology construction, and invalid patches are filtered through the shared validity checks before ring initialization.

svenwoop · 2026-09-23T05:35:36Z

        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()))


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.

Done. The half-edge pointer is fetched once into halfEdge and reused for the evaluation call.

svenwoop · 2026-09-23T06:00:35Z

        /* 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;


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.

The builder uses SubdivMesh::valid(), which now reaches the topology validity bit. That bit is intentionally per-topology: using the mesh-wide geometry validity for an attribute topology could reject an otherwise-valid attribute patch. Geometry vertex/hole validity remains tracked separately in invalidFace.

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 · 2026-09-23T09:08:35Z

+      const HalfEdge* halfEdge = topo->getHalfEdge(primID);
+
+      /* invalid patches cannot get evaluated */
+      if (unlikely(!this->valid(primID) || !topo->valid(primID)))


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.

Agreed. Interpolation now checks only topo->valid(primID), so a valid attribute topology is not rejected solely because the geometry topology is invalid.

svenwoop · 2026-09-23T09:09:18Z

      return faceValence <= MAX_RING_FACE_VALENCE && edgeValence <= MAX_RING_EDGE_VALENCE;
    }
+
+    /*! tests if the ring around the start vertex is within the supported size


We do not need that function, one can just use topo->valid() ?

Done for the interpolation paths: they call topo->valid() directly. The topology scan remains only at construction to precompute the validity state that this accessor returns.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

svenwoop · 2026-10-13T05:00:56Z

+        if (!p->validRingTopology()) return false;
+      }
+      return N >= 3 && N <= MAX_PATCH_VALENCE;
+    }


This duplicate code is still there.

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>

svenwoop requested changes Sep 23, 2026

View reviewed changes

Tighten subdivision validity review fixes

777e2e2

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

svenwoop requested changes Oct 13, 2026

View reviewed changes

stefanatwork and others added 2 commits October 13, 2026 14:18

Merge branch 'master' into fix/subdiv-ring-valence-overflow

0bc1f5c

Conversation

stefanatwork commented Sep 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svenwoop Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

svenwoop Sep 23, 2026 •

edited

Loading