Skip to content

Conversation

@joperezr
Copy link
Member

Fixes #40946
Fixes #39009

During the refactoring of S.DS.P, we changed some code that would get the search results back from an LDAP server and accidentally started calling the wrong native call to free the memory coming back from that result, causing it to a) have a big memory leak especially when we were getting a lot of results back and also b) we were sometimes causing a SegFault abort when we would sometimes free memory we didn't own. This PR will call ldap_msgfree instead of ldap_memfree to fix that issue and I have validated that the memory leak is fixed after this, and we don't have consistent repro for the memory corruption issue but will be most likely fixed by this as well.

cc: @tarekgh @danmosemsft @jkotas

@joperezr
Copy link
Member Author

Here is the documentation that says that the right way to free this memory is by calling ldap_msgfree instead of ldap_memfree as we were doing incorrectly:

https://docs.microsoft.com/en-us/windows/win32/api/winldap/nf-winldap-ldap_result

res
Contains the result(s) of the operation. Any results returned should be freed with a call to ldap_msgfree once they are no longer required by the application.

@joperezr joperezr requested a review from tarekgh August 19, 2020 19:36
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks to @uiopak that demonstrated it was all OSes and started with the Linux work. I see it was correctly using ldap_msgfree until then.

@danmoseley
Copy link
Member

I see we use a bunch of different freeing functions

ldap_memfreeW
ldap_value_freeW
ldap_controls_freeW
ldap_msgfree
ber_free
ldap_value_free_len
ber_bvfree
ber_bvecfree
ldap_control_freeW

Is it worth a scan to try to check for any other mismatches?

@joperezr
Copy link
Member Author

Is it worth a scan to try to check for any other mismatches?

I did a check, seems like we are calling the right free on other places. I will merge this and keep running some more extensive perf tests to make sure we don't have any other issues on our distros.

@joperezr joperezr merged commit c0c8854 into dotnet:master Aug 19, 2020
@joperezr joperezr deleted the FixMemoryLeak branch August 19, 2020 22:36
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

4 participants