Skip to content

[EventPipe] Add EventPipeBuffer corruption diagnostics #118874

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+404 −33
Open

[EventPipe] Add EventPipeBuffer corruption diagnostics #118874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fb9b360
[ClrConfig] Add EventPipeBufferGuardLevel Config Switch
mdh1418 Aug 14, 2025
6df06bc
[EventPipeBuffer] Add Buffer Guard macros
mdh1418 Aug 14, 2025
a16ece5
[EventPipeBuffer] Inject buffer guards
mdh1418 Aug 14, 2025
6a74500
[EventPipeBuffer] Ensure buffer guard consistent or FailFast
mdh1418 Aug 14, 2025
598421c
[EventPipe] Separate MetadataID from EventInstance
mdh1418 Aug 14, 2025
af7729e
[EventPipeBuffer] Add buffer ReadOnly protection
mdh1418 Aug 14, 2025
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
1 change: 1 addition & 0 deletions src/coreclr/inc/clrconfigvalues.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeCircularMB, W("EventPipeCircularMB"),
RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeProcNumbers, W("EventPipeProcNumbers"), 0, "Enable/disable capturing processor numbers in EventPipe event headers")
RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeOutputStreaming, W("EventPipeOutputStreaming"), 1, "Enable/disable streaming for trace file set in DOTNET_EventPipeOutputPath. Non-zero values enable streaming.")
RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeEnableStackwalk, W("EventPipeEnableStackwalk"), 1, "Set to 0 to disable collecting stacks for EventPipe events.")
RETAIL_CONFIG_DWORD_INFO(INTERNAL_EventPipeBufferGuardLevel, W("EventPipeBufferGuardLevel"), 0, "The level of EventPipe buffer header/footer guarding and validation")

//
// UserEvents
Expand Down
44 changes: 44 additions & 0 deletions src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,23 @@ ep_rt_config_value_get_enable_stackwalk (void)
return false;
}

static
inline
uint32_t
ep_rt_config_value_get_buffer_guard_level (void)
{
STATIC_CONTRACT_NOTHROW;

uint64_t value;
if (RhConfig::Environment::TryGetIntegerValue("EventPipeBufferGuardLevel", &value))
{
EP_ASSERT(value <= UINT32_MAX);
return static_cast<uint32_t>(value);
}

return 0;
}

/*
* EventPipeSampleProfiler.
*/
Expand Down Expand Up @@ -1803,5 +1820,32 @@ ep_rt_volatile_store_ptr_without_barrier (
ep_rt_aot_volatile_store_ptr_without_barrier(ptr, value);
}

/*
* Memory Protection
*/

static
inline
bool
ep_rt_vprotect (
void *addr,
size_t length,
EventPipePageProtection protection)
{
return true;
Copy link
Member

@noahfalk noahfalk Oct 14, 2025

Choose a reason for hiding this comment

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

@jkotas - is there some guiding principle on what we add to minipal and what should stay out?

Today EventPipe code relies on runtime-specific callouts for various OS functionality but I'm hoping we can shift that trajectory to depend more on minipal directly. This spot seems like an appealing example where we'd like to invoke an OS API but the existing pattern takes us through runtime-specific wrappers first. Adding VirtualProtect to minipal and using it feels like a good approach to me but I want to make sure I'm not abusing the intent of minipal.

Copy link
Member

@jkotas jkotas Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good candidates for minipal are unambiguous trivial methods used from number of different places. "get current time stamp" is a perfect example.

minipal is not meant to wrap everything with platform specific implementation.

I am not sure whether mprotect is a good candidate for minimap. Most places that call mprotect tend to come with their own unique requirements.

This spot seems like an appealing example where we'd like to invoke an OS API but the existing pattern takes us through runtime-specific wrappers first.

This was setup years ago to work around build system limitations. It does not make sense. It would make a lot more sense for eventpipe to have OS-specific PALs and avoid trying to fit into runtime-specific wrappers.

Copy link
Member

@noahfalk noahfalk Oct 15, 2025

Choose a reason for hiding this comment

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

Thanks Jan! We'll create our own EventPipe PAL as it sounds like these aren't appropriate for minipal.

Copy link
Member

@lateralusX lateralusX Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The EventPipe runtime shim was originally put into place to make majority of EventPipe code runtime agnostic, but still reuse runtime unique implementation of low-level artifacts like IO, lock, threading, atomics, mainly keeping the EventPipe<->runtime interaction stable for CoreCLR while porting code from C++ to C and integrated with Mono, originally it even reused each runtime container implementation, but since then, I broken out that part into native/containers and if we don't think there is much value continue using runtime specific implementations of these low level artifacts shimmed by EventPipe runtime layer, then we should probably move towards one EventPipe OS PAL source file shared by Mono/CoreCLR/NAOT.

There will still be some runtime specific things that will stay in the runtime shim layer, but it will be smaller, and it will be simpler to run EventPipe standalone, making it simpler to get our low-level native runtime tests running outside of runtime. I believe some of these things would potentially end up in minipal, we already have some artifacts that could be used by EventPipe in minipal like, mutex, hig-res timers, utf8-ucs2 conversions and I had some volatile/atomics functions in another PR that would suite minipal and EventPipe as well.

I had an ambition for a long time to get our low-level native EventPipe and container tests currently under Mono, https://github.com/dotnet/runtime/tree/main/src/mono/mono/eventpipe/test, running as a runtime test (as a shared native library or separate binary executed from test) running on all runtimes, part of that was to make EventPipe less dependent on runtime specific artifacts, moving towards one EventPipe OS PAL shared by all runtimes. If we believe that is a good direction, then doing that work could start moving us towards an EventPipe OS PAL.

}

