Fix several undefined behavior issues (strict aliasing and ODR violations) #4594
+40
−110
Conversation
This file contains 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
Dereferencing type-punned pointers breaks C++'s strict aliasing rules and invokes undefined behavior. Use memcpy() to copy the float to an unsigned int variable to access the bit pattern.
The macros GIM_IR(), GIM_SIR(), GIM_AIR(), and GIM_FR() dereference type-punned pointers, which is UB. The functions gim_inv_sqrt() and gim_sqrt() use these broken macros. Introduce two small inline functions copying the bits between the icompatible float and integer types using memcpy() and rewrite the macros so they use these functions. Note that three of the macros returned a reference before the change and thus could theoretically be used as lvalue; this is no longer possible. I couldn't find such usage in the code base and it would be rather odd for 3rd party code to abuse the API like that. However, if it is required that these macros return lvalues, then a more complicated approach would be needed with a custom class wrapping the reference and performing the copy on each access.
Use b3MakeInt2() to contruct an object of the required type instead of type-punning b3SortData fillValue in a call to m_fill->execute() in b3RadixSort32CL::execute(b3OpenCLArray<b3SortData>&, int).
csCfg of type b3ConstraintCfg is type-punned to b3SolverBase::ConstraintCfg& in solveContacts(). The two types are incompatible and this is UB. But they have almost the same definition, only m_staticIdx is initialized to a different value in the constructor. Declare b3ConstraintCfg to be a subclass of b3SolverBase::ConstraintCfg so we can convert between them.
The code tries to reinterpret an int as a void* with type-punning. This is not only a strict aliasing violation, but completely invalid on architectures where ints and pointers have different size. The leaf btDbvtNode's payload is a union which contains an int dataAsInt member, so we set that instead of void *data.
GIM_CONTACT is defined in btContactProcessingStructs.h and gim_contact.h, but with subtle differences, so there are two different types with the same name. This violates the C++ One Definition Rule and causes undefined behavior. Fix this by always using the same definition. Since gim_contact.h only provides the definition if btContactProcessingStructs.h isn't included before, we delete the diverging duplicate definition from the former header and #include the latter to provide the definition. Also drop unnecessary headers from btContactProcessingStructs.h and move over a comment from the deleted variant.
Both btSoftMultiBodyDynamicsWorld.cpp and btSoftRigidDynamicsWorld.cpp define a type btSoftSingleRayCallback. Though the types are very similar they are not identical, causing an ODR violation. Move the type definitions into anonymous namespaces so their names no longer collide.
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.
These changes should fix issue #4590.