From a8decae13072baea6912cd1fb311cd9bcf17fec2 Mon Sep 17 00:00:00 2001 From: henryfalconer Date: Thu, 26 Jan 2017 15:57:15 +0000 Subject: [PATCH] Fix for buffer overflow in OnOverlapCreatedTask After a crash occurred in our game, I found out that OnOverlapCreatedTask::runInternal could read off the end of an allocated buffer. The local variables currentSI and currentEI are pointers into the mShapeInteractions and mInteractionMarkers buffers respectively. Whenever an item in the buffer is used by an interaction, the pointer into the buffer (currentSI or currentEI) is incremented. Once all the items in the buffer have been used, the pointer will point to the memory address directly after the buffer, so if any code dereferences the pointer, it will be reading memory out of bounds, which rarely causes problems in practice but can occasionally cause a crash. The problem is that the call to mNPhaseCore->createRbElementInteraction on line 6232 dereferences both currentSI and currentEI, so once one of those pointers reaches the end of its buffer, this line will perform an invalid read. My change makes the affected buffers one element larger than they need to be, so that OnOverlapCreatedTask doesn't read invalid memory. This seemed better for performance than adding a check to ensure that the memory reads are within the bounds of the buffer, and the memory overhead is negligible. I previously had a 100% repro for a crash caused by this bug, and after making this change the crash no longer occurs. --- .../Source/SimulationController/src/ScScene.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/PhysX_3.4/Source/SimulationController/src/ScScene.cpp b/PhysX_3.4/Source/SimulationController/src/ScScene.cpp index 478693904..9903bf4ec 100644 --- a/PhysX_3.4/Source/SimulationController/src/ScScene.cpp +++ b/PhysX_3.4/Source/SimulationController/src/ScScene.cpp @@ -6314,13 +6314,13 @@ void Sc::Scene::preallocateContactManagers(PxBaseTask* continuation) { //We allocate at least 1 element in this array to ensure that the onOverlapCreated functions don't go bang! - mPreallocatedContactManagers.reserve(totalCreatedPairs); - mPreallocatedShapeInteractions.reserve(totalCreatedPairs); - mPreallocatedInteractionMarkers.reserve(totalSuppressPairs); + mPreallocatedContactManagers.reserve(totalCreatedPairs + 1); + mPreallocatedShapeInteractions.reserve(totalCreatedPairs + 1); + mPreallocatedInteractionMarkers.reserve(totalSuppressPairs + 1); - mPreallocatedContactManagers.forceSize_Unsafe(totalCreatedPairs); - mPreallocatedShapeInteractions.forceSize_Unsafe(totalCreatedPairs); - mPreallocatedInteractionMarkers.forceSize_Unsafe(totalSuppressPairs); + mPreallocatedContactManagers.forceSize_Unsafe(totalCreatedPairs + 1); + mPreallocatedShapeInteractions.forceSize_Unsafe(totalCreatedPairs + 1); + mPreallocatedInteractionMarkers.forceSize_Unsafe(totalSuppressPairs + 1); }