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

Prevent writes during EventSource.Disable #55862

Closed
wants to merge 5 commits into from
Closed

Prevent writes during EventSource.Disable #55862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60fa305
Null out m_provHandle when deleted
Jul 16, 2021
ddb2fb9
two-phase disable checking in EventSource
Jul 16, 2021
f7a43a8
Fix positioning of incr/decr pairs
Jul 19, 2021
0f7a8b9
Switch to interlocked.incr/decr
Jul 20, 2021
1725479
switch to a using statement with a ref struct
Jul 23, 2021
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
13 changes: 10 additions & 3 deletions ...libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipeEventProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ unsafe uint IEventProvider.EventRegister(
// Unregister an event provider.
uint IEventProvider.EventUnregister(long registrationHandle)
{
EventPipeInternal.DeleteProvider(m_provHandle);
IntPtr provHandle = m_provHandle;
m_provHandle = IntPtr.Zero;
EventPipeInternal.DeleteProvider(provHandle);
return 0;
}

Expand Down Expand Up @@ -82,8 +84,13 @@ int IEventProvider.EventActivityIdControl(Interop.Advapi32.ActivityControl Contr
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;
if (m_provHandle != IntPtr.Zero)
{
IntPtr eventHandlePtr = EventPipeInternal.DefineEvent(m_provHandle, eventID, keywords, eventVersion, level, pMetadata, metadataLength);
return eventHandlePtr;
}

return IntPtr.Zero;
}

// Get or set the per-thread activity ID.
Expand Down
36 changes: 31 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ public bool IsEnabled(EventLevel level, EventKeywords keywords, EventChannel cha
if (!IsEnabled())
return false;

if (!IsEnabledCommon(m_eventSourceEnabled, m_level, m_matchAnyKeyword, level, keywords, channel))
if (!IsEnabledCommon(IsEnabled(), m_level, m_matchAnyKeyword, level, keywords, channel))
return false;

return true;
Expand Down Expand Up @@ -1302,6 +1302,7 @@ protected unsafe void WriteEventCore(int eventId, int eventDataCount, EventSourc
[CLSCompliant(false)]
protected unsafe void WriteEventWithRelatedActivityIdCore(int eventId, Guid* relatedActivityId, int eventDataCount, EventSource.EventData* data)
{
using var writeGuard = new WriteGuard(this);
if (IsEnabled())
{
Debug.Assert(m_eventData != null); // You must have initialized this if you enabled the source.
Expand Down Expand Up @@ -1448,7 +1449,7 @@ protected virtual void Dispose(bool 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)
if (IsEnabled())
{
try
{
Expand All @@ -1457,6 +1458,12 @@ protected virtual void Dispose(bool disposing)
catch { } // If it fails, simply give up.
m_eventSourceEnabled = false;
}

// Wait till active writes have finished (stop waiting at 1 second)
// TODO: determine appropriate break out mechanism. Is 1 second appropriate? Is a timeout appropriate at all?
// TODO: use Volatile.Read instead? It's an int, so we shouldn't get tearing.
SpinWait.SpinUntil(() => Interlocked.CompareExchange(ref m_activeWritesCount, 0, 0) == 0, 1000);

if (m_etwProvider != null)
{
m_etwProvider.Dispose();
Expand Down Expand Up @@ -1625,7 +1632,7 @@ private unsafe void Initialize(Guid eventSourceGuid, string eventSourceName, str
#if FEATURE_PERFTRACING
m_eventPipeProvider = eventPipeProvider;
#endif
Debug.Assert(!m_eventSourceEnabled); // We can't be enabled until we are completely initted.
Debug.Assert(!IsEnabled()); // We can't be enabled until we are completely initted.
// We are logically completely initialized at this point.
m_completelyInited = true;
}
Expand Down Expand Up @@ -1892,6 +1899,7 @@ private static unsafe void AssertValidString(EventData* data)
#endif
private unsafe void WriteEventVarargs(int eventId, Guid* childActivityID, object?[] args)
{
using var writeGuard = new WriteGuard(this);
if (IsEnabled())
{
Debug.Assert(m_eventData != null); // You must have initialized this if you enabled the source.
Expand Down Expand Up @@ -2625,7 +2633,7 @@ internal void DoCommand(EventCommandEventArgs commandArgs)

if (commandArgs.enable)
{
if (!m_eventSourceEnabled)
if (!IsEnabled())
{
// EventSource turned on for the first time, simply copy the bits.
m_level = commandArgs.level;
Expand Down Expand Up @@ -3838,6 +3846,23 @@ private bool SelfDescribingEvents
}
}

// Helper class for guarding writes to prevent Dispose from happening while we are writing
private ref struct WriteGuard
{
private EventSource _source;

public WriteGuard(EventSource source)
{
_source = source;
Interlocked.Increment(ref _source.m_activeWritesCount);
}

public void Dispose()
{
Interlocked.Decrement(ref _source.m_activeWritesCount);
}
}

// private instance state
private string m_name = null!; // My friendly name (privided in ctor)
internal int m_id; // A small integer that is unique to this instance.
Expand All @@ -3852,9 +3877,10 @@ private bool SelfDescribingEvents
private bool m_eventSourceDisposed; // has Dispose been called.

// Enabling bits
private bool m_eventSourceEnabled; // am I enabled (any of my events are enabled for any dispatcher)
private volatile bool m_eventSourceEnabled; // am I enabled (any of my events are enabled for any dispatcher)
internal EventLevel m_level; // highest level enabled by any output dispatcher
internal EventKeywords m_matchAnyKeyword; // the logical OR of all levels enabled by any output dispatcher (zero is a special case) meaning 'all keywords'
private int m_activeWritesCount; // The number of currently active calls to Write methods that have already checked m_eventSourceEnabled

// Dispatching state
internal volatile EventDispatcher? m_Dispatchers; // Linked list of code:EventDispatchers we write the data to (we also do ETW specially)
Expand Down
218 changes: 113 additions & 105 deletions ...em.Private.CoreLib/src/System/Diagnostics/Tracing/TraceLogging/TraceLoggingEventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,10 +365,13 @@ private unsafe void WriteMultiMerge(
Guid* childActivityID,
params object?[] values)
{
using var writeGuard = new WriteGuard(this);

if (!this.IsEnabled())
{
return;
}

byte level = (options.valuesSet & EventSourceOptions.levelSet) != 0
? options.level
: eventTypes.level;
Expand Down Expand Up @@ -550,6 +553,7 @@ internal unsafe void WriteMultiMerge(
EventData* data)
{
#if FEATURE_MANAGED_ETW
using var writeGuard = new WriteGuard(this);
if (!this.IsEnabled())
{
return;
Expand Down Expand Up @@ -621,125 +625,129 @@ private unsafe void WriteImpl(
Guid* pRelatedActivityId,
TraceLoggingEventTypes eventTypes)
{
try
using var writeGuard = new WriteGuard(this);
if (IsEnabled())
{
fixed (EventSourceOptions* pOptions = &options)
try
{
options.Opcode = options.IsOpcodeSet ? options.Opcode : GetOpcodeWithDefault(options.Opcode, eventName);
NameInfo? nameInfo = this.UpdateDescriptor(eventName, eventTypes, ref options, out EventDescriptor descriptor);
if (nameInfo == null)
fixed (EventSourceOptions* pOptions = &options)
{
return;
}

#if FEATURE_PERFTRACING
IntPtr eventHandle = nameInfo.GetOrCreateEventHandle(m_eventPipeProvider, m_eventHandleTable, descriptor, eventTypes);
Debug.Assert(eventHandle != IntPtr.Zero);
#else
IntPtr eventHandle = IntPtr.Zero;
#endif

#if FEATURE_MANAGED_ETW
int pinCount = eventTypes.pinCount;
byte* scratch = stackalloc byte[eventTypes.scratchSize];
EventData* descriptors = stackalloc EventData[eventTypes.dataCount + 3];
for (int i = 0; i < eventTypes.dataCount + 3; i++)
descriptors[i] = default;

GCHandle* pins = stackalloc GCHandle[pinCount];
for (int i = 0; i < pinCount; i++)
pins[i] = default;

var providerMetadata = ProviderMetadata;
fixed (byte*
pMetadata0 = providerMetadata,
pMetadata1 = nameInfo.nameMetadata,
pMetadata2 = eventTypes.typeMetadata)
{
descriptors[0].SetMetadata(pMetadata0, providerMetadata.Length, 2);
descriptors[1].SetMetadata(pMetadata1, nameInfo.nameMetadata.Length, 1);
descriptors[2].SetMetadata(pMetadata2, eventTypes.typeMetadata.Length, 1);
#endif // FEATURE_MANAGED_ETW

#if ES_BUILD_STANDALONE
System.Runtime.CompilerServices.RuntimeHelpers.PrepareConstrainedRegions();
#endif
EventOpcode opcode = (EventOpcode)descriptor.Opcode;
options.Opcode = options.IsOpcodeSet ? options.Opcode : GetOpcodeWithDefault(options.Opcode, eventName);
NameInfo? nameInfo = this.UpdateDescriptor(eventName, eventTypes, ref options, out EventDescriptor descriptor);
if (nameInfo == null)
{
return;
}

Guid activityId = Guid.Empty;
Guid relatedActivityId = Guid.Empty;
if (pActivityId == null && pRelatedActivityId == null &&
((options.ActivityOptions & EventActivityOptions.Disable) == 0))
#if FEATURE_PERFTRACING
IntPtr eventHandle = nameInfo.GetOrCreateEventHandle(m_eventPipeProvider, m_eventHandleTable, descriptor, eventTypes);
Debug.Assert(eventHandle != IntPtr.Zero);
#else
IntPtr eventHandle = IntPtr.Zero;
#endif

#if FEATURE_MANAGED_ETW
int pinCount = eventTypes.pinCount;
byte* scratch = stackalloc byte[eventTypes.scratchSize];
EventData* descriptors = stackalloc EventData[eventTypes.dataCount + 3];
for (int i = 0; i < eventTypes.dataCount + 3; i++)
descriptors[i] = default;

GCHandle* pins = stackalloc GCHandle[pinCount];
for (int i = 0; i < pinCount; i++)
pins[i] = default;

var providerMetadata = ProviderMetadata;
fixed (byte*
pMetadata0 = providerMetadata,
pMetadata1 = nameInfo.nameMetadata,
pMetadata2 = eventTypes.typeMetadata)
{
if (opcode == EventOpcode.Start)
descriptors[0].SetMetadata(pMetadata0, providerMetadata.Length, 2);
descriptors[1].SetMetadata(pMetadata1, nameInfo.nameMetadata.Length, 1);
descriptors[2].SetMetadata(pMetadata2, eventTypes.typeMetadata.Length, 1);
#endif // FEATURE_MANAGED_ETW

#if ES_BUILD_STANDALONE
System.Runtime.CompilerServices.RuntimeHelpers.PrepareConstrainedRegions();
#endif
EventOpcode opcode = (EventOpcode)descriptor.Opcode;

Guid activityId = Guid.Empty;
Guid relatedActivityId = Guid.Empty;
if (pActivityId == null && pRelatedActivityId == null &&
((options.ActivityOptions & EventActivityOptions.Disable) == 0))
{
Debug.Assert(eventName != null, "GetOpcodeWithDefault should not returned Start when eventName is null");
m_activityTracker.OnStart(m_name, eventName, 0, ref activityId, ref relatedActivityId, options.ActivityOptions);
if (opcode == EventOpcode.Start)
{
Debug.Assert(eventName != null, "GetOpcodeWithDefault should not returned Start when eventName is null");
m_activityTracker.OnStart(m_name, eventName, 0, ref activityId, ref relatedActivityId, options.ActivityOptions);
}
else if (opcode == EventOpcode.Stop)
{
Debug.Assert(eventName != null, "GetOpcodeWithDefault should not returned Stop when eventName is null");
m_activityTracker.OnStop(m_name, eventName, 0, ref activityId);
}
if (activityId != Guid.Empty)
pActivityId = &activityId;
if (relatedActivityId != Guid.Empty)
pRelatedActivityId = &relatedActivityId;
}
else if (opcode == EventOpcode.Stop)

try
{
Debug.Assert(eventName != null, "GetOpcodeWithDefault should not returned Stop when eventName is null");
m_activityTracker.OnStop(m_name, eventName, 0, ref activityId);
#if FEATURE_MANAGED_ETW
DataCollector.ThreadInstance.Enable(
scratch,
eventTypes.scratchSize,
descriptors + 3,
eventTypes.dataCount,
pins,
pinCount);

TraceLoggingTypeInfo info = eventTypes.typeInfos[0];
info.WriteData(info.PropertyValueFactory(data));

this.WriteEventRaw(
eventName,
ref descriptor,
eventHandle,
pActivityId,
pRelatedActivityId,
(int)(DataCollector.ThreadInstance.Finish() - descriptors),
(IntPtr)descriptors);
#endif // FEATURE_MANAGED_ETW

// TODO enable filtering for listeners.
if (m_Dispatchers != null)
{
var eventData = (EventPayload?)(eventTypes.typeInfos[0].GetData(data));
WriteToAllListeners(eventName, ref descriptor, nameInfo.tags, pActivityId, pRelatedActivityId, eventData);
}
}
if (activityId != Guid.Empty)
pActivityId = &activityId;
if (relatedActivityId != Guid.Empty)
pRelatedActivityId = &relatedActivityId;
}

try
{
#if FEATURE_MANAGED_ETW
DataCollector.ThreadInstance.Enable(
scratch,
eventTypes.scratchSize,
descriptors + 3,
eventTypes.dataCount,
pins,
pinCount);

TraceLoggingTypeInfo info = eventTypes.typeInfos[0];
info.WriteData(info.PropertyValueFactory(data));

this.WriteEventRaw(
eventName,
ref descriptor,
eventHandle,
pActivityId,
pRelatedActivityId,
(int)(DataCollector.ThreadInstance.Finish() - descriptors),
(IntPtr)descriptors);
#endif // FEATURE_MANAGED_ETW

// TODO enable filtering for listeners.
if (m_Dispatchers != null)
catch (Exception ex)
{
var eventData = (EventPayload?)(eventTypes.typeInfos[0].GetData(data));
WriteToAllListeners(eventName, ref descriptor, nameInfo.tags, pActivityId, pRelatedActivityId, eventData);
if (ex is EventSourceException)
throw;
else
ThrowEventSourceException(eventName, ex);
}
#if FEATURE_MANAGED_ETW
finally
{
WriteCleanup(pins, pinCount);
}
}
catch (Exception ex)
{
if (ex is EventSourceException)
throw;
else
ThrowEventSourceException(eventName, ex);
}
#if FEATURE_MANAGED_ETW
finally
{
WriteCleanup(pins, pinCount);
}
#endif // FEATURE_MANAGED_ETW
}
#endif // FEATURE_MANAGED_ETW
}
}
catch (Exception ex)
{
if (ex is EventSourceException)
throw;
else
ThrowEventSourceException(eventName, ex);
catch (Exception ex)
{
if (ex is EventSourceException)
throw;
else
ThrowEventSourceException(eventName, ex);
}
}
}

Expand Down