/*
* Fail fast
*/

static
inline
void
ep_rt_fatal_error_with_message (const ep_char8_t *message)
{
/* Not implemented, no-op */
Copy link
Member

@noahfalk noahfalk Oct 14, 2025

Choose a reason for hiding this comment

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

We should make sure NativeAOT has an implementation of this too. Ideally similar to above we could have a shared minipal_raise_fatal_error() function that no longer needs runtime-specific callouts. Doing that depends on the fatal error handling staying simple, runtime agnostic, and directly aligning with underlying OS APIs.

Copy link
Member

@jkotas jkotas Oct 15, 2025

Choose a reason for hiding this comment

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

fatal error handling staying simple, runtime agnostic, and directly aligning with underlying OS APIs.

Fatal error handling includes runtime-specific crash dump and watson logic currently...

}

#endif /* ENABLE_PERFTRACING */
#endif /* EVENTPIPE_RT_AOT_H */
54 changes: 54 additions & 0 deletions src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@
#include <eventpipe/ep-string.h>
#include "fstream.h"
#include "typestring.h"
#include "clrhost.h"
#include "clrversion.h"
#include "hostinformation.h"
#include <minipal/guid.h>
#include <minipal/strings.h>
#include <minipal/time.h>
#include <stdio.h>

#undef EP_INFINITE_WAIT
#define EP_INFINITE_WAIT INFINITE
Expand Down Expand Up @@ -560,6 +562,15 @@ ep_rt_config_value_get_enable_stackwalk (void)
return CLRConfig::GetConfigValue(CLRConfig::INTERNAL_EventPipeEnableStackwalk) != 0;
}

static
inline
uint32_t
ep_rt_config_value_get_buffer_guard_level (void)
{
STATIC_CONTRACT_NOTHROW;
return CLRConfig::GetConfigValue (CLRConfig::INTERNAL_EventPipeBufferGuardLevel);
}

/*
* EventPipeSampleProfiler.
*/
Expand Down Expand Up @@ -1920,5 +1931,48 @@ ep_rt_volatile_store_ptr_without_barrier (
VolatileStoreWithoutBarrier<void *> ((void **)ptr, value);
}

/*
* Memory Protection
*/

static
inline
bool
ep_rt_vprotect (
void *addr,
size_t length,
EventPipePageProtection protection)
{
STATIC_CONTRACT_NOTHROW;
DWORD oldProtect;
bool result = false;

if (protection == EP_PAGE_PROTECTION_READONLY)
result = ClrVirtualProtect (addr, length, PAGE_READONLY, &oldProtect);
else if (protection == EP_PAGE_PROTECTION_READWRITE)
result = ClrVirtualProtect (addr, length, PAGE_READWRITE, &oldProtect);

return result;
}

/*
* Fail fast
* Uses COR_E_EXECUTIONENGINE to classify as runtime/internal corruption and routes through EEPOLICY
* to ensure proper crash dump and diagnostic reporting.
*/
static
EP_ALWAYS_INLINE
void
ep_rt_fatal_error_with_message (const ep_char8_t *message)
{
if (message != nullptr) {
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

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

Should use 'is not null' instead of '!= nullptr' according to the coding guidelines for null checks.

Copilot uses AI. Check for mistakes.
fputs(reinterpret_cast<const char*>(message), stderr);
fputc('\n', stderr);
fflush(stderr);
}

RaiseFailFastException (NULL, NULL, 0);
}

