Fix Memory Leak in System.DirectoryServices.Protocols caused by not freeing native memory #41065
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
…reeing native memory
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
joperezr
added
area-System.DirectoryServices
and removed
area-CodeGen-coreclr
Member
|
approved - significant memory leak, reliability issue, customer reported, regression. |
danmoseley
approved these changes
Aug 19, 2020
tarekgh
approved these changes
Aug 19, 2020
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Porting #41053 to release/5.0
Fixes #40946
Description
During a recent refactoring of System.DirectoryServices.Protocols code that added Linux implementation for the library, one bug was introduced since we started calling the wrong PInvoke in order to free some native resources which lead to a memory leak that would hit both the Windows and Unix case. This PR will make sure we call the right free instead so that the native resources are freed up correctly.
Customer Impact
Customers will not get a memory leak regression.
Regression?
Yes
Risk
low. Extensive testing was performed using both Benchmark.NET and the VS profiler in order to validate that these changes are now correctly freeing up the resources.
Test changes in this PR
We don't have the ability to test against a real LDAP server yet which is what would have caught this issue, but we have had several test runs since the original change was introduced where we would have native memory corruption and test execution would come to a halt. This change will make it so that will stop happening as we now know who was freeing up memory incorrectly.
cc: @danmosemsft @tarekgh