From: Johan Lorensson Date: Fri, 22 Jul 2022 07:44:45 +0000 (+0200) Subject: Fixes overwrite of last captured managed stack frame address in EventPipe stack trace... X-Git-Tag: accepted/tizen/unified/riscv/20231226.055536~7633 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=0c32267664e1b9913f2329827d998646fecafd91;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Fixes overwrite of last captured managed stack frame address in EventPipe stack trace. (#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 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 --- 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 --- 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 --- 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