Fixes overwrite of last captured managed stack frame address in EventPipe stack trace...

When collecting stack frames in EventPipe, buffer manager buffer layout
depends on the struct layout of the compiler up until data gets
serialized into blocks and put into output stream following nettrace
format.

In the past, there was a 1:1 map between the data collected for a stack
trace and what was copied into buffer manager memory. Due to inefficiency,
wasting a lot of memory when having small stack traces, this was
optimized by https://github.com/dotnet/runtime/pull/68134, greatly
reducing overhead and improved EventPipe throughput.

That change started to use a different struct when capturing the
callstack compared to the layout written into buffer manager.
Since buffer manager memory still relies on compiler struct layout,
code must take that into account when copying stack data into
buffer manager memory, but the new optimized implementation didn't,
meaning that it fails in cases where compiler adds padding inside
EventPipeStackContentsInstance (done on 64-bit bit systems).
That in turn will write event payload, starting 4 bytes into last
captured stack frame causing issues for tools to symbolicate address,
but payload data will still be correct, since EventPipeEventInstance
keeps pointer to payload data, meaning most of the event will still be
correct, covering up the overwrite to only affect last managed stack
frame and only on 64-bit release builds.

Fix adjust the size calculation and make sure it takes any padding
added by compiler into the computation of EventPipeEventInstance size.

