From 47c9a0961f3802d55aa8eb18f79017e28110f597 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Thu, 7 May 2020 09:02:02 -0700 Subject: [PATCH] Fix a potential memory leak due to EventPipe buffer allocation (#35924) * simplest fix * make the buffer size page-aligned * CR feedback * Add comment about memset --- src/coreclr/src/vm/eventpipebuffer.cpp | 17 +++++++++++++++-- src/coreclr/src/vm/eventpipebuffermanager.cpp | 3 +++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/vm/eventpipebuffer.cpp b/src/coreclr/src/vm/eventpipebuffer.cpp index eeed5246cd0e8..b1564bb14af38 100644 --- a/src/coreclr/src/vm/eventpipebuffer.cpp +++ b/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) diff --git a/src/coreclr/src/vm/eventpipebuffermanager.cpp b/src/coreclr/src/vm/eventpipebuffermanager.cpp index 4990c1582dd21..f0f5f45394394 100644 --- a/src/coreclr/src/vm/eventpipebuffermanager.cpp +++ b/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(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