Skip to content

Commit

Permalink
Fixes overwrite of last captured managed stack frame address in Event…
Browse files Browse the repository at this point in the history
…Pipe stack trace. (dotnet#72362)

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 dotnet#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.
  • Loading branch information
lateralusX authored Jul 22, 2022
1 parent d5d9d52 commit 0c32267
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 25 deletions.
6 changes: 3 additions & 3 deletions src/native/eventpipe/ep-buffer.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,21 @@ 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)
ep_raise_error ();

uint32_t proc_number;
proc_number = ep_rt_current_processor_get_number ();
EventPipeEventInstance *instance;
instance = ep_event_instance_init (
(EventPipeEventInstance *)buffer->current,
ep_event,
Expand Down
4 changes: 2 additions & 2 deletions src/native/eventpipe/ep-event-instance.h
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

/*
Expand Down
38 changes: 18 additions & 20 deletions src/native/eventpipe/ep-stack-contents.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 0c32267

Please sign in to comment.