From c5cdfaec1e0973ced3f97ef589cd0bece56067ad Mon Sep 17 00:00:00 2001 From: =?utf8?q?Henric=20M=C3=BCller?= Date: Fri, 10 Jun 2016 14:33:17 +0200 Subject: [PATCH] Moving BSTR conv to native code in SecureStringToBSTR. Previous implementation laid out the BSTR structure in directly memory in C# code. This layout was not correct for windows. Furthermore memory was allocated using AllocCoTaskMem but later freed using SysFreeString which on windows are different memory areas. Third issue is that SecureString asumes big-endian when copying the chars to its internal byte buffer. So on little-endian environments, a swap of bytes is needed before the buffer is sent to native side for BSTR conversion. This should probably be handled internally in SecureString instead. --- .../System.Runtime.InteropServices/Marshal.cs | 34 +++++------ mono/metadata/cominterop.c | 56 ++++++++++++------- mono/metadata/icall-def.h | 2 + mono/metadata/marshal.h | 8 ++- 4 files changed, 61 insertions(+), 39 deletions(-) diff --git a/mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs b/mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs index a1b028b08c1..f983beeb634 100644 --- a/mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs +++ b/mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs @@ -1051,25 +1051,21 @@ namespace System.Runtime.InteropServices { if (s == null) throw new ArgumentNullException ("s"); - int len = s.Length; - IntPtr ctm = AllocCoTaskMem ((len+1) * 2 + 4); - byte [] buffer = null; - WriteInt32 (ctm, 0, len*2); - try { - buffer = s.GetBuffer (); - for (int i = 0; i < len; i++) - WriteInt16 (ctm, 4 + (i * 2), (short) ((buffer [(i*2)] << 8) | (buffer [i*2+1]))); - WriteInt16 (ctm, 4 + buffer.Length, 0); - } finally { - if (buffer != null) - for (int i = buffer.Length; i > 0; ){ - i--; - buffer [i] = 0; - } + byte[] buffer = s.GetBuffer (); + int len = s.Length; + + // SecureString doesn't take endian-ness into account. + // Therefore swap bytes here before we send it to c-side if little-endian. + if (BitConverter.IsLittleEndian) { + for (int i = 0; i < buffer.Length; i += 2) { + byte b = buffer[i]; + buffer[i] = buffer[i + 1]; + buffer[i + 1] = b; + } } - return (IntPtr) ((long)ctm + 4); - } + return BufferToBSTR (buffer, len); + } public static IntPtr SecureStringToCoTaskMemAnsi (SecureString s) { @@ -1156,6 +1152,10 @@ namespace System.Runtime.InteropServices throw ex; } + + [MethodImplAttribute(MethodImplOptions.InternalCall)] + public extern static IntPtr BufferToBSTR (Array ptr, int slen); + [MethodImplAttribute(MethodImplOptions.InternalCall)] public extern static IntPtr UnsafeAddrOfPinnedArrayElement (Array arr, int index); diff --git a/mono/metadata/cominterop.c b/mono/metadata/cominterop.c index 889c19ece4f..bedd740079b 100644 --- a/mono/metadata/cominterop.c +++ b/mono/metadata/cominterop.c @@ -51,6 +51,15 @@ register_icall (gpointer func, const char *name, const char *sigstr, gboolean sa mono_register_jit_icall (func, name, sig, save); } +gpointer +mono_string_to_bstr(MonoString* ptr) +{ + if (!ptr) + return NULL; + + return mono_ptr_to_bstr(mono_string_chars(ptr), mono_string_length(ptr)); +} + #ifndef DISABLE_COM #define OPDEF(a,b,c,d,e,f,g,h,i,j) \ @@ -2742,37 +2751,37 @@ init_com_provider_ms (void) } gpointer -mono_string_to_bstr (MonoString *string_obj) +mono_ptr_to_bstr(gpointer ptr, int slen) { - if (!string_obj) + if (!ptr) return NULL; #ifdef HOST_WIN32 - return SysAllocStringLen (mono_string_chars (string_obj), mono_string_length (string_obj)); + return SysAllocStringLen (ptr, slen); #else if (com_provider == MONO_COM_DEFAULT) { - int slen = mono_string_length (string_obj); /* allocate len + 1 utf16 characters plus 4 byte integer for length*/ - char *ret = (char *)g_malloc ((slen + 1) * sizeof(gunichar2) + sizeof(guint32)); + char *ret = (char *)g_malloc((slen + 1) * sizeof(gunichar2) + sizeof(guint32)); if (ret == NULL) return NULL; - memcpy (ret + sizeof(guint32), mono_string_chars (string_obj), slen * sizeof(gunichar2)); - * ((guint32 *) ret) = slen * sizeof(gunichar2); - ret [4 + slen * sizeof(gunichar2)] = 0; - ret [5 + slen * sizeof(gunichar2)] = 0; + memcpy(ret + sizeof(guint32), ptr, slen * sizeof(gunichar2)); + *((guint32 *)ret) = slen * sizeof(gunichar2); + ret[4 + slen * sizeof(gunichar2)] = 0; + ret[5 + slen * sizeof(gunichar2)] = 0; return ret + 4; - } else if (com_provider == MONO_COM_MS && init_com_provider_ms ()) { + } + else if (com_provider == MONO_COM_MS && init_com_provider_ms()) { gpointer ret = NULL; gunichar* str = NULL; - guint32 len; - len = mono_string_length (string_obj); - str = g_utf16_to_ucs4 (mono_string_chars (string_obj), len, + guint32 len = slen; + str = g_utf16_to_ucs4(ptr, len, NULL, NULL, NULL); - ret = sys_alloc_string_len_ms (str, len); + ret = sys_alloc_string_len_ms(str, len); g_free(str); return ret; - } else { - g_assert_not_reached (); + } + else { + g_assert_not_reached(); } #endif } @@ -3411,20 +3420,19 @@ cominterop_release_all_rcws (void) } gpointer -mono_string_to_bstr (MonoString *string_obj) +mono_ptr_to_bstr (gpointer ptr, int slen) { - if (!string_obj) + if (!ptr) return NULL; #ifdef HOST_WIN32 - return SysAllocStringLen (mono_string_chars (string_obj), mono_string_length (string_obj)); + return SysAllocStringLen (ptr, slen); #else { - int slen = mono_string_length (string_obj); /* allocate len + 1 utf16 characters plus 4 byte integer for length*/ char *ret = g_malloc ((slen + 1) * sizeof(gunichar2) + sizeof(guint32)); if (ret == NULL) return NULL; - memcpy (ret + sizeof(guint32), mono_string_chars (string_obj), slen * sizeof(gunichar2)); + memcpy (ret + sizeof(guint32), ptr, slen * sizeof(gunichar2)); * ((guint32 *) ret) = slen * sizeof(gunichar2); ret [4 + slen * sizeof(gunichar2)] = 0; ret [5 + slen * sizeof(gunichar2)] = 0; @@ -3524,6 +3532,12 @@ ves_icall_System_Runtime_InteropServices_Marshal_StringToBSTR (MonoString* ptr) return mono_string_to_bstr(ptr); } +gpointer +ves_icall_System_Runtime_InteropServices_Marshal_BufferToBSTR (MonoArray* ptr, int len) +{ + return mono_ptr_to_bstr (ptr->vector, len); +} + void ves_icall_System_Runtime_InteropServices_Marshal_FreeBSTR (gpointer ptr) { diff --git a/mono/metadata/icall-def.h b/mono/metadata/icall-def.h index 432ddd6bea5..68903954c10 100644 --- a/mono/metadata/icall-def.h +++ b/mono/metadata/icall-def.h @@ -646,6 +646,7 @@ ICALL_TYPE(MARSHAL, "System.Runtime.InteropServices.Marshal", MARSHAL_2) #endif ICALL(MARSHAL_2, "AllocCoTaskMem", ves_icall_System_Runtime_InteropServices_Marshal_AllocCoTaskMem) ICALL(MARSHAL_3, "AllocHGlobal", ves_icall_System_Runtime_InteropServices_Marshal_AllocHGlobal) +ICALL(MARSHAL_50, "BufferToBSTR", ves_icall_System_Runtime_InteropServices_Marshal_BufferToBSTR) ICALL(MARSHAL_4, "DestroyStructure", ves_icall_System_Runtime_InteropServices_Marshal_DestroyStructure) ICALL(MARSHAL_5, "FreeBSTR", ves_icall_System_Runtime_InteropServices_Marshal_FreeBSTR) ICALL(MARSHAL_6, "FreeCoTaskMem", ves_icall_System_Runtime_InteropServices_Marshal_FreeCoTaskMem) @@ -692,6 +693,7 @@ ICALL(MARSHAL_32, "StringToHGlobalAnsi", ves_icall_System_Runtime_InteropService ICALL(MARSHAL_33, "StringToHGlobalUni", ves_icall_System_Runtime_InteropServices_Marshal_StringToHGlobalUni) ICALL(MARSHAL_34, "StructureToPtr", ves_icall_System_Runtime_InteropServices_Marshal_StructureToPtr) ICALL(MARSHAL_35, "UnsafeAddrOfPinnedArrayElement", ves_icall_System_Runtime_InteropServices_Marshal_UnsafeAddrOfPinnedArrayElement) + ICALL(MARSHAL_41, "copy_from_unmanaged", ves_icall_System_Runtime_InteropServices_Marshal_copy_from_unmanaged) ICALL(MARSHAL_42, "copy_to_unmanaged", ves_icall_System_Runtime_InteropServices_Marshal_copy_to_unmanaged) diff --git a/mono/metadata/marshal.h b/mono/metadata/marshal.h index d3c0a2b0517..58caa69d814 100644 --- a/mono/metadata/marshal.h +++ b/mono/metadata/marshal.h @@ -269,7 +269,10 @@ gpointer mono_string_to_ansibstr (MonoString *string_obj); gpointer -mono_string_to_bstr (MonoString *string_obj); +mono_ptr_to_bstr (gpointer ptr, int slen); + +gpointer +mono_string_to_bstr(MonoString* str); void mono_delegate_free_ftnptr (MonoDelegate *delegate); @@ -466,6 +469,9 @@ ves_icall_System_Runtime_InteropServices_Marshal_OffsetOf (MonoReflectionType *t gpointer ves_icall_System_Runtime_InteropServices_Marshal_StringToBSTR (MonoString *string); +gpointer +ves_icall_System_Runtime_InteropServices_Marshal_BufferToBSTR (MonoArray *ptr, int len); + gpointer ves_icall_System_Runtime_InteropServices_Marshal_StringToHGlobalAnsi (MonoString *string); -- 2.25.1