-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add hooks to debug OpenSSL memory allocations #111539
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Adeel Mujahid <[email protected]>
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 8 changed files in this pull request and generated 1 comment.
Files not reviewed (6)
- src/native/libs/System.Security.Cryptography.Native/apibridge_30.h: Language not supported
- src/native/libs/System.Security.Cryptography.Native/entrypoints.c: Language not supported
- src/native/libs/System.Security.Cryptography.Native/openssl.c: Language not supported
- src/native/libs/System.Security.Cryptography.Native/openssl.h: Language not supported
- src/native/libs/System.Security.Cryptography.Native/opensslshim.h: Language not supported
- src/native/libs/System.Security.Cryptography.Native/pal_ssl.c: Language not supported
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Crypto.cs
Outdated
Show resolved
Hide resolved
int newCount; | ||
struct memoryEntry* entry = (struct memoryEntry*)((char*)ptr - sizeof(struct memoryEntry)); | ||
CRYPTO_atomic_add(&g_allocatedMemory, (int)-entry->size, &newCount, g_allocLock); | ||
if (g_memoryCallback != NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling managed code from the free method is going to have reliability issues. The problem is that OpenSSL can call free very late during thread destruction after the .NET runtime cleaned up its own per-thread data structures. It will cause the per-thread .NET runtime data structures to be partially reinitialized and stuck in a weird state. This weird state will manifest as memory leak and a hard to diagnose crash later.
Take a look at #110442 to see the crash caused by calling back managed code from OpenSSL malloc/free and what it took to diagnose it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there some way to know if runtime or thread is going down @jkotas? I don't envision this to be enabled often or in production and we can also document the dragons ... or keep it hidden and make it debug hook for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think there is a reliable way to detect that the thread is in the middle of being shutdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd recommend (if you want to get some sort of memory tracking in) to instead write any memory tracking code in native code. It's not as nice as managed code, but as @jkotas points out, there's no reliable way to do this in managed code.
This ressurects #101626. CC: @wfurt.
Changes since his PR:
Open questions:
#if DEBUG
?We had several cases when users complained about large memory use. For than native it is quite difficult to figure out where the memory goes. This PR aims to make that somewhat easier.
OpenSSL provides hooks for memory function so this PR adds switch to optimally hook into that.
The only one caveat that the
CRYPTO_set_mem_functions
works only if called before any allocations e.g. it needs to be done very early in the process. So I end up putting into initialization process.The simple use pattern is something like
Dumping large allocation data set is slow and expensive. It is done under local so it blocks all other OpenSSL allocations. I feel this is ok for now but it should be used with caution. I also feel that access through Reflection is OK since this is only last resort debug hook e.g. it does not need stable API and convenient access.