[runtime] Fix Mono BStr marshaling, cleanup IL generation (#35804)

* [COM] Set and free BSTR size correctly

Apparently, while the previous 4 bytes contain the size, the actual allocation is either 4 or 8 bytes depending on the platform! See https://github.com/dotnet/runtime/blob/fcd862e06413a000f9cafa9d2f359226c60b9b42/src/coreclr/tests/src/Common/Platform/platformdefines.cpp#L418

* [meta] Cleanup string marshalling and support additional conversions

* [COM] Align BSTR creation to 16 bytes

* Enable test

* Feedback

* Fix offset

* Add support for AnsiBStr and TBStr marshalling

Also makes BStr changes netcore-only and enables AnsiBStr test. No TBStr test appears to exist.

* String marshaling IL gen refactoring

* Fix Windows build

* Feedback

src/coreclr/tests/issues.targets

@@ -1118,12 +1118,6 @@
         <ExcludeList Include="$(XunitTestBinBase)/Interop/PInvoke/SizeParamIndex/ReversePInvoke/PassingByRef/PassingByRefTest/**">
             <Issue>https://github.com/dotnet/runtime/issues/34196</Issue>
         </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/Interop/StringMarshalling/AnsiBSTR/AnsiBStrTest/**">
-            <Issue>needs triage</Issue>
-        </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/Interop/StringMarshalling/BSTR/BSTRTest/**">
-            <Issue>https://github.com/dotnet/runtime/issues/34375</Issue>
-        </ExcludeList>
         <ExcludeList Include="$(XunitTestBinBase)/Interop/StringMarshalling/LPSTR/LPSTRTest/**">
             <Issue>needs triage</Issue>
         </ExcludeList>

src/mono/mono/metadata/cominterop.c

@@ -2988,6 +2988,26 @@ init_com_provider_ms (void)
 #endif // WIN32
 #endif // DISABLE_COM
 
+// This function is used regardless of the BSTR type, so cast the return value
+// Inputted string length, in bytes, should include the null terminator
+// Returns the start of the string itself
+static gpointer
+mono_bstr_alloc (size_t str_byte_len)
+{
+	// Allocate string length plus pointer-size integer to store the length, aligned to 16 bytes
+	size_t alloc_size = str_byte_len + SIZEOF_VOID_P;
+	alloc_size += (16 - 1);
+	alloc_size &= ~(16 - 1);
+	gpointer ret = g_malloc0 (alloc_size);
+	return ret ? (char *)ret + SIZEOF_VOID_P : NULL;
+}
+
+static void
+mono_bstr_set_length (gunichar2 *bstr, int slen)
+{
+	*((guint32 *)bstr - 1) = slen * sizeof (gunichar2);
+}
+
 /* PTR can be NULL */
 mono_bstr
 mono_ptr_to_bstr (const gunichar2* ptr, int slen)
@@ -2998,12 +3018,24 @@ mono_ptr_to_bstr (const gunichar2* ptr, int slen)
 #ifndef DISABLE_COM
 	if (com_provider == MONO_COM_DEFAULT) {
 #endif
+		// In Mono, historically BSTR was allocated with a guaranteed size prefix of 4 bytes regardless of platform.
+		// Presumably this is due to the BStr documentation page, which indicates that behavior and then directs you to call
+		// SysAllocString on Windows to handle the allocation for you. Unfortunately, this is not actually how it works:
+		// The allocation pre-string is pointer-sized, and then only 4 bytes are used for the length regardless. Additionally,
+		// the total length is also aligned to a 16-byte boundary. This preserves the old behavior on legacy and fixes it for
+		// netcore moving forward.
+#ifdef ENABLE_NETCORE
+		mono_bstr const s = (mono_bstr)mono_bstr_alloc ((slen + 1) * sizeof (gunichar2));
+		if (s == NULL)
+			return NULL;
+#else
 		/* allocate len + 1 utf16 characters plus 4 byte integer for length*/
 		guint32 * const ret = (guint32 *)g_malloc ((slen + 1) * sizeof (gunichar2) + sizeof (guint32));
 		if (ret == NULL)
 			return NULL;
 		mono_bstr const s = (mono_bstr)(ret + 1);
-		*ret = slen * sizeof (gunichar2);
+#endif
+		mono_bstr_set_length (s, slen);
 		if (ptr)
 			memcpy (s, ptr, slen * sizeof (gunichar2));
 		s [slen] = 0;
@@ -3024,7 +3056,21 @@ mono_ptr_to_bstr (const gunichar2* ptr, int slen)
 #endif
 }
 
-static MonoStringHandle
+char *
+mono_ptr_to_ansibstr (const char *ptr, size_t slen)
+{
+	// FIXME: should this behave differently without DISABLE_COM?
+	char *s = (char *)mono_bstr_alloc ((slen + 1) * sizeof(char));
+	if (s == NULL)
+		return NULL;
+	*((guint32 *)s - 1) = slen * sizeof (char);
+	if (ptr)
+		memcpy (s, ptr, slen * sizeof (char));
+	s [slen] = 0;
+	return s;
+}
+
+MonoStringHandle
 mono_string_from_bstr_checked (mono_bstr_const bstr, MonoError *error)
 {
 	if (!bstr)
@@ -3079,7 +3125,11 @@ mono_free_bstr (/*mono_bstr_const*/gpointer bstr)
 #ifndef DISABLE_COM
 	if (com_provider == MONO_COM_DEFAULT) {
 #endif
+#ifdef ENABLE_NETCORE
+		g_free (((char *)bstr) - SIZEOF_VOID_P);
+#else // In Mono, historically BSTR was allocated with a guaranteed size prefix of 4 bytes regardless of platform
 		g_free (((char *)bstr) - 4);
+#endif
 #ifndef DISABLE_COM
 	} else if (com_provider == MONO_COM_MS && init_com_provider_ms ()) {
 		sys_free_string_ms ((mono_bstr_const)bstr);

src/mono/mono/metadata/cominterop.h

@@ -58,6 +58,9 @@ mono_cominterop_emit_marshal_safearray (EmitMarshalContext *m, int argnum,
 MONO_API MONO_RT_EXTERNAL_ONLY MonoString *
 mono_string_from_bstr (/*mono_bstr*/gpointer bstr);
 
+MonoStringHandle
+mono_string_from_bstr_checked (mono_bstr_const bstr, MonoError *error);
+
 MONO_API void 
 mono_free_bstr (/*mono_bstr_const*/gpointer bstr);
 

src/mono/mono/metadata/icall-def.h

@@ -1168,15 +1168,18 @@ MONO_HANDLE_REGISTER_ICALL (mono_marshal_string_to_utf16_copy, gunichar2_ptr, 1,
 MONO_HANDLE_REGISTER_ICALL (mono_object_isinst_icall, MonoObject, 2, (MonoObject, MonoClass_ptr))
 MONO_HANDLE_REGISTER_ICALL (mono_string_builder_to_utf16, gunichar2_ptr, 1, (MonoStringBuilder))
 MONO_HANDLE_REGISTER_ICALL (mono_string_builder_to_utf8, char_ptr, 1, (MonoStringBuilder))
+MONO_HANDLE_REGISTER_ICALL (mono_string_from_ansibstr, MonoString, 1, (const_char_ptr))
 MONO_HANDLE_REGISTER_ICALL (mono_string_from_bstr_icall, MonoString, 1, (mono_bstr_const))
 MONO_HANDLE_REGISTER_ICALL (mono_string_from_byvalstr, MonoString, 2, (const_char_ptr, int))
 MONO_HANDLE_REGISTER_ICALL (mono_string_from_byvalwstr, MonoString, 2, (const_gunichar2_ptr, int))
+MONO_HANDLE_REGISTER_ICALL (mono_string_from_tbstr, MonoString, 1, (gpointer))
 MONO_HANDLE_REGISTER_ICALL (mono_string_new_len_wrapper, MonoString, 2, (const_char_ptr, guint))
 MONO_HANDLE_REGISTER_ICALL (mono_string_new_wrapper_internal, MonoString, 1, (const_char_ptr))
-MONO_HANDLE_REGISTER_ICALL (mono_string_to_ansibstr, gpointer, 1, (MonoString))
+MONO_HANDLE_REGISTER_ICALL (mono_string_to_ansibstr, char_ptr, 1, (MonoString))
 MONO_HANDLE_REGISTER_ICALL (mono_string_to_bstr, mono_bstr, 1, (MonoString))
 MONO_HANDLE_REGISTER_ICALL (mono_string_to_byvalstr, void, 3, (char_ptr, MonoString, int))
 MONO_HANDLE_REGISTER_ICALL (mono_string_to_byvalwstr, void, 3, (gunichar2_ptr, MonoString, int))
+MONO_HANDLE_REGISTER_ICALL (mono_string_to_tbstr, gpointer, 1, (MonoString))
 MONO_HANDLE_REGISTER_ICALL (mono_string_to_utf16_internal, mono_unichar2_ptr, 1, (MonoString))
 MONO_HANDLE_REGISTER_ICALL (mono_string_to_utf32_internal, mono_unichar4_ptr, 1, (MonoString)) // embedding API
 MONO_HANDLE_REGISTER_ICALL (mono_string_utf16_to_builder, void, 2, (MonoStringBuilder, const_gunichar2_ptr))

src/mono/mono/metadata/jit-icall-reg.h

@@ -276,15 +276,18 @@ MONO_JIT_ICALL (mono_resume_unwind) \
 MONO_JIT_ICALL (mono_rethrow_preserve_exception) \
 MONO_JIT_ICALL (mono_string_builder_to_utf16) \
 MONO_JIT_ICALL (mono_string_builder_to_utf8) \
+MONO_JIT_ICALL (mono_string_from_ansibstr) \
 MONO_JIT_ICALL (mono_string_from_bstr_icall) \
 MONO_JIT_ICALL (mono_string_from_byvalstr) \
 MONO_JIT_ICALL (mono_string_from_byvalwstr) \
+MONO_JIT_ICALL (mono_string_from_tbstr) \
 MONO_JIT_ICALL (mono_string_new_len_wrapper) \
 MONO_JIT_ICALL (mono_string_new_wrapper_internal) \
 MONO_JIT_ICALL (mono_string_to_ansibstr) \
 MONO_JIT_ICALL (mono_string_to_bstr) \
 MONO_JIT_ICALL (mono_string_to_byvalstr) \
 MONO_JIT_ICALL (mono_string_to_byvalwstr) \
+MONO_JIT_ICALL (mono_string_to_tbstr) \
 MONO_JIT_ICALL (mono_string_to_utf16_internal) \
 MONO_JIT_ICALL (mono_string_to_utf8str) \
 MONO_JIT_ICALL (mono_string_utf16_to_builder) \