Skip to content
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

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Concurrent;
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Security.Cryptography.X509Certificates;
Expand Down Expand Up @@ -170,5 +171,106 @@ 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);
wfurt marked this conversation as resolved.
Show resolved Hide resolved
// We only need to store the keys but we use ConcurrentDictionary to avoid locking
private static ConcurrentDictionary<UIntPtr, UIntPtr>? _allocations;

[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);
_allocations!.TryAdd(ptr, ptr);
break;
case MemoryOperation.Realloc:
if ((IntPtr)oldPtr != IntPtr.Zero)
{
_allocations!.TryRemove(oldPtr, out _);
}
_allocations!.TryAdd(ptr, ptr);
break;
case MemoryOperation.Free:
_allocations!.TryRemove(ptr, out _);
break;
}
}

public static unsafe void EnableTracking()
{
_allocations ??= new ConcurrentDictionary<UIntPtr, UIntPtr>();
_allocations!.Clear();
SetMemoryTracking(&MemoryTrackinCallback);
}

public static unsafe void DisableTracking()
{
SetMemoryTracking(null);
_allocations!.Clear();
}

public static unsafe Tuple<UIntPtr, int, string>[] GetIncrementalAllocations()
Copy link
Member

Choose a reason for hiding this comment

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

The original PR had IntPtr, is there some reason to use UIntPtr over IntPtr here?

{
if (_allocations == null || _allocations.IsEmpty)
{
return Array.Empty<Tuple<UIntPtr, int, string>>();
}

Tuple<UIntPtr, int, string>[] allocations = new Tuple<UIntPtr, int, string>[_allocations.Count];
Copy link
Contributor

Choose a reason for hiding this comment

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

I was browsing PRs and accidentally stumbled upon this one.

Is there a specific reason this code uses obsolete Tuple over ValueTple aka (UIntPtr, int, string)? Thanks!

int index = 0;
foreach ((UIntPtr ptr, UIntPtr value) 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++;
}
Comment on lines +263 to +270
Copy link
Member

Choose a reason for hiding this comment

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

Can the _allocations.Count value change while iterating over _allocations?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. I expect the ConcurrentDictionary to handle parallel access gracefully but you may not get consistent snapshot. We did it while back with the HashSet and manual locking. Since the iteration can take a while I feel it is better to allow concurrent access and deliver what we can. In ideal situation one would enable tracking run the repro and dump whatever it remains.

Copy link
Member

Choose a reason for hiding this comment

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

My point is that the new size may not necessarily match the size of the allocations array, and allocations[index] access may throw.

Copy link
Member

Choose a reason for hiding this comment

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

Also, ConcurrentDictionary.Count is extremely slow.


return allocations;
}
#endif
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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 GetMemoryDebug to cctor?

Copy link
Member

Choose a reason for hiding this comment

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

This should also be moved to Interop.Crypto, because following code

var ci = typeof(SslStream).Assembly.GetTypes().First(t => t.Name == "Crypto");
ci.InvokeMember("EnableTracking", BindingFlags.InvokeMethod | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static, null, null, null);

Will enable tracking, but later when Interop.OpenSsl gets initialized, it turns the tracking of because of

                Interop.Crypto.EnableTracking();
                Interop.Crypto.GetIncrementalAllocations();
                Interop.Crypto.DisableTracking();

in GetMemoryDebug()

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the comment. Based on the feedback I specifically removed the option to enable the detailed tracking via environment to avoid cases when managed code may be invoked on incompatible thread. What remains is ability to subscribe for the detailed reporting later (when all the initialization is done) when caller feels it is safe. I know this part may be tricky to describe. But so far I failed to construct case where it failed e.g. all the cases I was interested in so far just worked and provided the info I was looking for.

Copy link
Member

Choose a reason for hiding this comment

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

Following program will not print any memory addresses, although one would expect that it should

