Repository navigation
Conversation
CCDSweep() loads lastTm0.p and lastTm1.p with V3LoadA(), an aligned 16-byte load. PxVec3 p is the last member of the 28-byte PxTransform, at offset 16, so the load always reads 4 bytes past the end of the transform. The caller, PxsCCDPair::sweepFindToi(), keeps lastTm0 and lastTm1 in 16-byte aligned stack locals, so AddressSanitizer reports the read as a stack-buffer-overflow as soon as a CCD pair is swept. The extra lane is masked to zero and never used, so the result is not affected, but the read is out of bounds. The same function already loads transform0.p and transform1.p with V3LoadU() a few lines below. Load the last transforms with V3LoadU() too, which builds the vector from the three components. PhysX 5 does not have this problem: its CCDSweep() takes PxTransform32, a 32-byte padded transform, so the aligned load stays inside the object. Signed-off-by: Jonas Karlsson <jonas.karlsson@qt.io>
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
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.
Loads
lastTm0.pandlastTm1.pinCCDSweep()withV3LoadU()instead ofV3LoadA(), so the sweep reads only the 12 bytes of the position.The bug
CCDSweep()inGuCCDSweepPrimitives.cpploads the positions of the last transforms withV3LoadA(), an aligned 16-byte load that masks the fourth lane to zero.PxVec3 pis the last member of the 28-bytePxTransform, at offset 16, so the load always reads 4 bytes past the end of the transform.The caller,
PxsCCDPair::sweepFindToi()inPxsCCD.cpp, keepslastTm0andlastTm1in 16-byte aligned stack locals (PX_ALIGN(16, PxTransform lastTm0)), so AddressSanitizer reports the read as a stack-buffer-overflow as soon as a CCD pair is swept. The masked lane is never used, so the sweep result is correct. This is an out-of-bounds read (undefined behavior), not a wrong answer.The same function already loads
transform0.pandtransform1.pwithV3LoadU()a few lines below, so the two aligned loads look like an oversight rather than a deliberate choice.V3LoadU()builds the vector with_mm_set_ps()from the three components.PhysX 5 does not have this problem: its
CCDSweep()takesPxTransform32, a 32-byte padded transform, so the aligned load stays inside the object. This change gives the 4.1 branch the same guarantee without changing the type.How to reproduce
Version: PhysX 4.1.2, branch
4.1at a2c0428Steps:
-fsanitize=address.PxSceneFlag::eENABLE_CCDand a static box.PxRigidBodyFlag::eENABLE_CCD, moving fast enough towards the box to be a CCD pair, and simulate one step.Expected: The sphere is stopped on top of the box and the sweep reads only the 12 bytes of each position.
Testing
The same change has shipped in the PhysX 4.1 copy bundled with Qt Quick 3D Physics since Qt 6.11 and 6.12 (https://codereview.qt-project.org/c/qt/qtquick3dphysics/+/758361). There AddressSanitizer reported the read from
CCDSweep()in a sphere-versus-box CCD test before the change and nothing after it, and the simulation results were unchanged.