jrouwe / JoltPhysics

A multi core friendly rigid body physics and collision detection library. Written in C++. Suitable for games and VR applications. Used by Horizon Forbidden West.
MIT License
6.73k stars 449 forks source link

Freeing a batch of objects with non-trivial destructors in FixedSizeFreeList results in an infinite loop. #1267

Closed randomuserhi closed 1 month ago

randomuserhi commented 1 month ago

Creating a FixedSizeFreeList of an item with a non-trivial destructor causes this while loop to run forever:

do
{
    ObjectStorage &storage = GetStorage(object_idx);
    storage.mObject.~Object();
    object_idx = storage.mNextFreeObject.load(memory_order_relaxed);
}
while (object_idx != cInvalidObjectIndex);

This is because FixedSizeFreeList<T>::AddObjectToBatch never sets the last item in the batch linked list to cInvalidObjectIndex. reference

Either the last object in the batch needs to have mNextFreeObject set to cInvalidObjectIndex or the while loop in FixedSizeFreeList<T>::DestructObjectBatch needs to check the index is not equal to the batch's last index, also ensuring to do the last item (Simply changing the loop to while (object_idx != ioBatch.mLastObjectIndex) will skip the last item). Using ioBatch.size is another potential fix: For example, replacing the snippet here with:

// Call destructors
if constexpr (!is_trivially_destructible<Object>())
{
    uint32 object_idx = ioBatch.mFirstObjectIndex;
    for (uint32 count = ioBatch.mNumObjects; count != 0; --count)
    {
        ObjectStorage &storage = GetStorage(object_idx);
        storage.mObject.~Object();
        object_idx = storage.mNextFreeObject.load(memory_order_relaxed);
    }
}
jrouwe commented 1 month ago

Thanks for reporting! It seems that Jolt is not using that code path. Note that I see FixedSizeFreeList as an internal type and didn't expect anyone to use it. What are you using it for?

randomuserhi commented 1 month ago

I'm actually not using it, but instead reading through your implementation as a learning exercise and just happened to stumble upon it by pure chance!