// var ossl = typeof(SslStream).Assembly.GetTypes().First(t => t.Name == "OpenSsl");
// ossl.InvokeMember("GetMemoryDebug", BindingFlags.InvokeMethod | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static, null, null, null);
var ci = typeof(SslStream).Assembly.GetTypes().First(t => t.Name == "Crypto");
ci.InvokeMember("EnableTracking", BindingFlags.InvokeMethod | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static, null, null, null);

HttpClient client = new HttpClient();
await client.GetAsync("https://www.microsoft.com");

using Process process = Process.GetCurrentProcess();

Console.WriteLine($"Bytes known to GC [{GC.GetTotalMemory(false)}], process working set [{process.WorkingSet64}]");
Console.WriteLine("OpenSSL memory {0}", ci.InvokeMember("GetOpenSslAllocatedMemory", BindingFlags.InvokeMethod | BindingFlags.Public | BindingFlags.Static, null, null, null));

Tuple<UIntPtr, int, string>[] allocations = (Tuple<UIntPtr, int, string>[])ci.InvokeMember("GetIncrementalAllocations", BindingFlags.InvokeMethod | BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Static | BindingFlags.Instance, null, null, null);
for (int j = 0; j < allocations.Length; j++)
{
    (UIntPtr ptr, int size, string src) = allocations[j];
    Console.WriteLine("Allocated {0} bytes at 0x{1:x} from {2}", size, ptr, src);
}

It starts to work if you uncomment the first two lines.


#region internal methods
internal static SafeChannelBindingHandle? QueryChannelBinding(SafeSslHandle context, ChannelBindingKind bindingType)
Expand Down Expand Up @@ -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)
{
Expand Down
5 changes: 0 additions & 5 deletions src/libraries/System.Net.Http/src/System.Net.Http.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -432,11 +432,6 @@
Link="Common\System\Net\Security\CertificateHelper.Unix.cs" />
</ItemGroup>

<ItemGroup Condition="'$(TargetPlatformIdentifier)' != '' and '$(TargetPlatformIdentifier)' != 'windows' and '$(TargetPlatformIdentifier)' != 'browser' and '$(TargetPlatformIdentifier)' != 'osx' and '$(TargetPlatformIdentifier)' != 'ios' and '$(TargetPlatformIdentifier)' != 'tvos'">
<Compile Include="$(CommonPath)Interop\Unix\System.Security.Cryptography.Native\Interop.Initialization.cs"
Link="Common\Interop\Unix\System.Security.Cryptography.Native\Interop.Initialization.cs" />
</ItemGroup>

<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'browser'">
<Compile Include="$(CommonPath)\System\Net\HttpStatusDescription.cs"
Link="Common\System\Net\HttpStatusDescription.cs" />
Expand Down
13 changes: 13 additions & 0 deletions src/native/libs/System.Security.Cryptography.Native/apibridge_30.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@
#pragma once
#include "pal_types.h"

typedef void *(*CRYPTO_malloc_fn)(size_t num, const char *file, int line);
typedef void *(*CRYPTO_realloc_fn)(void *addr, size_t num, const char *file, int line);
typedef void (*CRYPTO_free_fn)(void *addr, const char *file, int line);

#ifndef CRYPTO_RWLOCK
typedef void CRYPTO_RWLOCK;
#endif

CRYPTO_RWLOCK *CRYPTO_THREAD_lock_new(void);
int CRYPTO_atomic_add(int *val, int amount, int *ret, CRYPTO_RWLOCK *lock);

typedef struct evp_mac_st EVP_MAC;
typedef struct evp_mac_ctx_st EVP_MAC_CTX;

Expand All @@ -14,3 +25,5 @@ int local_EVP_PKEY_CTX_set_rsa_oaep_md(EVP_PKEY_CTX* ctx, const EVP_MD* md);
int local_EVP_PKEY_CTX_set_rsa_padding(EVP_PKEY_CTX* ctx, int pad_mode);
int local_EVP_PKEY_CTX_set_rsa_pss_saltlen(EVP_PKEY_CTX* ctx, int saltlen);
int local_EVP_PKEY_CTX_set_signature_md(EVP_PKEY_CTX* ctx, const EVP_MD* md);

int CRYPTO_set_mem_functions11(CRYPTO_malloc_fn malloc_fn, CRYPTO_realloc_fn realloc_fn, CRYPTO_free_fn free_fn);
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ static const Entry s_cryptoNative[] =
DllImportEntry(CryptoNative_GetECKeyParameters)
DllImportEntry(CryptoNative_GetMaxMdSize)
DllImportEntry(CryptoNative_GetMemoryBioSize)
DllImportEntry(CryptoNative_GetMemoryUse)
DllImportEntry(CryptoNative_SetMemoryTracking)
DllImportEntry(CryptoNative_GetObjectDefinitionByName)
DllImportEntry(CryptoNative_GetOcspRequestDerSize)
DllImportEntry(CryptoNative_GetPkcs7Certificates)
Expand Down
144 changes: 144 additions & 0 deletions src/native/libs/System.Security.Cryptography.Native/openssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1478,6 +1478,118 @@ 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;
} __attribute__((aligned(8)));

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)
{
#if defined(__GNUC__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Werror=use-after-free"
wfurt marked this conversation as resolved.
Show resolved Hide resolved
#endif
// Now try just the _majorVer added
g_memoryCallback(ReallocOperation, newPtr, ptr, entry->size, file, line);

#if defined(__GNUC__)
#pragma GCC diagnostic pop
#endif
}

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;

