Validate buffer type and slot in rtcInterpolate/rtcInterpolateN - #629
Open
stefanatwork wants to merge 1 commit into
Open
stefanatwork wants to merge 1 commit into
stefanatwork wants to merge 1 commit into
Conversation
The bufferType and bufferSlot members of RTCInterpolateArguments and RTCInterpolateNArguments are passed from the public API straight into the per geometry interpolate_impl() routines, where the slot is used to index the fixed size vertices, tangents and vertexAttribs arrays of buffer descriptors. The only guard was an assert that is removed in release builds, and that was additionally off by one for vertex attributes as it used bufferSlot <= vertexAttribs.size() instead of <. An out of range slot therefore read a buffer descriptor past the end of the descriptor array. The resulting pointer and stride were then used to load vertex data, and the loaded bytes were written into the caller visible interpolation output arrays. This affects all interpolatable geometry types, both the vertex and the vertex attribute buffer branch, and both rtcInterpolate and rtcInterpolateN. A buffer type other than RTC_BUFFER_TYPE_VERTEX or RTC_BUFFER_TYPE_VERTEX_ATTRIBUTE was silently treated as a vertex buffer instead of being rejected. Add Geometry::checkInterpolateBuffer(), which rejects unsupported buffer types, vertex slots that are not smaller than numTimeSteps, and vertex attribute slots that are not smaller than the vertex attribute count. These bounds also cover the vertex_buffer_tags and vertex_attrib_buffer_tags arrays of subdivision meshes, which are sized by the same two counts. Invalid requests now raise RTC_ERROR_INVALID_ARGUMENT and interpolate no data. Use the helper at all interpolation sites, replacing the asserts, and document the requirement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Collaborator
|
This function will be used during rendering, do we really want to put runtime checks here? IMHO the asserts are fine and API documentation should specify what input to expect. If we add checks then usage of checkInterpolateBuffer is not optimal as this adds additional branching on bufferType that the code anyway does already. There were some asserts inside the if (bufferType == RTC_BUFFER_TYPE_VERTEX_ATTRIBUTE) conditional that can get converted to exception. |
svenwoop
requested changes
Sep 23, 2026
| /* calculate base pointer and stride */ | ||
| assert((bufferType == RTC_BUFFER_TYPE_VERTEX && bufferSlot < numTimeSteps) || | ||
| (bufferType == RTC_BUFFER_TYPE_VERTEX_ATTRIBUTE && bufferSlot <= vertexAttribs.size())); | ||
| checkInterpolateBuffer(bufferType,bufferSlot,vertexAttribs.size()); |
Collaborator
There was a problem hiding this comment.
adds additional branching that is anyway present below
| /* we interpolate vertex attributes linearly for hermite basis */ | ||
| if (bufferType == RTC_BUFFER_TYPE_VERTEX_ATTRIBUTE) | ||
| { | ||
| assert(bufferSlot <= vertexAttribs.size()); |
Collaborator
There was a problem hiding this comment.
this should be converted to exception
| @@ -732,7 +732,6 @@ | |||
| /* interpolation for vertex buffers */ | |||
| else | |||
Collaborator
There was a problem hiding this comment.
here enter only on VERTEX_BUFFER type
| if (ddPdudu) mem<vfloat<N>>::storeu(valid,ddPdudu+i,curve.eval_dudu(u)); | ||
| } | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
here add also and raise exception when wrong buffer type used
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 bufferType and bufferSlot members of RTCInterpolateArguments and RTCInterpolateNArguments are passed from the public API straight into the per geometry interpolate_impl() routines, where the slot is used to index the fixed size vertices, tangents and vertexAttribs arrays of buffer descriptors. The only guard was an assert that is removed in release builds, and that was additionally off by one for vertex attributes as it used bufferSlot <= vertexAttribs.size() instead of <.
An out of range slot therefore read a buffer descriptor past the end of the descriptor array. The resulting pointer and stride were then used to load vertex data, and the loaded bytes were written into the caller visible interpolation output arrays.
This affects all interpolatable geometry types, both the vertex and the vertex attribute buffer branch, and both rtcInterpolate and rtcInterpolateN. A buffer type other than RTC_BUFFER_TYPE_VERTEX or RTC_BUFFER_TYPE_VERTEX_ATTRIBUTE was silently treated as a vertex buffer instead of being rejected.
Add Geometry::checkInterpolateBuffer(), which rejects unsupported buffer types, vertex slots that are not smaller than numTimeSteps, and vertex attribute slots that are not smaller than the vertex attribute count. These bounds also cover the vertex_buffer_tags and vertex_attrib_buffer_tags arrays of subdivision meshes, which are sized by the same two counts. Invalid requests now raise RTC_ERROR_INVALID_ARGUMENT and interpolate no data. Use the helper at all interpolation sites, replacing the asserts, and document the requirement.