Rare race condition in EventSource dispose/finalizer

[josalem]

There is a rare race that can result in a use-after-free on the native end of EventPipeEventProvider and potentially the EtwEventProvider.

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

Lines 1420 to 1484 in c90aa4d

	#region IDisposable Members
	/// <summary>
	/// Disposes of an EventSource.
	/// </summary>
	public void Dispose()
	{
	this.Dispose(true);
	GC.SuppressFinalize(this);
	}
	/// <summary>
	/// Disposes of an EventSource.
	/// </summary>
	/// <remarks>
	/// Called from Dispose() with disposing=true, and from the finalizer (~EventSource) with disposing=false.
	/// Guidelines:
	/// 1. We may be called more than once: do nothing after the first call.
	/// 2. Avoid throwing exceptions if disposing is false, i.e. if we're being finalized.
	/// </remarks>
	/// <param name="disposing">True if called from Dispose(), false if called from the finalizer.</param>
	protected virtual void Dispose(bool disposing)
	{
	if (!IsSupported)
	{
	return;
	}
	if (disposing)
	{
	#if FEATURE_MANAGED_ETW
	// Send the manifest one more time to ensure circular buffers have a chance to get to this information
	// even in scenarios with a high volume of ETW events.
	if (m_eventSourceEnabled)
	{
	try
	{
	SendManifest(m_rawManifest);
	}
	catch { } // If it fails, simply give up.
	m_eventSourceEnabled = false;
	}
	if (m_etwProvider != null)
	{
	m_etwProvider.Dispose();
	m_etwProvider = null!;
	}
	#endif
	#if FEATURE_PERFTRACING
	if (m_eventPipeProvider != null)
	{
	m_eventPipeProvider.Dispose();
	m_eventPipeProvider = null!;
	}
	#endif
	}
	m_eventSourceEnabled = false;
	m_eventSourceDisposed = true;
	}
	/// <summary>
	/// Finalizer for EventSource
	/// </summary>
	~EventSource()
	{
	this.Dispose(false);
	}
	#endregion

If one thread (A) is calling EventSource.Dispose(), and another is in the process of writing (B), it is possible for the following sequence to occur:

A: (1)Dispose -> (3)m_eventSourceEnabled = false -> (4)m_eventPipeProvider.Dispose() -> (6)m_eventPipeProvider = null
B: (2)if (IsEnabled) -> (5)use m_eventPipeProvider

EventPipeEventProvider.Dispose() calls EventPipeEventProvider.EventUnregister(). This deletes the underlying native structures (the only time EventPipeProvider::m_pEventList is set to nullptr). The managed code, does not unset the m_provHandle member, so if someone got a reference to this managed object, they would have a pointer to freed memory. The managed provider has been marked as disabled, however, not all code paths check that value. Specifically:

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs

Lines 81 to 87 in c90aa4d

	// Define an EventPipeEvent handle.
	unsafe IntPtr IEventProvider.DefineEventHandle(uint eventID, string eventName, long keywords, uint eventVersion, uint level,
	byte *pMetadata, uint metadataLength)
	{
	IntPtr eventHandlePtr = EventPipeInternal.DefineEvent(m_provHandle, eventID, keywords, eventVersion, level, pMetadata, metadataLength);
	return eventHandlePtr;
	}

which is where we AV in this case. We get here from TraceLoggingEventSource.WriteImpl() which ends up in NameInfo.GetOrCreateEventHandle() which calls DefineEvent on the provider:

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/TraceLogging/NameInfo.cs

Lines 81 to 123 in 9da4d07

	public IntPtr GetOrCreateEventHandle(EventProvider provider, TraceLoggingEventHandleTable eventHandleTable, EventDescriptor descriptor, TraceLoggingEventTypes eventTypes)
	{
	IntPtr eventHandle;
	if ((eventHandle = eventHandleTable[descriptor.EventId]) == IntPtr.Zero)
	{
	lock (eventHandleTable)
	{
	if ((eventHandle = eventHandleTable[descriptor.EventId]) == IntPtr.Zero)
	{
	byte[]? metadata = EventPipeMetadataGenerator.Instance.GenerateEventMetadata(
	descriptor.EventId,
	name,
	(EventKeywords)descriptor.Keywords,
	(EventLevel)descriptor.Level,
	descriptor.Version,
	(EventOpcode)descriptor.Opcode,
	eventTypes);
	uint metadataLength = (metadata != null) ? (uint)metadata.Length : 0;
	unsafe
	{
	fixed (byte* pMetadataBlob = metadata)
	{
	// Define the event.
	eventHandle = provider.m_eventProvider.DefineEventHandle(
	(uint)descriptor.EventId,
	name,
	descriptor.Keywords,
	descriptor.Version,
	descriptor.Level,
	pMetadataBlob,
	metadataLength);
	}
	}
	// Cache the event handle.
	eventHandleTable.SetEventHandle(descriptor.EventId, eventHandle);
	}
	}
	}
	return eventHandle;
	}