Expand All @@ -1490,7 +1602,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();
Expand Down
13 changes: 13 additions & 0 deletions src/native/libs/System.Security.Cryptography.Native/openssl.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,16 @@ PALEXPORT int64_t CryptoNative_OpenSslVersionNumber(void);
PALEXPORT void CryptoNative_RegisterLegacyAlgorithms(void);

PALEXPORT int32_t CryptoNative_OpenSslAvailable(void);

PALEXPORT int32_t CryptoNative_GetMemoryUse(int* totalUsed, int* allocationCount);

typedef enum
{
MallocOperation = 1,
ReallocOperation = 2,
FreeOperation = 3,
} MemoryOperation;

typedef void (*CRYPTO_allocation_cb)(MemoryOperation operation, void* ptr, void* oldPtr, int size, const char *file, int line);

PALEXPORT int32_t CryptoNative_SetMemoryTracking(CRYPTO_allocation_cb callback);
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,9 @@ extern bool g_libSslUses32BitTime;
REQUIRED_FUNCTION(CRYPTO_malloc) \
LEGACY_FUNCTION(CRYPTO_num_locks) \
LEGACY_FUNCTION(CRYPTO_set_locking_callback) \
LIGHTUP_FUNCTION(CRYPTO_THREAD_lock_new) \
LIGHTUP_FUNCTION(CRYPTO_atomic_add) \
RENAMED_FUNCTION(CRYPTO_set_mem_functions11, CRYPTO_set_mem_functions) \
REQUIRED_FUNCTION(d2i_ASN1_BIT_STRING) \
REQUIRED_FUNCTION(d2i_BASIC_CONSTRAINTS) \
REQUIRED_FUNCTION(d2i_EXTENDED_KEY_USAGE) \
Expand Down Expand Up @@ -781,6 +784,9 @@ extern TYPEOF(OPENSSL_gmtime)* OPENSSL_gmtime_ptr;
#define CRYPTO_malloc CRYPTO_malloc_ptr
#define CRYPTO_num_locks CRYPTO_num_locks_ptr
#define CRYPTO_set_locking_callback CRYPTO_set_locking_callback_ptr
#define CRYPTO_THREAD_lock_new CRYPTO_THREAD_lock_new_ptr
#define CRYPTO_atomic_add CRYPTO_atomic_add_ptr
#define CRYPTO_set_mem_functions11 CRYPTO_set_mem_functions11_ptr
#define d2i_ASN1_BIT_STRING d2i_ASN1_BIT_STRING_ptr
#define d2i_BASIC_CONSTRAINTS d2i_BASIC_CONSTRAINTS_ptr
#define d2i_EXTENDED_KEY_USAGE d2i_EXTENDED_KEY_USAGE_ptr
Expand Down
Loading
Loading