Fix heap corruption issue in DriverProxy #67
Closed
Conversation
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
This commit fixes an issue where the buffer used by the DriverProxy class can become unpinned causing clients to send corrupt messages to the Media Driver. UnsafeBuffer has a constructor that takes a ByteBuffer. ByteBuffer contains a pointer to a managed array that has been pinned with a GCHandle. When an UnsafeBuffer is constructed from a ByteBuffer it takes a copy of the pointer, but does not take ownership of the GCHandle. If the ByteBuffer is allowed to be garbage collected the GCHandle is freed in its finalizer unpinning the managed array. The unpinned array may be moved by the garbage collector leaving UnsafeBuffer pointing to invalid memory. This commit fixes the bug by ensuring that we keep a reference to the ByteBuffer that is wrapped by the UnsafeBuffer in DriverProxy. This prevents the garbage collector from calling the ByteBuffer's finalizer and unpinning the managed array whilst it is still being used by the DriverProxy. It also fixes a similar issue in the SimplePublisher sample.
|
Good catch! My preference would be to keep the ByteBuffer in a field of the UnsafeBuffer. Let me know if you want to give it a go or I can sort it. |
|
Something like this https://github.com/JPWatson/Aeron.NET/tree/premature-buffer-disposable |
|
Fixed. Thank you for you help. |
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.
This commit fixes an issue where the buffer used by the DriverProxy class can become unpinned causing clients to send corrupt messages to the Media Driver.
UnsafeBuffer has a constructor that takes a ByteBuffer. ByteBuffer contains a pointer to a managed array that has been pinned with a GCHandle. When an UnsafeBuffer is constructed from a ByteBuffer it takes a copy of the pointer, but does not take ownership of the GCHandle. If the ByteBuffer is allowed to be garbage collected the GCHandle is freed in its finalizer unpinning the managed array. The unpinned array may be moved by the garbage collector leaving UnsafeBuffer pointing to invalid memory.
This commit fixes the bug by ensuring that we keep a reference to the ByteBuffer that is wrapped by the UnsafeBuffer in DriverProxy. This prevents the garbage collector from calling the ByteBuffer's finalizer and unpinning the managed array whilst it is still being used by the DriverProxy. It also fixes a similar issue in the SimplePublisher sample.
An more invasive, but cleaner, fix would be to give UnsafeBuffer a constructor that did the job of the ByteBuffer - creating a pointer to an array with the correct alignment. I think that in all cases that a ByteBuffer is used it's immediately passed into an UnsafeBuffer. Please let me know if you would prefer that instead.