#endif /* ENABLE_PERFTRACING */
#endif /* __EVENTPIPE_RT_CORECLR_H__ */
43 changes: 43 additions & 0 deletions src/mono/mono/eventpipe/ep-rt-mono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,21 @@ ep_rt_config_value_get_enable_stackwalk (void)
return value_uint32_t != 0;
}

static
inline
uint32_t
ep_rt_config_value_get_buffer_guard_level (void)
{
uint32_t buffer_guard_level = 0;
gchar *value = g_getenv ("DOTNET_EventPipeBufferGuardLevel");
if (!value)
value = g_getenv ("COMPlus_EventPipeBufferGuardLevel");
Copy link
Member

@jkotas jkotas Oct 15, 2025

Choose a reason for hiding this comment

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

We should not need to support for COMPlus_ for any new variables.

(We are keeping COMPlus_ path for now for existing variables. Ideally, we will delete it at some point.)

if (value)
buffer_guard_level = (uint32_t)atoi (value);
g_free (value);
return buffer_guard_level;
}

/*
* EventPipeSampleProfiler.
*/
Expand Down Expand Up @@ -1901,6 +1916,34 @@ ep_rt_volatile_store_ptr_without_barrier (
*ptr = value;
}


/*
* Memory Protection
*/

static
inline
bool
ep_rt_vprotect (
void *addr,
size_t length,
EventPipePageProtection protection)
{
return true;
}

/*
* Fail fast
*/

static
inline
void
ep_rt_fatal_error_with_message (const ep_char8_t *message)
{
/* Not implemented, no-op */
}

