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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Expand Up @@ -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;
Expand Down Expand Up @@ -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 UIntPtr Offset = (UIntPtr)sizeof(MemoryEntry);
jkotas marked this conversation as resolved.
Show resolved Hide resolved
private static HashSet<UIntPtr>? _allocations;
Copy link
Member

Choose a reason for hiding this comment

The 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)
{
Span<MemoryEntry> entry = new Span<MemoryEntry>((void*)ptr, 1);
jkotas marked this conversation as resolved.
Show resolved Hide resolved

Debug.Assert(entry[0].File != null);
Debug.Assert(ptr != UIntPtr.Zero);

switch (operation)
{
case MemoryOperation.Malloc:
Debug.Assert(size == entry[0].Size);
lock (_allocations!)
{
_allocations!.Add(ptr);
Copy link
Member

Choose a reason for hiding this comment

The 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

#101626 (comment)

Or does that hold only for the malloc/free calls and not GC-allocated memory?

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Member Author

Choose a reason for hiding this comment

The 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 #if DEBUG for now to limit the exposure. While this limits use in production it would allow us to experiment more and perhaps hook it to test runs. I'm yet to see case where it actually fails. Since this can be set during run for some particular operation(s) it may avoid the cases we are concern about e.g. threads operations. AFAIK there is API to get loaded providers so we may for example check FIPS or 3rd party modules.

}
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)
{
Span<MemoryEntry> entry = new Span<MemoryEntry>((void*)ptr, 1);
jkotas marked this conversation as resolved.
Show resolved Hide resolved
allocations[index] = new Tuple<UIntPtr, int, string>(ptr + Offset, entry[0].Size, $"{Marshal.PtrToStringAnsi((IntPtr)entry[0].File)}:{entry[0].Line}");
index++;
}

return allocations;
}
}
#endif
}
}
Expand Up @@ -29,6 +29,7 @@ static OpenSsl()

internal static partial class CryptoInitializer
{

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

static CryptoInitializer()
{
if (EnsureOpenSslInitialized() != 0)
Expand All @@ -41,6 +42,7 @@ static CryptoInitializer()
// these libraries will be unable to operate correctly.
throw new InvalidOperationException();
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

}

internal static void Initialize()
Expand Down
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?


#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
6 changes: 1 addition & 5 deletions src/libraries/System.Net.Http/src/System.Net.Http.csproj
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 Expand Up @@ -476,6 +471,7 @@
<Reference Include="System.Collections" />
<Reference Include="System.Collections.Concurrent" />
<Reference Include="System.Diagnostics.Tracing" />
<Reference Include="System.Diagnostics.StackTrace" />
jkotas marked this conversation as resolved.
Show resolved Hide resolved
<Reference Include="System.Memory" />
<Reference Include="System.Runtime" />
<Reference Include="System.Runtime.InteropServices" />
Expand Down
13 changes: 13 additions & 0 deletions src/native/libs/System.Security.Cryptography.Native/apibridge_30.h
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);
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
138 changes: 138 additions & 0 deletions src/native/libs/System.Security.Cryptography.Native/openssl.c
Expand Up @@ -1464,6 +1464,112 @@ int32_t CryptoNative_OpenSslAvailable(void)
#endif
}

static CRYPTO_RWLOCK* g_allocLock = NULL;
static int g_allocatedMemory;
static int g_allocationCount;

static CRYPTO_malloc_fn g_mallocFunction;
jkotas marked this conversation as resolved.
Show resolved Hide resolved
static CRYPTO_realloc_fn g_reallocFunction;
static CRYPTO_free_fn g_freefunction;
static CRYPTO_allocation_cb g_memoryCallback;

struct memoryEntry
{
int size;
int line;
const char* file;
};
Comment on lines +1487 to +1492
Copy link
Member

Choose a reason for hiding this comment

The 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;

Expand All @@ -1476,7 +1582,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