From 6ef1612dc9ea2d864657de0b86a4502e8c0fca25 Mon Sep 17 00:00:00 2001 From: "Werner, Stefan" Date: Mon, 21 Sep 2026 15:45:16 +0200 Subject: [PATCH 1/2] Fix heap buffer overflow for high valence subdivision vertices 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> --- kernels/common/scene_subdiv_mesh.cpp | 30 ++++++++++++- kernels/subdiv/catmullclark_ring.h | 29 ++++++++++++ kernels/subdiv/half_edge.h | 66 +++++++++++++++++++++++++++- 3 files changed, 122 insertions(+), 3 deletions(-) diff --git a/kernels/common/scene_subdiv_mesh.cpp b/kernels/common/scene_subdiv_mesh.cpp index 4dc2080d36..804831e27c 100644 --- a/kernels/common/scene_subdiv_mesh.cpp +++ b/kernels/common/scene_subdiv_mesh.cpp @@ -561,8 +561,11 @@ namespace embree /* we have to calculate patch_type last! */ HalfEdge::PatchType patch_type = edge->patchType(); - for (size_t i=0; ifaceVertices[f]; i++) + const char valid_sizes = edge->validPatchSizes() ? 1 : 0; + for (size_t i=0; ifaceVertices[f]; i++) { edge[i].patch_type = patch_type; + edge[i].valid_sizes = valid_sizes; + } } }); } @@ -878,6 +881,17 @@ namespace embree bool has_P = P; bool has_dP = dPdu; assert(!has_dP || dPdv); bool has_ddP = ddPdudu; assert(!has_ddP || (ddPdvdv && ddPdudu)); + + /* patches that exceed the supported valence limits cannot get evaluated */ + if (unlikely(!topo->getHalfEdge(primID)->hasValidSizes())) + { + for (unsigned int j=0; jgetHalfEdge(primID)->hasValidSizes())) + { + for (unsigned int j=0; j MAX_RING_EDGE_VALENCE)) + break; + vertex_level = max(vertex_level,p->edge_level); crease_weight[i/2] = p->edge_crease_weight; assert(p->hasOpposite() || p->edge_crease_weight == float(inf)); @@ -178,6 +182,9 @@ namespace embree if (index0 < min_vertex_index) { min_vertex_index = index0; min_vertex_index_face = i>>1; } /*! mark first border edge and store dummy vertex for face between the two border edges */ + if (unlikely(i+2 > MAX_RING_EDGE_VALENCE)) + break; + border_index = i; crease_weight[i/2] = inf; ring[i++] = Vertex_t::loadu(vertices+index0*stride); @@ -196,6 +203,10 @@ namespace embree eval_unique_identifier = min_vertex_index; eval_start_index = min_vertex_index_face; + /* the ring may have been truncated above, thus clamp the start index */ + if (unlikely(eval_start_index >= face_valence)) + eval_start_index = 0; + assert( hasValidPositions() ); } @@ -593,6 +604,10 @@ namespace embree vertex_level = 0.0f; do { + /* stop when the ring buffers are exhausted to avoid writing out of bounds */ + if (unlikely(f+1 > MAX_RING_FACE_VALENCE || e+1 > MAX_RING_EDGE_VALENCE)) + break; + HalfEdge* p_prev = p->prev(); HalfEdge* p_next = p->next(); const float crease_weight = p->edge_crease_weight; @@ -606,11 +621,16 @@ namespace embree /* store first N-2 vertices of face */ unsigned int vn = 0; for (p = p_next; p!=p_prev; p=p->next()) { + if (unlikely(e >= MAX_RING_EDGE_VALENCE)) break; ring[e++] = Vertex_t::loadu(vertices+p->getStartVertexIndex()*stride); vn++; } faces[f++] = Face(vn,crease_weight); only_quads &= (vn == 2); + + /* stop in case the face got truncated above */ + if (unlikely(p != p_prev)) + break; /* continue with next face */ if (likely(p->hasOpposite())) @@ -624,6 +644,9 @@ namespace embree if (vertex_index < min_vertex_index) { min_vertex_index = vertex_index; min_vertex_index_face = f; min_vertex_index_vertex = e; } /*! mark first border edge and store dummy vertex for face between the two border edges */ + if (unlikely(f+1 > MAX_RING_FACE_VALENCE || e+2 > MAX_RING_EDGE_VALENCE)) + break; + border_face = f; faces[f++] = Face(2,inf); ring[e++] = Vertex_t::loadu(vertices+p->getStartVertexIndex()*stride); @@ -643,6 +666,12 @@ namespace embree eval_start_face_index = min_vertex_index_face; eval_start_vertex_index = min_vertex_index_vertex; + /* the ring may have been truncated above, thus clamp the start indices */ + if (unlikely(eval_start_face_index >= face_valence)) + eval_start_face_index = 0; + if (unlikely(eval_start_vertex_index >= edge_valence)) + eval_start_vertex_index = 0; + assert( hasValidPositions() ); } diff --git a/kernels/subdiv/half_edge.h b/kernels/subdiv/half_edge.h index 8cb4f24845..b61ef1c013 100644 --- a/kernels/subdiv/half_edge.h +++ b/kernels/subdiv/half_edge.h @@ -48,7 +48,7 @@ namespace embree HalfEdge () : vtx_index(-1), next_half_edge_ofs(0), prev_half_edge_ofs(0), opposite_half_edge_ofs(0), edge_crease_weight(0), - vertex_crease_weight(0), edge_level(0), patch_type(COMPLEX_PATCH), vertex_type(REGULAR_VERTEX) + vertex_crease_weight(0), edge_level(0), patch_type(COMPLEX_PATCH), vertex_type(REGULAR_VERTEX), valid_sizes(0) { static_assert(sizeof(HalfEdge) == 32, "invalid half edge size"); } @@ -354,6 +354,67 @@ namespace embree return faceValence <= MAX_RING_FACE_VALENCE && edgeValence <= MAX_RING_EDGE_VALENCE; } + + /*! 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 + { + size_t faceValence = 0; + size_t edgeValence = 0; + + const HalfEdge* p = this; + do + { + /* check size of current face */ + const size_t n = p->numEdges(); + if (n < 3 || n > MAX_PATCH_VALENCE) + return false; + edgeValence += n-2; + + faceValence++; + p = p->prev(); + + /* continue with next face */ + if (likely(p->hasOpposite())) + p = p->opposite(); + + /* if there is no opposite go the long way to the other side of the border */ + else { + faceValence++; + edgeValence++; + p = this; + while (p->hasOpposite()) + p = p->opposite()->next(); + } + + /* stop early for degenerated topology */ + if (faceValence > MAX_RING_FACE_VALENCE || edgeValence > MAX_RING_EDGE_VALENCE) + return false; + + } while (p != this); + + return true; + } + + public: + + /*! tests if this patch and all its rings are within the supported size + * limits. Patches that are not, cannot get evaluated. */ + __forceinline bool validPatchSizes() const + { + size_t N = 1; + if (!this->validRingSizes()) return false; + for (const HalfEdge* p=this->next(); p!=this; p=p->next(), N++) { + if (!p->validRingSizes()) return false; + } + return N >= 3 && N <= MAX_PATCH_VALENCE; + } + + /*! returns the cached result of validPatchSizes computed at commit time */ + __forceinline bool hasValidSizes() const { + return valid_sizes != 0; + } private: unsigned int vtx_index; //!< index of edge start vertex @@ -367,6 +428,7 @@ namespace embree float edge_level; //!< subdivision factor for edge 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 + char align[1]; }; } From 126d1247b4f0f16cc0b356f2479c2e47c3b1156c Mon Sep 17 00:00:00 2001 From: "Werner, Stefan" Date: Wed, 23 Sep 2026 10:11:55 +0200 Subject: [PATCH 2/2] Address subdivision validity review feedback 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> --- kernels/common/scene_subdiv_mesh.cpp | 20 +++++++++------- kernels/common/scene_subdiv_mesh.h | 2 ++ kernels/subdiv/catmullclark_ring.h | 35 +++++----------------------- kernels/subdiv/half_edge.h | 17 +++++--------- 4 files changed, 26 insertions(+), 48 deletions(-) diff --git a/kernels/common/scene_subdiv_mesh.cpp b/kernels/common/scene_subdiv_mesh.cpp index 804831e27c..2545b506b9 100644 --- a/kernels/common/scene_subdiv_mesh.cpp +++ b/kernels/common/scene_subdiv_mesh.cpp @@ -561,10 +561,10 @@ namespace embree /* we have to calculate patch_type last! */ HalfEdge::PatchType patch_type = edge->patchType(); - const char valid_sizes = edge->validPatchSizes() ? 1 : 0; + const bool valid = edge->validPatchTopology(); for (size_t i=0; ifaceVertices[f]; i++) { edge[i].patch_type = patch_type; - edge[i].valid_sizes = valid_sizes; + edge[i].valid_patch = valid; } } }); @@ -882,8 +882,10 @@ namespace embree bool has_dP = dPdu; assert(!has_dP || dPdv); bool has_ddP = ddPdudu; assert(!has_ddP || (ddPdvdv && ddPdudu)); - /* patches that exceed the supported valence limits cannot get evaluated */ - if (unlikely(!topo->getHalfEdge(primID)->hasValidSizes())) + const HalfEdge* halfEdge = topo->getHalfEdge(primID); + + /* invalid patches cannot get evaluated */ + if (unlikely(!this->valid(primID) || !topo->valid(primID))) { for (unsigned int j=0; j(baseEntry->at(interpolationSlot(primID,i/4,stride)),commitCounter, - topo->getHalfEdge(primID),src+i*sizeof(float),stride,u,v, + halfEdge,src+i*sizeof(float),stride,u,v, has_P ? &Pt : nullptr, has_dP ? &dPdut : nullptr, has_dP ? &dPdvt : nullptr, @@ -980,8 +982,10 @@ namespace embree 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())) + const HalfEdge* halfEdge = topo->getHalfEdge(primID); + + /* invalid patches cannot get evaluated */ + if (unlikely(!this->valid(primID) || !topo->valid(primID))) { for (unsigned int j=0; j(baseEntry->at(interpolationSlot(primID,j/4,stride)),commitCounter, - topo->getHalfEdge(primID),src+j*sizeof(float),stride,valid1,uu,vv, + halfEdge,src+j*sizeof(float),stride,valid1,uu,vv, P ? P+j*N+i : nullptr, dPdu ? dPdu+j*N+i : nullptr, dPdv ? dPdv+j*N+i : nullptr, diff --git a/kernels/common/scene_subdiv_mesh.h b/kernels/common/scene_subdiv_mesh.h index 49bb8e769a..d99988f4da 100644 --- a/kernels/common/scene_subdiv_mesh.h +++ b/kernels/common/scene_subdiv_mesh.h @@ -169,6 +169,8 @@ namespace embree /*! check if the i'th primitive is valid in this topology */ __forceinline bool valid(size_t i) const { + if (unlikely(!getHalfEdge(i)->valid_patch)) + return false; if (unlikely(subdiv_mode == RTC_SUBDIVISION_MODE_NO_BOUNDARY)) { if (getHalfEdge(i)->faceHasBorder()) return false; } diff --git a/kernels/subdiv/catmullclark_ring.h b/kernels/subdiv/catmullclark_ring.h index ad11657188..1a046a0312 100644 --- a/kernels/subdiv/catmullclark_ring.h +++ b/kernels/subdiv/catmullclark_ring.h @@ -151,10 +151,7 @@ namespace embree do { - /* stop when the ring buffers are exhausted to avoid writing out of bounds */ - if (unlikely(i+2 > MAX_RING_EDGE_VALENCE)) - break; - + assert(i+2 <= MAX_RING_EDGE_VALENCE); vertex_level = max(vertex_level,p->edge_level); crease_weight[i/2] = p->edge_crease_weight; assert(p->hasOpposite() || p->edge_crease_weight == float(inf)); @@ -182,9 +179,7 @@ namespace embree if (index0 < min_vertex_index) { min_vertex_index = index0; min_vertex_index_face = i>>1; } /*! mark first border edge and store dummy vertex for face between the two border edges */ - if (unlikely(i+2 > MAX_RING_EDGE_VALENCE)) - break; - + assert(i+2 <= MAX_RING_EDGE_VALENCE); border_index = i; crease_weight[i/2] = inf; ring[i++] = Vertex_t::loadu(vertices+index0*stride); @@ -203,10 +198,6 @@ namespace embree eval_unique_identifier = min_vertex_index; eval_start_index = min_vertex_index_face; - /* the ring may have been truncated above, thus clamp the start index */ - if (unlikely(eval_start_index >= face_valence)) - eval_start_index = 0; - assert( hasValidPositions() ); } @@ -604,10 +595,6 @@ namespace embree vertex_level = 0.0f; do { - /* stop when the ring buffers are exhausted to avoid writing out of bounds */ - if (unlikely(f+1 > MAX_RING_FACE_VALENCE || e+1 > MAX_RING_EDGE_VALENCE)) - break; - HalfEdge* p_prev = p->prev(); HalfEdge* p_next = p->next(); const float crease_weight = p->edge_crease_weight; @@ -621,16 +608,13 @@ namespace embree /* store first N-2 vertices of face */ unsigned int vn = 0; for (p = p_next; p!=p_prev; p=p->next()) { - if (unlikely(e >= MAX_RING_EDGE_VALENCE)) break; + assert(e < MAX_RING_EDGE_VALENCE); ring[e++] = Vertex_t::loadu(vertices+p->getStartVertexIndex()*stride); vn++; } + assert(f < MAX_RING_FACE_VALENCE); faces[f++] = Face(vn,crease_weight); only_quads &= (vn == 2); - - /* stop in case the face got truncated above */ - if (unlikely(p != p_prev)) - break; /* continue with next face */ if (likely(p->hasOpposite())) @@ -644,9 +628,8 @@ namespace embree if (vertex_index < min_vertex_index) { min_vertex_index = vertex_index; min_vertex_index_face = f; min_vertex_index_vertex = e; } /*! mark first border edge and store dummy vertex for face between the two border edges */ - if (unlikely(f+1 > MAX_RING_FACE_VALENCE || e+2 > MAX_RING_EDGE_VALENCE)) - break; - + assert(f < MAX_RING_FACE_VALENCE); + assert(e+2 <= MAX_RING_EDGE_VALENCE); border_face = f; faces[f++] = Face(2,inf); ring[e++] = Vertex_t::loadu(vertices+p->getStartVertexIndex()*stride); @@ -666,12 +649,6 @@ namespace embree eval_start_face_index = min_vertex_index_face; eval_start_vertex_index = min_vertex_index_vertex; - /* the ring may have been truncated above, thus clamp the start indices */ - if (unlikely(eval_start_face_index >= face_valence)) - eval_start_face_index = 0; - if (unlikely(eval_start_vertex_index >= edge_valence)) - eval_start_vertex_index = 0; - assert( hasValidPositions() ); } diff --git a/kernels/subdiv/half_edge.h b/kernels/subdiv/half_edge.h index b61ef1c013..7fbd7892d2 100644 --- a/kernels/subdiv/half_edge.h +++ b/kernels/subdiv/half_edge.h @@ -48,7 +48,7 @@ namespace embree HalfEdge () : vtx_index(-1), next_half_edge_ofs(0), prev_half_edge_ofs(0), opposite_half_edge_ofs(0), edge_crease_weight(0), - vertex_crease_weight(0), edge_level(0), patch_type(COMPLEX_PATCH), vertex_type(REGULAR_VERTEX), valid_sizes(0) + vertex_crease_weight(0), edge_level(0), patch_type(COMPLEX_PATCH), vertex_type(REGULAR_VERTEX), valid_patch(false) { static_assert(sizeof(HalfEdge) == 32, "invalid half edge size"); } @@ -358,7 +358,7 @@ namespace embree /*! 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 + __forceinline bool validRingTopology() const { size_t faceValence = 0; size_t edgeValence = 0; @@ -401,20 +401,15 @@ namespace embree /*! tests if this patch and all its rings are within the supported size * limits. Patches that are not, cannot get evaluated. */ - __forceinline bool validPatchSizes() const + __forceinline bool validPatchTopology() const { size_t N = 1; - if (!this->validRingSizes()) return false; + if (!this->validRingTopology()) return false; for (const HalfEdge* p=this->next(); p!=this; p=p->next(), N++) { - if (!p->validRingSizes()) return false; + if (!p->validRingTopology()) return false; } return N >= 3 && N <= MAX_PATCH_VALENCE; } - - /*! returns the cached result of validPatchSizes computed at commit time */ - __forceinline bool hasValidSizes() const { - return valid_sizes != 0; - } private: unsigned int vtx_index; //!< index of edge start vertex @@ -428,7 +423,7 @@ namespace embree float edge_level; //!< subdivision factor for edge PatchType patch_type; //!< stores type of subdiv patch VertexType vertex_type; //!< stores type of the start vertex - char valid_sizes; //!< stores if the patch and all its rings are within the supported size limits + bool valid_patch; //!< stores if the patch can be evaluated char align[1]; }; }