Fix a potential memory leak due to EventPipe buffer allocation (dotne…

…t#35924)

* simplest fix

* make the buffer size page-aligned

* CR feedback

* Add comment about memset

src/coreclr/src/vm/eventpipebuffer.cpp

@@ -23,8 +23,21 @@ EventPipeBuffer::EventPipeBuffer(unsigned int bufferSize, EventPipeThread* pWrit
     m_state = EventPipeBufferState::WRITABLE;
     m_pWriterThread = pWriterThread;
     m_eventSequenceNumber = eventSequenceNumber;
-    m_pBuffer = new BYTE[bufferSize];
+    // Use ClrVirtualAlloc instead of malloc to allocate buffer to avoid potential internal fragmentation in the native CRT heap.
+    // (See https://github.com/dotnet/runtime/pull/35924 and https://github.com/microsoft/ApplicationInsights-dotnet/issues/1678 for more details)
+    //
+    // This fix does cause a little bit of performance regression (1-2%) in throughput,
+    // but within acceptable boundaries, while minimizing the risk of the fix to be backported
+    // to servicing releases. We may come back in the future to reassess this and potentially improve
+    // the throughput via more performant solution afterwards.
+    m_pBuffer = (BYTE*)ClrVirtualAlloc(NULL, bufferSize, MEM_COMMIT, PAGE_READWRITE);
+
+    // memset may be unnecessary here because VirtualAlloc with MEM_COMMIT zero-initializes the pages and mmap also zero-initializes
+    // if MAP_UNINITIALIZED isn't passed (which ClrVirtualAlloc doesn't). If this memset ends up being a perf cost in future investigations
+    // we may remove this. But for risk mitigation we're leaving it as-is.
+    // (See https://github.com/dotnet/runtime/pull/35924#discussion_r421282564 for discussion on this)
     memset(m_pBuffer, 0, bufferSize);
+
     m_pLimit = m_pBuffer + bufferSize;
     m_pCurrent = GetNextAlignedAddress(m_pBuffer);
 
@@ -47,7 +60,7 @@ EventPipeBuffer::~EventPipeBuffer()
     }
     CONTRACTL_END;
 
-    delete[] m_pBuffer;
+    ClrVirtualFree(m_pBuffer, 0, MEM_RELEASE);
 }
 
 bool EventPipeBuffer::WriteEvent(Thread *pThread, EventPipeSession &session, EventPipeEvent &event, EventPipeEventPayload &payload, LPCGUID pActivityId, LPCGUID pRelatedActivityId, StackContents *pStack)

src/coreclr/src/vm/eventpipebuffermanager.cpp

@@ -168,6 +168,9 @@ EventPipeBuffer* EventPipeBufferManager::AllocateBufferForThread(EventPipeThread
         const unsigned int maxBufferSize = 1024 * 1024;
         bufferSize = Min(bufferSize, maxBufferSize);
 
+        // Make the buffer size fit into with pagesize-aligned block, since ClrVirtualAlloc expects page-aligned sizes to be passed as arguments (see ctor of EventPipeBuffer)
+        bufferSize = (bufferSize + g_SystemInfo.dwAllocationGranularity - 1) & ~static_cast<unsigned int>(g_SystemInfo.dwAllocationGranularity - 1);
+
         // EX_TRY is used here as opposed to new (nothrow) because
         // the constructor also allocates a private buffer, which
         // could throw, and cannot be easily checked