Skip to content

Fix race condition in ProviderNameToGuid causing ERROR_INSUFFICIENT_BUFFER crashes #2357

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

Merged
brianrob merged 9 commits into from
Jan 23, 2026
Merged

Fix race condition in ProviderNameToGuid causing ERROR_INSUFFICIENT_BUFFER crashes #2357

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
02659ce
Initial plan
Copilot Jan 23, 2026
76093ea
Fix ERROR_INSUFFICIENT_BUFFER in ProviderNameToGuid by adding retry loop
Copilot Jan 23, 2026
88bfa5d
Address code review: improve error handling and messages
Copilot Jan 23, 2026
01f45a6
Fix comment clarity in retry loop
Copilot Jan 23, 2026
0e6fadb
Optimize retry loop to use single API call per iteration
Copilot Jan 23, 2026
226f120
Always assign providersByName to avoid NullReferenceException
Copilot Jan 23, 2026
394deab
Add test for ProviderNameToGuid to verify it doesn't throw
Copilot Jan 23, 2026
2953988
Clarify test comment to reference GetProviderGuidByName
Copilot Jan 23, 2026
21dc21e
Switch from stackalloc to a heap allocation.
brianrob Jan 23, 2026
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
43 changes: 31 additions & 12 deletions src/TraceEvent/TraceEventSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1760,27 +1760,46 @@ internal static SortedDictionary<string, Guid> ProviderNameToGuid
{
SortedDictionary<string, Guid> providersByName = new SortedDictionary<string, Guid>(StringComparer.OrdinalIgnoreCase);
int buffSize = 0;
var hr = TraceEventNativeMethods.TdhEnumerateProviders(null, ref buffSize);
Debug.Assert(hr == 122); // ERROR_INSUFFICIENT_BUFFER
var buffer = stackalloc byte[buffSize];
var providersDesc = (TraceEventNativeMethods.PROVIDER_ENUMERATION_INFO*)buffer;
byte* buffer = null;
TraceEventNativeMethods.PROVIDER_ENUMERATION_INFO* providersDesc = null;
int hr;

hr = TraceEventNativeMethods.TdhEnumerateProviders(providersDesc, ref buffSize);
if ((hr == 0) && (providersDesc != null))
// Retry loop to handle the case where the buffer size changes between calls
// This can happen if providers are registered/unregistered between the two calls
for (; ; )
{
if (buffSize > 0)
{
var space = stackalloc byte[buffSize];

@brianrob brianrob Jan 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should not need another local variable. This should just be directly assigned to buffer.

Copilot AI Jan 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kept the intermediate space variable because C# requires it for assigning stackalloc to a pointer (direct assignment causes error CS8346). This matches the pattern in GetEnabledProvidersForSession (lines 2571-2572). Is there a way to avoid the intermediate variable that I'm missing?

@leculver leculver Jan 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be using stackalloc at all inside of a loop. How big can buffSize get? Using stackalloc at all is likely over-optimization here. We only ever run this code once (and ensure that with a lock) surely just allocating on the heap shouldn't cause too much trouble here, and will ensure easier to follow code, just use Array.Resize or similar.

buffer = space;
providersDesc = (TraceEventNativeMethods.PROVIDER_ENUMERATION_INFO*)buffer;
}

hr = TraceEventNativeMethods.TdhEnumerateProviders(providersDesc, ref buffSize);
if (hr == 0)
{
break;
}

// Error 122 means buffer not big enough. For that error we retry, everything else simply fail.
if (hr != 122)
{
throw new Exception("Failed to enumerate trace providers. TdhEnumerateProviders failed HR = " + hr);
}
}

if (providersDesc != null)

@brianrob brianrob Jan 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If after all this, hr == 0 but providersDesc is null, then we should just assign providersByName (now an empty dictionary) to s_providersByName so that any subsequent lookups fail, but they don't throw a NullReferenceException.

Copilot AI Jan 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Now always assigns providersByName to s_providersByName (even if empty when providersDesc is null) to avoid NullReferenceException on subsequent lookups. (commit 226f120)

{
var providers = (TraceEventNativeMethods.TRACE_PROVIDER_INFO*)&providersDesc[1];
for (int i = 0; i < providersDesc->NumberOfProviders; i++)
{
var name = new string((char*)&buffer[providers[i].ProviderNameOffset]);
providersByName[name] = providers[i].ProviderGuid;
}

s_providersByName = providersByName;
}
else
{
throw new Exception("TdhEnumerateProviders failed HR = " + hr);
}

// Always assign providersByName to avoid NullReferenceException on subsequent lookups
s_providersByName = providersByName;
}
}
}
Expand Down