/*
* EventPipe Native Events.
*/
Expand Down
10 changes: 5 additions & 5 deletions src/native/eventpipe/ep-block.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,7 @@ bool
ep_event_block_base_write_event (
EventPipeEventBlockBase *event_block_base,
EventPipeEventInstance *event_instance,
uint32_t metadata_id,
uint64_t capture_thread_id,
uint32_t sequence_number,
uint32_t stack_id,
Expand Down Expand Up @@ -539,7 +540,6 @@ ep_event_block_base_write_event (

ep_write_buffer_uint32_t (&write_pointer, total_size);

uint32_t metadata_id = ep_event_instance_get_metadata_id (event_instance);
EP_ASSERT ((metadata_id & (1 << 31)) == 0);

metadata_id |= (!is_sorted_event ? 1 << 31 : 0);
Expand Down Expand Up @@ -579,16 +579,16 @@ ep_event_block_base_write_event (
uint8_t *header_write_pointer = &event_block_base->compressed_header[0];
EventPipeEventHeader *last_header = &event_block_base->last_header;

if (ep_event_instance_get_metadata_id (event_instance) != last_header->metadata_id) {
header_write_pointer = event_block_base_write_var_uint32 (header_write_pointer, ep_event_instance_get_metadata_id (event_instance));
if (metadata_id != last_header->metadata_id) {
header_write_pointer = event_block_base_write_var_uint32 (header_write_pointer, metadata_id);
flags |= 1;
}

if (is_sorted_event) {
flags |= (1 << 6);
}

if (last_header->sequence_number + (ep_event_instance_get_metadata_id (event_instance) != 0 ? 1 : 0) != sequence_number ||
if (last_header->sequence_number + (metadata_id != 0 ? 1 : 0) != sequence_number ||
last_header->capture_thread_id != capture_thread_id || last_header->capture_proc_number != capture_proc_number) {
header_write_pointer = event_block_base_write_var_uint32 (header_write_pointer, sequence_number - last_header->sequence_number - 1);
header_write_pointer = event_block_base_write_var_uint64 (header_write_pointer, capture_thread_id);
Expand Down Expand Up @@ -638,7 +638,7 @@ ep_event_block_base_write_event (
ep_raise_error ();
}

last_header->metadata_id = ep_event_instance_get_metadata_id (event_instance);
last_header->metadata_id = metadata_id;
last_header->sequence_number = sequence_number;
last_header->thread_id = ep_event_instance_get_thread_id (event_instance);
last_header->capture_thread_id = capture_thread_id;
Expand Down
1 change: 1 addition & 0 deletions src/native/eventpipe/ep-block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ bool
ep_event_block_base_write_event (
EventPipeEventBlockBase *event_block_base,
EventPipeEventInstance *event_instance,
uint32_t metadata_id,
uint64_t capture_thread_id,
uint32_t sequence_number,
uint32_t stack_id,
Expand Down
14 changes: 9 additions & 5 deletions src/native/eventpipe/ep-buffer-manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,17 +472,17 @@ buffer_manager_allocate_buffer_for_thread (
uint32_t buffer_size = base_buffer_size * size_multiplier;
EP_ASSERT(buffer_size > 0);


buffer_size = EP_MAX (request_size, buffer_size);
uint32_t guard_overhead = (buffer_manager->buffer_guard_level > EP_BUFFER_GUARD_LEVEL_NONE) ? EP_BUFFER_HEADER_GUARD_SIZE + EP_BUFFER_FOOTER_GUARD_SIZE : 0;
buffer_size = EP_MAX (request_size + guard_overhead, buffer_size);

// Don't allow the buffer size to exceed 1MB.
const uint32_t max_buffer_size = 1024 * 1024;
buffer_size = EP_MIN (buffer_size, max_buffer_size);


// Make sure that buffer size >= request size so that the buffer size does not
// Make sure that buffer size >= request size + guard overhead so that the buffer size does not
// determine the max event size.
EP_ASSERT (request_size <= buffer_size);
EP_ASSERT (request_size + guard_overhead <= buffer_size);

// Make the buffer size fit into with pagesize-aligned block, since ep_rt_valloc0 expects page-aligned sizes to be passed as arguments
buffer_size = (buffer_size + ep_rt_system_get_alloc_granularity () - 1) & ~(uint32_t)(ep_rt_system_get_alloc_granularity () - 1);
Expand All @@ -493,7 +493,7 @@ buffer_manager_allocate_buffer_for_thread (

// The sequence counter is exclusively mutated on this thread so this is a thread-local read.
sequence_number = ep_thread_session_state_get_volatile_sequence_number (thread_session_state);
new_buffer = ep_buffer_alloc (buffer_size, ep_thread_session_state_get_thread (thread_session_state), sequence_number);
new_buffer = ep_buffer_alloc (buffer_size, ep_thread_session_state_get_thread (thread_session_state), sequence_number, buffer_manager->buffer_guard_level);
ep_raise_error_if_nok (new_buffer != NULL);

// Adding a buffer to the buffer list requires us to take the lock.
Expand Down Expand Up @@ -808,6 +808,7 @@ ep_buffer_manager_alloc (
size_t max_size_of_all_buffers,
size_t sequence_point_allocation_budget)
{
uint32_t buffer_guard_level;
EventPipeBufferManager *instance = ep_rt_object_alloc (EventPipeBufferManager);
ep_raise_error_if_nok (instance != NULL);

Expand Down Expand Up @@ -851,6 +852,9 @@ ep_buffer_manager_alloc (
instance->remaining_sequence_point_alloc_budget = sequence_point_allocation_budget;
}

buffer_guard_level = EP_MIN (ep_rt_config_value_get_buffer_guard_level (), EP_BUFFER_GUARD_LEVEL_PROTECT_OUTSIDE_WRITES);
Copy link
Member

@noahfalk noahfalk Oct 14, 2025

Choose a reason for hiding this comment

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

It looks like this is trying to clamp the range into valid enum values. I'd suggest define EP_BUFFER_GUARD_LEVEL_MAX = EP_BUFFER_GUARD_LEVEL_PROTECT_OUTSIDE_WRITES in the enum definition, then use that MAX value here. It better conveys the intent and its more likely to get updated correctly if we ever change the levels in the future.

instance->buffer_guard_level = (EventPipeBufferGuardLevel)buffer_guard_level;

ep_on_exit:
return instance;

Expand Down
3 changes: 3 additions & 0 deletions src/native/eventpipe/ep-buffer-manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@ struct _EventPipeBufferManager_Internal {
// number of times an event was dropped due to it being too
// large to fit in the 64KB size limit
volatile int64_t num_oversized_events_dropped;
// EventPipeBufferGuardLevel during allocation
EventPipeBufferGuardLevel buffer_guard_level;

#ifdef EP_CHECKED_BUILD
volatile int64_t num_events_stored;
Expand All @@ -146,6 +148,7 @@ struct _EventPipeBufferManager {
#endif

EP_DEFINE_GETTER_REF(EventPipeBufferManager *, buffer_manager, ep_rt_wait_event_handle_t *, rt_wait_event)
EP_DEFINE_GETTER(EventPipeBufferManager *, buffer_manager, EventPipeBufferGuardLevel, buffer_guard_level)

EventPipeBufferManager *
ep_buffer_manager_alloc (
Expand Down
Loading
Loading