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.

Sign up for GitHub

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

Fix a potential memory leak due to EventPipe buffer allocation #35924

Merged
merged 4 commits into from
May 7, 2020
Merged

Fix a potential memory leak due to EventPipe buffer allocation #35924

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e183302
simplest fix
sywhang May 4, 2020
d3d64a8
make the buffer size page-aligned
sywhang May 7, 2020
7fe4042
CR feedback
sywhang May 7, 2020
53c2ad3
Add comment about memset
sywhang May 7, 2020
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
4 changes: 2 additions & 2 deletions src/coreclr/src/vm/eventpipebuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ EventPipeBuffer::EventPipeBuffer(unsigned int bufferSize, EventPipeThread* pWrit
m_state = EventPipeBufferState::WRITABLE;
m_pWriterThread = pWriterThread;
m_eventSequenceNumber = eventSequenceNumber;
m_pBuffer = new BYTE[bufferSize];
m_pBuffer = (BYTE*)ClrVirtualAlloc(NULL, bufferSize, MEM_COMMIT, PAGE_READWRITE);
sywhang marked this conversation as resolved.
Show resolved Hide resolved
memset(m_pBuffer, 0, bufferSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can skip the memset now since you used the MEM_COMMIT flag here:

The function also guarantees that when the caller later initially accesses the memory, the contents will be zero.
-docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that doc for the Windows VirtualAlloc API? I don't think the PAL implementation memsets it to 0 by default.

Copy link
Contributor

@josalem josalem May 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

runtime/src/coreclr/src/pal/src/map/virtual.cpp

Lines 1358 to 1362 in 84ec25f

/* Test for un-supported flags. */
if ( ( flAllocationType & ~( MEM_COMMIT | MEM_RESERVE | MEM_RESET | MEM_TOP_DOWN | MEM_RESERVE_EXECUTABLE | MEM_LARGE_PAGES ) ) != 0 )
{
ASSERT( "flAllocationType can be one, or any combination of MEM_COMMIT, \
MEM_RESERVE, MEM_TOP_DOWN, MEM_RESERVE_EXECUTABLE, or MEM_LARGE_PAGES.\n" );

These comments seem to indicate it is supported, but I'll double check that we actually mimic that behavior

Copy link
Member

Choose a reason for hiding this comment

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

Based on mmap docs and our code not passing MAP_UNINITIALIZED flag to mmap I would assume the memory we get back from VirtualAlloc is zero initialized. However I'd recommend only adding a comment to the memset referencing this discussion rather than removing it. We'd like to take this into servicing which has low tolerance for risk and we've already benchmarked the current code as being good enough as-is. In the future we might do some performance measurements to improve the throughput of EventPipe - if the memset shows up in that analysis we can address it then, if not we've saved time worrying about it.

m_pLimit = m_pBuffer + bufferSize;
m_pCurrent = GetNextAlignedAddress(m_pBuffer);
Expand All @@ -47,7 +47,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)
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/src/vm/eventpipebuffermanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
sywhang marked this conversation as resolved.
Show resolved Hide resolved
bufferSize = bufferSize + (g_SystemInfo.dwAllocationGranularity - (bufferSize % g_SystemInfo.dwAllocationGranularity));
sywhang marked this conversation as resolved.
Show resolved Hide resolved

// 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
Expand Down