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 #101626
base: main
Are you sure you want to change the base?
Changes from all commits
a163dbf
9b5b3d3
ea6ac3f
0338b06
8744c2d
401178f
803fc94
cd55d63
638df97
dd9b017
d42f271
01f556f
173827a
a86c713
c0838ea
60e933e
7c03ee3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Runtime.InteropServices; | ||
using System.Security.Cryptography.X509Certificates; | ||
|
@@ -170,5 +171,117 @@ internal static byte[] GetDynamicBuffer<THandle>(NegativeSizeReadMethod<THandle> | |
|
||
return bytes; | ||
} | ||
|
||
[LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_GetMemoryUse")] | ||
internal static partial int GetMemoryUse(ref int memoryUse, ref int allocationCount); | ||
|
||
public static int GetOpenSslAllocatedMemory() | ||
{ | ||
int used = 0; | ||
int count = 0; | ||
GetMemoryUse(ref used, ref count); | ||
return used; | ||
} | ||
|
||
public static int GetOpenSslAllocationCount() | ||
{ | ||
int used = 0; | ||
int count = 0; | ||
GetMemoryUse(ref used, ref count); | ||
return count; | ||
} | ||
#if DEBUG | ||
[LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SetMemoryTracking")] | ||
private static unsafe partial int SetMemoryTracking(delegate* unmanaged<MemoryOperation, UIntPtr, UIntPtr, int, char*, int, void> trackingCallback); | ||
|
||
[StructLayout(LayoutKind.Sequential)] | ||
private unsafe struct MemoryEntry | ||
{ | ||
public int Size; | ||
public int Line; | ||
public char* File; | ||
} | ||
|
||
private enum MemoryOperation | ||
{ | ||
Malloc = 1, | ||
Realloc = 2, | ||
Free = 3, | ||
} | ||
|
||
private static readonly unsafe nuint Offset = (nuint)sizeof(MemoryEntry); | ||
private static HashSet<UIntPtr>? _allocations; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using ConcurrentDictionary<UIntPtr, something> would allow us to remove some of the locks and reduce contention. |
||
|
||
[UnmanagedCallersOnly] | ||
private static unsafe void MemoryTrackinCallback(MemoryOperation operation, UIntPtr ptr, UIntPtr oldPtr, int size, char* file, int line) | ||
{ | ||
ref MemoryEntry entry = ref *(MemoryEntry*)ptr; | ||
|
||
Debug.Assert(entry.File != null); | ||
Debug.Assert(ptr != UIntPtr.Zero); | ||
|
||
switch (operation) | ||
{ | ||
case MemoryOperation.Malloc: | ||
Debug.Assert(size == entry.Size); | ||
lock (_allocations!) | ||
{ | ||
_allocations!.Add(ptr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understood the original comment from @jkotas, then this is still a potential problem Or does that hold only for the malloc/free calls and not GC-allocated memory? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not safe to use any managed code if this can be called from places like thread destructor. #101626 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right. I split the change now into two parts. The basic counters are implemented in native as @janvorli suggested. That also eliminates need to fiddle with the crypto initialization. Now for the managed part. I made this whole section |
||
} | ||
break; | ||
case MemoryOperation.Realloc: | ||
lock (_allocations!) | ||
{ | ||
if ((IntPtr)oldPtr != IntPtr.Zero) | ||
{ | ||
_allocations!.Remove(oldPtr); | ||
} | ||
_allocations!.Add(ptr); | ||
} | ||
break; | ||
case MemoryOperation.Free: | ||
lock (_allocations!) | ||
{ | ||
_allocations!.Remove(ptr); | ||
} | ||
break; | ||
} | ||
} | ||
|
||
public static unsafe void EnableTracking() | ||
{ | ||
_allocations ??= new HashSet<UIntPtr>(); | ||
_allocations!.Clear(); | ||
SetMemoryTracking(&MemoryTrackinCallback); | ||
} | ||
|
||
public static unsafe void DisableTracking() | ||
{ | ||
SetMemoryTracking(null); | ||
_allocations!.Clear(); | ||
} | ||
|
||
public static unsafe Tuple<UIntPtr, int, string>[] GetIncrementalAllocations() | ||
{ | ||
if (_allocations == null || _allocations.Count == 0) | ||
{ | ||
return Array.Empty<Tuple<UIntPtr, int, string>>(); | ||
} | ||
|
||
lock (_allocations!) | ||
{ | ||
Tuple<UIntPtr, int, string>[] allocations = new Tuple<UIntPtr, int, string>[_allocations.Count]; | ||
int index = 0; | ||
foreach (UIntPtr ptr in _allocations) | ||
{ | ||
ref MemoryEntry entry = ref *(MemoryEntry*)ptr; | ||
allocations[index] = new Tuple<UIntPtr, int, string>(ptr + Offset, entry.Size, $"{Marshal.PtrToStringAnsi((IntPtr)entry.File)}:{entry.Line}"); | ||
index++; | ||
} | ||
|
||
return allocations; | ||
} | ||
} | ||
#endif | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -29,6 +29,7 @@ static OpenSsl() | |||
|
||||
internal static partial class CryptoInitializer | ||||
{ | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
static CryptoInitializer() | ||||
{ | ||||
if (EnsureOpenSslInitialized() != 0) | ||||
|
@@ -41,6 +42,7 @@ static CryptoInitializer() | |||
// these libraries will be unable to operate correctly. | ||||
throw new InvalidOperationException(); | ||||
} | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
} | ||||
|
||||
internal static void Initialize() | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,8 +23,12 @@ internal static partial class OpenSsl | |
{ | ||
private const string TlsCacheSizeCtxName = "System.Net.Security.TlsCacheSize"; | ||
private const string TlsCacheSizeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE"; | ||
private const string OpenSslDebugEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_OPENSSL_MEMORY_DEBUG"; | ||
private const SslProtocols FakeAlpnSslProtocol = (SslProtocols)1; // used to distinguish server sessions with ALPN | ||
private static readonly ConcurrentDictionary<SslProtocols, SafeSslContextHandle> s_clientSslContexts = new ConcurrentDictionary<SslProtocols, SafeSslContextHandle>(); | ||
#pragma warning disable CA1823 | ||
private static readonly bool MemoryDebug = GetMemoryDebug(); | ||
#pragma warning restore CA1823 | ||
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the field is not used anywhere, can we move the call to |
||
|
||
#region internal methods | ||
internal static SafeChannelBindingHandle? QueryChannelBinding(SafeSslHandle context, ChannelBindingKind bindingType) | ||
|
@@ -63,6 +67,23 @@ private static int GetCacheSize() | |
return cacheSize; | ||
} | ||
|
||
private static bool GetMemoryDebug() | ||
{ | ||
string? value = Environment.GetEnvironmentVariable(OpenSslDebugEnvironmentVariable); | ||
if (int.TryParse(value, CultureInfo.InvariantCulture, out int enabled) && enabled == 1) | ||
{ | ||
Interop.Crypto.GetOpenSslAllocationCount(); | ||
Interop.Crypto.GetOpenSslAllocatedMemory(); | ||
#if DEBUG | ||
Interop.Crypto.EnableTracking(); | ||
Interop.Crypto.GetIncrementalAllocations(); | ||
Interop.Crypto.DisableTracking(); | ||
#endif | ||
} | ||
|
||
return enabled == 1; | ||
} | ||
|
||
// This is helper function to adjust requested protocols based on CipherSuitePolicy and system capability. | ||
private static SslProtocols CalculateEffectiveProtocols(SslAuthenticationOptions sslAuthenticationOptions) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1478,6 +1478,109 @@ int32_t CryptoNative_OpenSslAvailable(void) | |
#endif | ||
} | ||
|
||
static CRYPTO_RWLOCK* g_allocLock = NULL; | ||
static int g_allocatedMemory; | ||
static int g_allocationCount; | ||
|
||
static CRYPTO_allocation_cb g_memoryCallback; | ||
|
||
struct memoryEntry | ||
{ | ||
int size; | ||
int line; | ||
const char* file; | ||
}; | ||
Comment on lines
+1487
to
+1492
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did we consider the alignment of this on 32-bit platforms? As jkotas said in one of the comments, this has 12B on 32 bit platforms, so after adjusting the returned ptr will not be aligned on 8B boundary. According to the docs, OpenSSL has OPENSSL_aligned_malloc, which it probably uses internally when it matters, but we should still verify that this does not cause any problems. |
||
|
||
static void* mallocFunction(size_t size, const char *file, int line) | ||
{ | ||
void* ptr = malloc(size + sizeof(struct memoryEntry)); | ||
if (ptr != NULL) | ||
{ | ||
int newCount; | ||
CRYPTO_atomic_add(&g_allocatedMemory, (int)size, &newCount, g_allocLock); | ||
CRYPTO_atomic_add(&g_allocationCount, 1, &newCount, g_allocLock); | ||
struct memoryEntry* entry = (struct memoryEntry*)ptr; | ||
entry->size = (int)size; | ||
entry->line = line; | ||
entry->file = file; | ||
|
||
if (g_memoryCallback != NULL) | ||
{ | ||
g_memoryCallback(MallocOperation, ptr, NULL, entry->size, file, line); | ||
} | ||
} | ||
|
||
return (void*)((char*)ptr + sizeof(struct memoryEntry)); | ||
} | ||
|
||
static void* reallocFunction (void *ptr, size_t size, const char *file, int line) | ||
{ | ||
struct memoryEntry* entry; | ||
int newCount; | ||
|
||
if (ptr != NULL) | ||
{ | ||
ptr = (void*)((char*)ptr - sizeof(struct memoryEntry)); | ||
entry = (struct memoryEntry*)ptr; | ||
CRYPTO_atomic_add(&g_allocatedMemory, (int)(-entry->size), &newCount, g_allocLock); | ||
} | ||
|
||
void* newPtr = realloc(ptr, size + sizeof(struct memoryEntry)); | ||
if (newPtr != NULL) | ||
{ | ||
CRYPTO_atomic_add(&g_allocatedMemory, (int)size, &newCount, g_allocLock); | ||
CRYPTO_atomic_add(&g_allocationCount, 1, &newCount, g_allocLock); | ||
|
||
entry = (struct memoryEntry*)newPtr; | ||
entry->size = (int)size; | ||
entry->line = line; | ||
entry->file = file; | ||
|
||
if (g_memoryCallback != NULL) | ||
{ | ||
g_memoryCallback(ReallocOperation, newPtr, ptr, entry->size, file, line); | ||
} | ||
|
||
return (void*)((char*)newPtr + sizeof(struct memoryEntry)); | ||
} | ||
|
||
return NULL; | ||
} | ||
|
||
static void freeFunction(void *ptr, const char *file, int line) | ||
{ | ||
if (ptr != NULL) | ||
{ | ||
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) | ||
{ | ||
g_memoryCallback(FreeOperation, entry, NULL, entry->size, file, line); | ||
} | ||
|
||
free(entry); | ||
} | ||
} | ||
|
||
int32_t CryptoNative_GetMemoryUse(int* totalUsed, int* allocationCount) | ||
{ | ||
if (totalUsed == NULL || allocationCount == NULL) | ||
{ | ||
return 0; | ||
} | ||
*totalUsed = g_allocatedMemory; | ||
*allocationCount = g_allocationCount; | ||
|
||
return 1; | ||
} | ||
|
||
PALEXPORT int32_t CryptoNative_SetMemoryTracking(CRYPTO_allocation_cb callback) | ||
{ | ||
g_memoryCallback = callback; | ||
return 1; | ||
} | ||
|
||
static int32_t g_initStatus = 1; | ||
int g_x509_ocsp_index = -1; | ||
|
||
|
@@ -1490,7 +1593,39 @@ static int32_t EnsureOpenSslInitializedCore(void) | |
// Otherwise call the 1.1 one. | ||
#ifdef FEATURE_DISTRO_AGNOSTIC_SSL | ||
InitializeOpenSSLShim(); | ||
#endif | ||
|
||
const char* debug = getenv("DOTNET_SYSTEM_NET_SECURITY_OPENSSL_MEMORY_DEBUG"); | ||
if (debug != NULL && strcmp(debug, "1") == 0) | ||
{ | ||
// This needs to be done before any allocation is done e.g. EnsureOpenSsl* is called. | ||
// And it also needs to be after the pointers are loaded for DISTRO_AGNOSTIC_SSL | ||
#ifdef FEATURE_DISTRO_AGNOSTIC_SSL | ||
if (API_EXISTS(CRYPTO_THREAD_lock_new)) | ||
{ | ||
// This should cover 1.1.1+ | ||
|
||
CRYPTO_set_mem_functions11(mallocFunction, reallocFunction, freeFunction); | ||
g_allocLock = CRYPTO_THREAD_lock_new(); | ||
|
||
if (!API_EXISTS(SSL_state)) | ||
{ | ||
// CRYPTO_set_mem_functions exists in OpenSSL 1.0.1 as well but it has different prototype | ||
// and that makes it difficult to use with managed callbacks. | ||
// Since 1.0 is long time out of support we use it only on 1.1.1+ | ||
CRYPTO_set_mem_functions11(mallocFunction, reallocFunction, freeFunction); | ||
g_allocLock = CRYPTO_THREAD_lock_new(); | ||
} | ||
} | ||
#elif OPENSSL_VERSION_NUMBER >= OPENSSL_VERSION_1_1_0_RTM | ||
// OpenSSL 1.0 has different prototypes and it is out of support so we enable this only | ||
// on 1.1.1+ | ||
CRYPTO_set_mem_functions(mallocFunction, reallocFunction, freeFunction); | ||
g_allocLock = CRYPTO_THREAD_lock_new(); | ||
#endif | ||
} | ||
|
||
#ifdef FEATURE_DISTRO_AGNOSTIC_SSL | ||
if (API_EXISTS(SSL_state)) | ||
{ | ||
ret = EnsureOpenSsl10Initialized(); | ||
|
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.
This does not need to be cached in a static