diff --git a/src/native/eventpipe/ep-buffer.c b/src/native/eventpipe/ep-buffer.c
index 5b24189..0bf8150 100644 (file)
--- a/src/native/eventpipe/ep-buffer.c
+++ b/src/native/eventpipe/ep-buffer.c
@@ -82,13 +82,14 @@ ep_buffer_write_event (
        EP_ASSERT ((EventPipeBufferState)buffer->state == EP_BUFFER_STATE_WRITABLE);
 
        bool success = true;
+       EventPipeEventInstance *instance = NULL;
 
        // Calculate the location of the data payload.
        uint8_t *data_dest;
-       data_dest = (ep_event_payload_get_size (payload) == 0 ? NULL : buffer->current + sizeof(EventPipeEventInstance) - sizeof (EventPipeStackContentsInstance) + ep_stack_contents_get_instance_size (stack));
+       data_dest = (ep_event_payload_get_size (payload) == 0 ? NULL : buffer->current + sizeof (*instance) - sizeof (instance->stack_contents_instance.stack_frames) + ep_stack_contents_get_full_size (stack));
 
        // Calculate the size of the event.
-       uint32_t event_size = sizeof (EventPipeEventInstance) - sizeof (EventPipeStackContentsInstance) + ep_stack_contents_get_instance_size (stack) + ep_event_payload_get_size (payload);
+       uint32_t event_size = sizeof (*instance) - sizeof (instance->stack_contents_instance.stack_frames) + ep_stack_contents_get_full_size (stack) + ep_event_payload_get_size (payload);
 
        // Make sure we have enough space to write the event.
        if(buffer->current + event_size > buffer->limit)
@@ -96,7 +97,6 @@ ep_buffer_write_event (
 
        uint32_t proc_number;
        proc_number = ep_rt_current_processor_get_number ();
-       EventPipeEventInstance *instance;
        instance = ep_event_instance_init (
                (EventPipeEventInstance *)buffer->current,
                ep_event,

diff --git a/src/native/eventpipe/ep-event-instance.h b/src/native/eventpipe/ep-event-instance.h
index 11f7336..0ffb516 100644 (file)
--- a/src/native/eventpipe/ep-event-instance.h
+++ b/src/native/eventpipe/ep-event-instance.h
@@ -104,8 +104,8 @@ ep_event_instance_get_flattened_size (const EventPipeEventInstance *ep_event_ins
 {
        EP_ASSERT (ep_event_instance != NULL);
        return ep_event_instance_get_data (ep_event_instance) ?
-               sizeof (EventPipeEventInstance) - sizeof (EventPipeStackContentsInstance) + ep_stack_contents_instance_get_total_size (ep_event_instance_get_stack_contents_instance_cref (ep_event_instance)) + ep_event_instance_get_data_len (ep_event_instance) :
-               sizeof (EventPipeEventInstance) - sizeof (EventPipeStackContentsInstance) + ep_stack_contents_instance_get_total_size (ep_event_instance_get_stack_contents_instance_cref (ep_event_instance));
+               sizeof (*ep_event_instance) - sizeof (ep_event_instance->stack_contents_instance.stack_frames) + ep_stack_contents_instance_get_full_size (ep_event_instance_get_stack_contents_instance_cref (ep_event_instance)) + ep_event_instance_get_data_len (ep_event_instance) :
+               sizeof (*ep_event_instance) - sizeof (ep_event_instance->stack_contents_instance.stack_frames) + ep_stack_contents_instance_get_full_size (ep_event_instance_get_stack_contents_instance_cref (ep_event_instance));
 }
 
 /*

diff --git a/src/native/eventpipe/ep-stack-contents.h b/src/native/eventpipe/ep-stack-contents.h
index b006bae..80abaf8 100644 (file)
--- a/src/native/eventpipe/ep-stack-contents.h
+++ b/src/native/eventpipe/ep-stack-contents.h
@@ -140,13 +140,12 @@ ep_stack_contents_get_size (const EventPipeStackContents *stack_contents)
 static
 inline
 uint32_t
-ep_stack_contents_get_instance_size (const EventPipeStackContents *stack_contents)
+ep_stack_contents_get_full_size (const EventPipeStackContents *stack_contents)
 {
-       // The total size including the size
 #ifdef EP_CHECKED_BUILD
-       return stack_contents ? (ep_stack_contents_get_next_available_frame (stack_contents) * sizeof (uintptr_t) * 2) + sizeof (uint32_t) : sizeof (uint32_t);
+       return stack_contents ? (ep_stack_contents_get_next_available_frame (stack_contents) * sizeof (uintptr_t) * 2) : 0;
 #else /* EP_CHECKED_BUILD */
-       return stack_contents ? (ep_stack_contents_get_next_available_frame (stack_contents) * sizeof (uintptr_t)) + sizeof (uint32_t) : sizeof (uint32_t);
+       return stack_contents ? (ep_stack_contents_get_next_available_frame (stack_contents) * sizeof (uintptr_t)): 0;
 #endif
 }
 
@@ -215,58 +214,57 @@ EventPipeStackContentsInstance *
 ep_stack_contents_instance_alloc (void);
 
 EventPipeStackContentsInstance *
-ep_stack_contents_instance_init (EventPipeStackContentsInstance *stack_contents);
+ep_stack_contents_instance_init (EventPipeStackContentsInstance *stack_contents_instance);
 
 void
-ep_stack_contents_instance_fini (EventPipeStackContentsInstance *stack_contents);
+ep_stack_contents_instance_fini (EventPipeStackContentsInstance *stack_contents_instance);
 
 void
-ep_stack_contents_instance_free (EventPipeStackContentsInstance *stack_contents);
+ep_stack_contents_instance_free (EventPipeStackContentsInstance *stack_contents_instance);
 
 static
 inline
 void
-ep_stack_contents_instance_reset (EventPipeStackContentsInstance *stack_contents)
+ep_stack_contents_instance_reset (EventPipeStackContentsInstance *stack_contents_instance)
 {
-       ep_stack_contents_instance_set_next_available_frame (stack_contents, 0);
+       ep_stack_contents_instance_set_next_available_frame (stack_contents_instance, 0);
 }
 
 static
 inline
 uint32_t
-ep_stack_contents_instance_get_size (const EventPipeStackContentsInstance *stack_contents)
+ep_stack_contents_instance_get_size (const EventPipeStackContentsInstance *stack_contents_instance)
 {
-       EP_ASSERT (stack_contents != NULL);
-       return (ep_stack_contents_instance_get_next_available_frame (stack_contents) * sizeof (uintptr_t));
+       EP_ASSERT (stack_contents_instance != NULL);
+       return (ep_stack_contents_instance_get_next_available_frame (stack_contents_instance) * sizeof (uintptr_t));
 }
 
 static
 inline
 uint32_t
-ep_stack_contents_instance_get_length (EventPipeStackContentsInstance *stack_contents)
+ep_stack_contents_instance_get_length (EventPipeStackContentsInstance *stack_contents_instance)
 {
-       return ep_stack_contents_instance_get_next_available_frame (stack_contents);
+       return ep_stack_contents_instance_get_next_available_frame (stack_contents_instance);
 }
 
 static
 inline
 uint32_t
-ep_stack_contents_instance_get_total_size (const EventPipeStackContentsInstance *stack_contents_instance)
+ep_stack_contents_instance_get_full_size (const EventPipeStackContentsInstance *stack_contents_instance)
 {
-       // The total size including the size
 #ifdef EP_CHECKED_BUILD
-       return stack_contents_instance ? (ep_stack_contents_instance_get_next_available_frame (stack_contents_instance) * sizeof (uintptr_t) * 2) + sizeof (uint32_t) : sizeof (uint32_t);
+       return stack_contents_instance ? (ep_stack_contents_instance_get_next_available_frame (stack_contents_instance) * sizeof (uintptr_t) * 2) : 0;
 #else /* EP_CHECKED_BUILD */
-       return stack_contents_instance ? (ep_stack_contents_instance_get_next_available_frame (stack_contents_instance) * sizeof (uintptr_t)) + sizeof (uint32_t) : sizeof (uint32_t);
+       return stack_contents_instance ? (ep_stack_contents_instance_get_next_available_frame (stack_contents_instance) * sizeof (uintptr_t)) : 0;
 #endif
 }
 
 static
 inline
 bool
-ep_stack_contents_instance_is_empty (EventPipeStackContentsInstance *stack_contents)
+ep_stack_contents_instance_is_empty (EventPipeStackContentsInstance *stack_contents_instance)
 {
-       return (ep_stack_contents_instance_get_next_available_frame (stack_contents) == 0);
+       return (ep_stack_contents_instance_get_next_available_frame (stack_contents_instance) == 0);
 }
 
 static