[2020-02][marshal] Fix VARIANT and BSTR marshaling in structs (#20918)mono-6.12.0.130

* [tests] Add BStr and VARIANT in structs interop tests

* [marshal] Fix reverse P/Invoke marshaling VARIANT fields

Partial revert of https://github.com/mono/mono/pull/8732

If a managed struct is declared as

        public struct StructWithVariant
        {
            [MarshalAs (UnmanagedType.Struct)]
            public object data;
        };

Then the `data` field should be marshalled as a VARIANT.

The fix checks that the field's type is `object` and uses VARIANT for
marshaling, otherwise it continues as in mono/mono#8732 and uses
MONO_MARSHAL_CONV_OBJECT_STRUCT

* [marshal] Marshal BSTR from reverse pinvokes

4 files changed, 129 insertions, 4 deletions

diff --git a/mono/metadata/marshal-ilgen.c b/mono/metadata/marshal-ilgen.c
index 5472727ef0f..de6fefed06e 100644
--- a/mono/metadata/marshal-ilgen.c
+++ b/mono/metadata/marshal-ilgen.c
@@ -488,6 +488,13 @@ emit_ptr_to_object_conv (MonoMethodBuilder *mb, MonoType *type, MonoMarshalConv
 		mono_mb_emit_icall (mb, ves_icall_mono_string_from_utf16);
 		mono_mb_emit_byte (mb, CEE_STIND_REF);
 		break;
+	case MONO_MARSHAL_CONV_STR_BSTR:
+		mono_mb_emit_ldloc (mb, 1);
+		mono_mb_emit_ldloc (mb, 0);
+		mono_mb_emit_byte (mb, CEE_LDIND_I);
+		mono_mb_emit_icall (mb, mono_string_from_bstr_icall);
+		mono_mb_emit_byte (mb, CEE_STIND_REF);
+		break;
 	case MONO_MARSHAL_CONV_OBJECT_STRUCT: {
 		MonoClass *klass = mono_class_from_mono_type_internal (type);
 		int src_var, dst_var;
@@ -576,7 +583,6 @@ emit_ptr_to_object_conv (MonoMethodBuilder *mb, MonoType *type, MonoMarshalConv
 		break;
 	}
 		
-	case MONO_MARSHAL_CONV_STR_BSTR:
 	case MONO_MARSHAL_CONV_STR_ANSIBSTR:
 	case MONO_MARSHAL_CONV_STR_TBSTR:
 	case MONO_MARSHAL_CONV_ARRAY_SAVEARRAY:
diff --git a/mono/metadata/metadata.c b/mono/metadata/metadata.c
index 5f6568da9b0..b2987016ef2 100644
--- a/mono/metadata/metadata.c
+++ b/mono/metadata/metadata.c
@@ -6781,7 +6781,17 @@ handle_enum:
 		if (mspec) {
 			switch (mspec->native) {
 			case MONO_NATIVE_STRUCT:
-				*conv = MONO_MARSHAL_CONV_OBJECT_STRUCT;
+				// [MarshalAs(UnmanagedType.Struct)]
+				// object field;
+				//
+				// becomes a VARIANT
+				//
+				// [MarshalAs(UnmangedType.Struct)]
+				// SomeClass field;
+				//
+				// becomes uses the CONV_OBJECT_STRUCT conversion
+				if (t != MONO_TYPE_OBJECT)
+					*conv = MONO_MARSHAL_CONV_OBJECT_STRUCT;
 				return MONO_NATIVE_STRUCT;
 			case MONO_NATIVE_CUSTOM:
 				return MONO_NATIVE_CUSTOM;
diff --git a/mono/tests/cominterop.cs b/mono/tests/cominterop.cs
index e83cafd0625..11cb68bcdbc 100644
--- a/mono/tests/cominterop.cs
+++ b/mono/tests/cominterop.cs
@@ -224,7 +224,21 @@ public class Tests
 	[DllImport ("libtest")]
 	public static extern int mono_test_marshal_variant_out_bool_false_unmanaged (VarRefFunc func);
 
-    [DllImport ("libtest")]
+	public delegate int CheckStructWithVariantFunc ([MarshalAs (UnmanagedType.Struct)] StructWithVariant obj);
+	[DllImport ("libtest")]
+	public static extern int mono_test_marshal_struct_with_variant_in_unmanaged (CheckStructWithVariantFunc func);
+
+	public delegate int CheckStructWithBstrFunc ([MarshalAs (UnmanagedType.Struct)] StructWithBstr obj);
+	[DllImport ("libtest")]
+	public static extern int mono_test_marshal_struct_with_bstr_in_unmanaged (CheckStructWithBstrFunc func);
+
+	[DllImport ("libtest")]
+	public static extern int mono_test_marshal_struct_with_variant_out_unmanaged ([MarshalAs (UnmanagedType.Struct)] StructWithVariant obj);
+
+	[DllImport ("libtest")]
+	public static extern int mono_test_marshal_struct_with_bstr_out_unmanaged ([MarshalAs (UnmanagedType.Struct)] StructWithBstr obj);
+
+	[DllImport ("libtest")]
 	public static extern int mono_test_marshal_com_object_create (out IntPtr pUnk);
 
 	[DllImport ("libtest")]
@@ -338,7 +352,6 @@ public class Tests
 
 	public static int Main ()
 	{
-
 		bool isWindows = !(((int)Environment.OSVersion.Platform == 4) ||
 			((int)Environment.OSVersion.Platform == 128));
 
@@ -356,6 +369,15 @@ public class Tests
 			if (mono_test_marshal_bstr_out_null (out str) != 0 || str != null)
 				return 2;
 
+			var sbfunc = new CheckStructWithBstrFunc(mono_test_marshal_struct_with_bstr_callback);
+			if (mono_test_marshal_struct_with_bstr_in_unmanaged(sbfunc) != 0)
+				return 3;
+
+			StructWithBstr swithB;
+			swithB.data = "this is a test string";
+			if (mono_test_marshal_struct_with_bstr_out_unmanaged (swithB) != 0)
+				return 4;
+
 			#endregion // BSTR Tests
 
 			#region VARIANT Tests
@@ -481,6 +503,15 @@ public class Tests
 			if (mono_test_marshal_variant_out_bstr_byref (out obj) != 0 || (string)obj != "PI")
 				return 107;
 
+			var svfunc = new CheckStructWithVariantFunc(mono_test_marshal_struct_with_variant_callback);
+			if (mono_test_marshal_struct_with_variant_in_unmanaged(svfunc) != 0)
+				return 108;
+
+			StructWithVariant swithV;
+			swithV.data = (object)-123;
+			if (mono_test_marshal_struct_with_variant_out_unmanaged (swithV) != 0)
+				return 109;
+
 			#endregion // VARIANT Tests
 
 			#region Runtime Callable Wrapper Tests
@@ -1581,8 +1612,38 @@ public class Tests
 	public static int TestIfaceNoIcall (ITestPresSig itest) {
 		return itest.Return22NoICall () == 22 ? 0 : 1;
 	}
+
+        public static int mono_test_marshal_struct_with_variant_callback(StructWithVariant sv)
+        {
+            if (sv.data.GetType() != typeof(int))
+                return 1;
+            if ((int)sv.data != -123)
+                return 2;
+            return 0;
+        }
+
+        public static int mono_test_marshal_struct_with_bstr_callback(StructWithBstr sb)
+        {
+            if (sb.data.GetType() != typeof(string))
+                return 1;
+            if ((string)sb.data != "this is a test string")
+                return 2;
+            return 0;
+        }
 }
 
 public class TestVisible
 {
 }
+
+public struct StructWithVariant
+{
+	[MarshalAs (UnmanagedType.Struct)]
+	public object data;
+}
+
+public struct StructWithBstr
+{
+	[MarshalAs (UnmanagedType.BStr)]
+	public string data;
+}
diff --git a/mono/tests/libtest.c b/mono/tests/libtest.c
index 3f14636bb25..e5e5f2f14d3 100644
--- a/mono/tests/libtest.c
+++ b/mono/tests/libtest.c
@@ -3378,6 +3378,54 @@ mono_test_marshal_variant_out_bool_false_unmanaged(VarRefFunc func)
 	return 1;
 }
 
+typedef struct _StructWithVariant {
+    VARIANT data;
+} StructWithVariant;
+typedef int (STDCALL *CheckStructWithVariantFunc) (StructWithVariant sv);
+
+LIBTEST_API int STDCALL 
+mono_test_marshal_struct_with_variant_in_unmanaged(CheckStructWithVariantFunc func)
+{
+    StructWithVariant sv;
+    sv.data.vt = VT_I4;
+    sv.data.lVal = -123;
+    return func(sv);
+}
+
+LIBTEST_API int STDCALL
+mono_test_marshal_struct_with_variant_out_unmanaged (StructWithVariant sv)
+{
+	if (sv.data.vt != VT_I4)
+		return 1;
+	if (sv.data.lVal != -123)
+		return 2;
+	return 0;
+}
+
+typedef struct _StructWithBstr {
+    gunichar2* data;
+} StructWithBstr;
+typedef int (STDCALL *CheckStructWithBstrFunc) (StructWithBstr sb);
+
+LIBTEST_API int STDCALL 
+mono_test_marshal_struct_with_bstr_in_unmanaged(CheckStructWithBstrFunc func)
+{
+    StructWithBstr sb;
+    sb.data = marshal_bstr_alloc("this is a test string");
+    return func(sb);
+}
+
+LIBTEST_API int STDCALL
+mono_test_marshal_struct_with_bstr_out_unmanaged (StructWithBstr sb)
+{
+	char *s = g_utf16_to_utf8 (sb.data, g_utf16_len (sb.data), NULL, NULL, NULL);
+	gboolean same = !strcmp (s, "this is a test string");
+	g_free (s);
+	if (!same)
+		return 1;
+	return 0;
+}
+
 typedef struct MonoComObject MonoComObject;
 typedef struct MonoDefItfObject MonoDefItfObject;