Fix for buffer overflow in OnOverlapCreatedTask #7
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
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.
|
Just because the memory is allocated doesn't mean its contents are valid. Based on your description, it doesn't sound like a good fix. |
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.
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.