Fix unaligned UTF16 string read in collect tracing EventPipe command. (#66828)

Collect tracing 2 EventPipe command triggers an unaligned UTF16 string
read that could cause a SIGBUS on platforms not supporting unalinged
reads of UTF16 strings (so far only seen on 32-bit ARM Linux CI machine).

On CoreCLR this could even cause a unalinged int read due to
optimizations used in UTF8Encoding::GetByteCount.

diff --git a/src/native/eventpipe/ds-eventpipe-protocol.c b/src/native/eventpipe/ds-eventpipe-protocol.c
index 6405e42b0f67ffe5ae1bbb07c61f3f09dc41cc9f..36c3588b2a3db858db4e26aacf9cd2a078468149 100644 (file)
--- a/src/native/eventpipe/ds-eventpipe-protocol.c
+++ b/src/native/eventpipe/ds-eventpipe-protocol.c
@@ -140,7 +140,7 @@ eventpipe_collect_tracing_command_try_parse_rundown_requested (
        EP_ASSERT (buffer_len != NULL);
        EP_ASSERT (rundown_requested != NULL);
 
-       return ds_ipc_message_try_parse_value (buffer, buffer_len, (uint8_t *)rundown_requested, (uint32_t)sizeof (bool));
+       return ds_ipc_message_try_parse_value (buffer, buffer_len, (uint8_t *)rundown_requested, 1);
 }
 
 static
@@ -160,6 +160,9 @@ eventpipe_collect_tracing_command_try_parse_config (
        const uint32_t max_count_configs = 1000;
        uint32_t count_configs = 0;
 
+       uint8_t *provider_name_byte_array = NULL;
+       uint8_t *filter_data_byte_array = NULL;
+
        ep_char8_t *provider_name_utf8 = NULL;
        ep_char8_t *filter_data_utf8 = NULL;
 
@@ -176,20 +179,27 @@ eventpipe_collect_tracing_command_try_parse_config (
                ep_raise_error_if_nok (ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, &log_level));
                ep_raise_error_if_nok (log_level <= EP_EVENT_LEVEL_VERBOSE);
 
-               const ep_char16_t *provider_name = NULL;
-               ep_raise_error_if_nok (ds_ipc_message_try_parse_string_utf16_t (buffer, buffer_len, &provider_name));
+               uint32_t provider_name_byte_array_len = 0;
+               ep_raise_error_if_nok (ds_ipc_message_try_parse_string_utf16_t_byte_array_alloc (buffer, buffer_len, &provider_name_byte_array, &provider_name_byte_array_len));
 
-               provider_name_utf8 = ep_rt_utf16_to_utf8_string (provider_name, -1);
+               provider_name_utf8 = ep_rt_utf16_to_utf8_string ((const ep_char16_t *)provider_name_byte_array, -1);
                ep_raise_error_if_nok (provider_name_utf8 != NULL);
 
                ep_raise_error_if_nok (!ep_rt_utf8_string_is_null_or_empty (provider_name_utf8));
 
-               const ep_char16_t *filter_data = NULL; // This parameter is optional.
-               ds_ipc_message_try_parse_string_utf16_t (buffer, buffer_len, &filter_data);
+               ep_rt_byte_array_free (provider_name_byte_array);
+               provider_name_byte_array = NULL;
+
+               uint32_t filter_data_byte_array_len = 0;
+               ep_raise_error_if_nok (ds_ipc_message_try_parse_string_utf16_t_byte_array_alloc (buffer, buffer_len, &filter_data_byte_array, &filter_data_byte_array_len));
 
-               if (filter_data) {
-                       filter_data_utf8 = ep_rt_utf16_to_utf8_string (filter_data, -1);
+               // This parameter is optional.
+               if (filter_data_byte_array) {
+                       filter_data_utf8 = ep_rt_utf16_to_utf8_string ((const ep_char16_t *)filter_data_byte_array, -1);
                        ep_raise_error_if_nok (filter_data_utf8 != NULL);
+
+                       ep_rt_byte_array_free (filter_data_byte_array);
+                       filter_data_byte_array = NULL;
                }
 
                EventPipeProviderConfiguration provider_config;
@@ -209,7 +219,9 @@ ep_on_exit:
 
 ep_on_error:
        count_configs = 0;
+       ep_rt_byte_array_free (provider_name_byte_array);
        ep_rt_utf8_string_free (provider_name_utf8);
+       ep_rt_byte_array_free (filter_data_byte_array);
        ep_rt_utf8_string_free (filter_data_utf8);
        ep_exit_error_handler ();
 }

diff --git a/src/native/eventpipe/ds-protocol.c b/src/native/eventpipe/ds-protocol.c
index 606376449ad9ba14e2fd922124ce1e6febcd3a28..a49b741013885d50b7ac511f5e62bf9c67239faa 100644 (file)
--- a/src/native/eventpipe/ds-protocol.c
+++ b/src/native/eventpipe/ds-protocol.c
@@ -59,6 +59,14 @@ ipc_message_flatten (
        uint16_t payload_len,
        ds_ipc_flatten_payload_func flatten_payload);
 
+static
+bool
+ipc_message_try_parse_string_utf16_t_byte_array (
+       uint8_t **buffer,
+       uint32_t *buffer_len,
+       const uint8_t **string_byte_array,
+       uint32_t *string_byte_array_len);
+
 /*
 * DiagnosticsIpc
 */
@@ -306,6 +314,50 @@ ep_on_error:
        ep_exit_error_handler ();
 }
 
+static
+bool
+ipc_message_try_parse_string_utf16_t_byte_array (
+       uint8_t **buffer,
+       uint32_t *buffer_len,
+       const uint8_t **string_byte_array,
+       uint32_t *string_byte_array_len)
+{
+       EP_ASSERT (buffer != NULL);
+       EP_ASSERT (buffer_len != NULL);
+       EP_ASSERT (string_byte_array != NULL);
+       EP_ASSERT (string_byte_array_len != NULL);
+
+       bool result = false;
+
+       ep_raise_error_if_nok (ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, string_byte_array_len));
+       *string_byte_array_len *= sizeof (ep_char16_t);
+
+       if (*string_byte_array_len != 0) {
+               if (*string_byte_array_len > *buffer_len)
+                       ep_raise_error ();
+
+               if (((const ep_char16_t *)*buffer) [(*string_byte_array_len / sizeof (ep_char16_t)) - 1] != 0)
+                       ep_raise_error ();
+
+               *string_byte_array = *buffer;
+
+       } else {
+               *string_byte_array = NULL;
+       }
+
+       *buffer = *buffer + *string_byte_array_len;
+       *buffer_len = *buffer_len - *string_byte_array_len;
+
+       result = true;
+
+ep_on_exit:
+       return result;
+
+ep_on_error:
+       EP_ASSERT (!result);
+       ep_exit_error_handler ();
+}
+
 DiagnosticsIpcMessage *
 ds_ipc_message_init (DiagnosticsIpcMessage *message)
 {
@@ -383,38 +435,35 @@ ds_ipc_message_try_parse_uint32_t (
        return result;
 }
 
-// TODO: Strings are in little endian format in buffer.
 bool
-ds_ipc_message_try_parse_string_utf16_t (
+ds_ipc_message_try_parse_string_utf16_t_byte_array_alloc (
        uint8_t **buffer,
        uint32_t *buffer_len,
-       const ep_char16_t **value)
+       uint8_t **string_byte_array,
+       uint32_t *string_byte_array_len)
 {
        EP_ASSERT (buffer != NULL);
        EP_ASSERT (buffer_len != NULL);
-       EP_ASSERT (value != NULL);
+       EP_ASSERT (string_byte_array != NULL);
+       EP_ASSERT (string_byte_array_len != NULL);
 
        bool result = false;
 
-       uint32_t string_len = 0;
-       ep_raise_error_if_nok (ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, &string_len));
+       const uint8_t *temp_buffer = NULL;
+       uint32_t temp_buffer_len = 0;
 
-       if (string_len != 0) {
-               if (string_len > (*buffer_len / sizeof (ep_char16_t)))
-                       ep_raise_error ();
-
-               if (((const ep_char16_t *)*buffer) [string_len - 1] != 0)
-                       ep_raise_error ();
+       ep_raise_error_if_nok (ipc_message_try_parse_string_utf16_t_byte_array (buffer, buffer_len, (const uint8_t **)&temp_buffer, &temp_buffer_len));
 
-               *value = (ep_char16_t *)*buffer;
+       if (temp_buffer_len != 0) {
+               *string_byte_array = ep_rt_byte_array_alloc (temp_buffer_len);
+               ep_raise_error_if_nok (*string_byte_array != NULL);
 
+               memcpy (*string_byte_array, temp_buffer, temp_buffer_len);
        } else {
-               *value = NULL;
+               *string_byte_array = NULL;
        }
 
-       *buffer = *buffer + (string_len * sizeof (ep_char16_t));
-       *buffer_len = *buffer_len - (string_len * sizeof (ep_char16_t));
-
+       *string_byte_array_len = temp_buffer_len;
        result = true;
 
 ep_on_exit:
@@ -425,6 +474,21 @@ ep_on_error:
        ep_exit_error_handler ();
 }
 
+bool
+ds_ipc_message_try_parse_string_utf16_t (
+       uint8_t **buffer,
+       uint32_t *buffer_len,
+       const ep_char16_t **value)
+{
+       EP_ASSERT (buffer != NULL);
+       EP_ASSERT (buffer_len != NULL);
+       EP_ASSERT (value != NULL);
+       EP_ASSERT (!(((size_t)*buffer) & 0x1));
+
+       uint32_t string_byte_array_len = 0;
+       return ipc_message_try_parse_string_utf16_t_byte_array (buffer, buffer_len, (const uint8_t **)value, &string_byte_array_len);
+}
+
 bool
 ds_ipc_message_initialize_header_uint32_t_payload (
        DiagnosticsIpcMessage *message,

diff --git a/src/native/eventpipe/ds-protocol.h b/src/native/eventpipe/ds-protocol.h
index 137fb7fd1fcc50a9cb85091093383cbe40017129..4f6e317044da40e8121d7239c2f2e94abc224bd3 100644 (file)
--- a/src/native/eventpipe/ds-protocol.h
+++ b/src/native/eventpipe/ds-protocol.h
@@ -145,6 +145,13 @@ ds_ipc_message_try_parse_int32_t (
        return ds_ipc_message_try_parse_uint32_t (buffer, buffer_len, (uint32_t *)value);
 }
 
+bool
+ds_ipc_message_try_parse_string_utf16_t_byte_array_alloc (
+       uint8_t **buffer,
+       uint32_t *buffer_len,
+       uint8_t **string_byte_array,
+       uint32_t *string_byte_array_len);
+
 bool
 ds_ipc_message_try_parse_string_utf16_t (
        uint8_t **buffer,