I'm still pinning down the exact sequencing of events in the hopes I can create a deterministic repro.

I believe this is the cause for the failures on dotnet/coreclr#28179 and #55240.

CC @tommcdon @noahfalk @dotnet/dotnet-diag

[josalem]

Migrating the conversation from #55862 to here:

@noahfalk:

@josalem and I chatted earlier, but just to record it - most BCL types don't provide a thread-safety guarantee for Dispose() and I wonder if it is necessary here. The synchronization necessary to provide that guarantee comes with perf costs I'm hopeful we can avoid.

• If the runtime will automatically call Dispose() on a thread of our choosing at shutdown then we do need some thread-safety guarantee, but does the runtime need to be calling Dispose() at all? I'm not sure what benefit it gives us but research may reveal that is necessary.
• Since the runtime automatically calls Write for EventCounters we would need to modify Dispose() to ensure that we stopped the EventCounter Write() calls prior to freeing the provider.

@brianrob:

I agree. I was trying to figure out what I was concerned about with regard to this change and I couldn't quite verbalize it. I think you hit it on the head @noahfalk.

@noahfalk:

I was looking at a related bug and wound up doing some research on why we call EventSource.Dispose() at shutdown. So far I see two reasons:

Avoiding native->managed callbacks after we've made it illegal to call back into managed code here
Logging ETW manifests at the end of the trace here
With a little refactoring we could create a new internal API like EventSource.HandleShutdown() that handles only these concerns while still leaving us in a state where concurrent calls to Write don't generate failures (whether any data is logged is optional). Then we would call this new API and not Dispose() in the shutdown path. Dispose() would continue to do what it does now including freeing memory, but unlike HandleShutdown() the onus could be on the caller to ensure they don't make any concurrent calls to other EventSource APIs during/after the call to Dispose().

In the case of EventPipe, HandleShutdown() could disable the callbacks without deleting the provider. In the case of ETW past precedent suggests concurrent calls to EventUnregister and EventWrite are already safe (we've presumably been doing it for many years with no reported issues).

@AaronRobinsonMSFT @vitek-karas @elinor-fung - The comment in the source references throwing a COMPLUS_BOOT_EXCEPTION and I find no reference to it any longer. I do recall that we used to block native->managed calls at some point during AppDomain shutdown, but do you know if that constraint is a relic of the desktop runtime that is no longer an issue on CoreCLR?

[jkotas]

The comment in the source references throwing a COMPLUS_BOOT_EXCEPTION and I find no reference to it any longer. I do recall that we used to block native->managed calls at some point during AppDomain shutdown, but do you know if that constraint is a relic of the desktop runtime that is no longer an issue on CoreCLR?

Correct, COMPLUS_BOOT_EXCEPTION does not exist in CoreCLR. We do not try to prevent threads from running managed code as the process is shutting down.

[josalem]

Having investigated this a little bit I think there are a couple things we could do to solve it:

• (1) In EventSource.Dispose, unregister and disable any associated counters before cleaning up resources.
  • This is problematic since it means we need to make EventSource.Dispose wait until the counters are actually disabled. They get copied while under the counter lock, so we'd need to wait till those copies are done writing to make sure the race doesn't happen.
  • This is really only needed if a user manually disposes an EventSource before the end of the process, i.e., doesn't use the public static MySource Log = new MySource(...); pattern.
• (2) In AppContext.OnProcessEnd, disable all counters and stop the PollForValues thread before the logic that disposes all EventSources.
  • This will prevent the problem from occurring, but only if the public static MySource Log = new MySource(...); pattern is followed

Neither of the above solutions actually prevents a user from writing their own code that attempts to write events while an event source is being disabled.

It's worth noting that this has only been observed on arm32. Most likely due to instruction ordering constraints being looser than other architectures.

As a result, I'm leaning towards option 2.