-
Notifications
You must be signed in to change notification settings - Fork 11
[metal backend] fix buffer overruns #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ze, otherwise setVertexBytes() / setFragmentBytes() read beyond end of buffer
…ommon parent class (ExtendedRenderTexture) and move barrierStages up to ExtendedRenderTexture
|
|
||
| struct ExtendedRenderTexture : RenderTexture { | ||
| RenderTextureDesc desc; | ||
| RenderBarrierStages barrierStages = RenderBarrierStage::NONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the right approach, I think it was just a failure on our side. ExtendedRenderTexture is any renderable texture which may be a drawable or not.
dcvz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this!
squidbus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the comment already left.
Co-authored-by: dcvz <david@dcvz.io>
|
Committed the suggested change. Thanks for reviewing. |
|
Thanks again @gcsmith ! |
Signature 1:
(this is likely the same root cause as rt64/rt64#218)
The solution is to alignUp() the buffer size so that
pushConstants[rangeIndex].data.size()matchespushConstants[rangeIndex].size. Otherwise, setVertexBytes() reads beyond the end of the buffer.Signature 2:
There is an invalid cast from an instance of
MetalDrawable(fetched viaMetalSwapChain::getTexture()) toMetalTexturewhen passed toMetalCommandList::barriers().They share the same parent class
ExtendedRenderTexture, butMetalTextureis not aMetalDrawableso when we access the memberinterfaceTexture->barrierStageswe are again reading an OOB address wheninterfaceTexture == &drawables[2](wheredrawableshas 3 items), and for indices 0 and 1 we alias the wrong data.My change fixes the invalid access by casting to the parent class (and moving the
barrierStagesstate up toExtendedRenderTexturefor that to work), but it wasn't clear to me what the original intent was (e.g. wasMetalTexturesupposed to extendMetalDrawable?). This fixes the bug and the MM recomp now runs without crashing, but I'm not sure if this fix is architecturally correct.