[Mono] Fix support for nested structs with explicit layout (#61467)
authorSimon Rozsival <simon@rozsival.com>
Thu, 2 Dec 2021 13:08:32 +0000 (14:08 +0100)
committerGitHub <noreply@github.com>
Thu, 2 Dec 2021 13:08:32 +0000 (14:08 +0100)
* Add test case for nested struct with explicit layout

* Add license

* Modify test

* Temporarily enable the test in the CI pipeline

* Allow running Mac Catalyst builds in App Sandbox

* Allow enabling app sandbox for the other Mac Catalyst sample

* Revert "Allow enabling app sandbox for the other Mac Catalyst sample"

This reverts commit 8c3c9f92daa47d1de844dbe897155d119415fb74.

* Revert "Allow running Mac Catalyst builds in App Sandbox"

This reverts commit 1e404165d1859bc9e25dbcdb0b4dc74a839dc708.

* Add more test cases

* WIP: First implementation of the recursive check

* Improve code

* Fix test case

* Add more test cases

* Unify test cases

* Bugfixes

* Remove test which behaves differently depending on the target platform

* Remove sequential layout test which behaves differently on Linux and on Windows

* Reorganize test cases

* Remove call to mono_class_setup_fields

* Fix embedding detection

* Fix layout validation for generic structs

* Remove unintentional change

* Revert temporary change to the runtime.yml pipeline

* Code clean-up

* Revert unrelated change

* Use getters instead of directly accessing MonoClass fields

13 files changed:
src/mono/mono/metadata/class-init.c
src/tests/Loader/classloader/explicitlayout/NestedStructs/case01.cs [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case01.csproj [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case02.cs [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case02.csproj [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case03.cs [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case03.csproj [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case04.cs [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case04.csproj [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case05.cs [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/NestedStructs/case05.csproj [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/misc/array_segment.cs [new file with mode: 0644]
src/tests/Loader/classloader/explicitlayout/misc/array_segment.csproj [new file with mode: 0644]

index 86eadc6..c53f4e5 100644 (file)
@@ -1864,6 +1864,85 @@ mono_class_is_gparam_with_nonblittable_parent (MonoClass *klass)
        return parent_class != mono_defaults.object_class;
 }
 
+/**
+ * Checks if there are any overlapping object and non-object fields.
+ * The alignment of object reference fields is checked elswhere and this function assumes
+ * that all references are aligned correctly.
+ *
+ * \param layout_check A buffer to check which bytes hold object references or values
+ * \param klass Checked struct
+ * \param field_offsets Offsets of the klass' fields relative to the start of layout_check
+ * \param field_count Count of klass fields
+ * \param invalid_field_offset When the layout is invalid it will be set to the offset of the field which is invalid
+ * 
+ * \return True if the layout of the struct is valid, otherwise false.
+ */
+static gboolean
+validate_struct_fields_overlaps (guint8 *layout_check, int layout_size, MonoClass *klass, const int *field_offsets, const int field_count, int *invalid_field_offset)
+{
+       MonoClassField *field;
+       MonoType *ftype;
+       int field_offset;
+
+       for (int i = 0; i < field_count && !mono_class_has_failure (klass); i++) {
+               // using mono_class_get_fields_internal isn't appropriate here because it will
+               // try to call mono_class_setup_fields which is what we're doing already
+               field = &m_class_get_fields (klass) [i];
+               field_offset = field_offsets [i];
+
+               if (!field)
+                       continue;
+               if (mono_field_is_deleted (field))
+                       continue;
+               if (field->type->attrs & FIELD_ATTRIBUTE_STATIC)
+                       continue;
+
+               ftype = mono_type_get_underlying_type (field->type);
+               ftype = mono_type_get_basic_type_from_generic (ftype);
+
+               if (mono_type_is_struct (ftype)) {
+                       // recursively check the layout of the embedded struct
+                       MonoClass *embedded_class = mono_class_from_mono_type_internal (ftype);
+                       g_assert (m_class_is_fields_inited (embedded_class));
+
+                       const int embedded_fields_count = mono_class_get_field_count (embedded_class);
+                       int *embedded_offsets = g_new0 (int, embedded_fields_count);
+                       for (int j = 0; j < embedded_fields_count; ++j) {
+                               embedded_offsets [j] = field_offset + m_class_get_fields (embedded_class) [j].offset - MONO_ABI_SIZEOF (MonoObject);
+                       }
+
+                       gboolean is_valid = validate_struct_fields_overlaps (layout_check, layout_size, embedded_class, embedded_offsets, embedded_fields_count, invalid_field_offset);
+                       g_free (embedded_offsets);
+
+                       if (!is_valid) {
+                               // overwrite whatever was in the invalid_field_offset with the offset of the currently checked field
+                               // we want to return the outer most invalid field
+                               *invalid_field_offset = field_offset;
+                               return FALSE;
+                       }
+               } else {
+                       int align = 0;
+                       int size = mono_type_size (field->type, &align);
+                       guint8 type = type_has_references (klass, ftype) ? 1 : 2;
+
+                       // Mark the bytes used by this fields type based on if it contains references or not.
+                       // Make sure there are no overlaps between object and non-object fields.
+                       for (int j = 0; j < size; j++) {
+                               int checked_byte = field_offset + j;
+                               g_assert(checked_byte < layout_size);
+
+                               if (layout_check [checked_byte] != 0 && layout_check [checked_byte] != type) {
+                                       *invalid_field_offset = field_offset;
+                                       return FALSE;
+                               }
+                               layout_check [checked_byte] = type;
+                       }
+               }
+       }
+
+       return TRUE;
+}
+
 /*
  * mono_class_layout_fields:
  * @class: a class
@@ -2154,29 +2233,12 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_
                }
 
                /* check for incorrectly aligned or overlapped by a non-object field */
-               guint8 *layout_check;   
+               guint8 *layout_check;
                if (has_references) {
                        layout_check = g_new0 (guint8, real_size);
-                       for (i = 0; i < top && !mono_class_has_failure (klass); i++) {
-                               field = &klass->fields [i];
-                               if (!field)
-                                       continue;
-                               if (mono_field_is_deleted (field))
-                                       continue;
-                               if (field->type->attrs & FIELD_ATTRIBUTE_STATIC)
-                                       continue;
-                               int align = 0;
-                               int size = mono_type_size (field->type, &align);
-                               MonoType *ftype = mono_type_get_underlying_type (field->type);
-                               ftype = mono_type_get_basic_type_from_generic (ftype);
-                               guint8 type =  type_has_references (klass, ftype) ? 1 : 2;                              
-                               for (int j = 0; j < size; j++) {
-                                       if (layout_check [field_offsets [i] + j] != 0 && layout_check [field_offsets [i] + j] != type) {
-                                               mono_class_set_type_load_failure (klass, "Could not load type '%s' because it contains an object field at offset %d that is incorrectly aligned or overlapped by a non-object field.", klass->name, field->offset);
-                                               break;
-                                       }
-                                       layout_check [field_offsets [i] + j] = type;
-                               }
+                       int invalid_field_offset;
+                       if (!validate_struct_fields_overlaps (layout_check, real_size, klass, field_offsets, top, &invalid_field_offset)) {
+                               mono_class_set_type_load_failure (klass, "Could not load type '%s' because it contains an object field at offset %d that is incorrectly aligned or overlapped by a non-object field.", klass->name, invalid_field_offset);
                        }
                        g_free (layout_check);
                }
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case01.cs b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case01.cs
new file mode 100644 (file)
index 0000000..eb86c19
--- /dev/null
@@ -0,0 +1,74 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.InteropServices;
+
+[StructLayout(LayoutKind.Explicit, Size = 24)]
+public struct ComplexStruct
+{
+    [FieldOffset(0)]
+    public object? Object;
+
+    [FieldOffset(0)]
+    public InnerStruct Inner;
+
+    [FieldOffset(8)]
+    public double Double;
+
+    [FieldOffset(8)]
+    public ulong High;
+
+    [FieldOffset(16)]
+    public ulong Low;
+}
+
+[StructLayout(LayoutKind.Explicit, Size = 16)]
+public struct InnerStruct
+{
+    [FieldOffset(0)]
+    public object? Object;
+
+    [FieldOffset(8)]
+    public int High;
+
+    [FieldOffset(12)]
+    public int Low;
+}
+
+public class Test_NestedStructsWithExplicitLayout_Case01 {
+    private ComplexStruct currentCount = default;
+
+    private void IncrementCount()
+    {
+        var x = new ComplexStruct();
+        x.Inner.High = currentCount.Inner.High + 1;
+        currentCount = x;
+    }
+
+    public static int Main ()
+    {
+        try
+        {
+            var instance = new Test_NestedStructsWithExplicitLayout_Case01();
+            instance.IncrementCount();
+            var result = 99 + instance.currentCount.Inner.High;
+
+            if (result == 100)
+            {
+                Console.WriteLine("PASS: union of Explict + Explicit works correctly");
+            }
+
+            return result;
+        }
+        catch (TypeLoadException e)
+        {
+            Console.WriteLine("FAIL: type was not loaded");
+            return 101;
+        }
+        catch (Exception e)
+        {
+            return 102;
+        }
+    }
+}
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case01.csproj b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case01.csproj
new file mode 100644 (file)
index 0000000..1c3478d
--- /dev/null
@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="case01.cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case02.cs b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case02.cs
new file mode 100644 (file)
index 0000000..0f9926d
--- /dev/null
@@ -0,0 +1,89 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.InteropServices;
+
+[StructLayout(LayoutKind.Explicit, Size = 16)]
+public struct FirstLevel
+{
+    [FieldOffset(0)]
+    public object? Object;
+
+    [FieldOffset(0)]
+    public SecondLevel SecondLevel;
+
+    [FieldOffset(8)]
+    public int High;
+
+    [FieldOffset(12)]
+    public int Low;
+}
+
+[StructLayout(LayoutKind.Explicit, Size = 16)]
+public struct SecondLevel
+{
+    [FieldOffset(0)]
+    public object? Object;
+
+    [FieldOffset(0)]
+    public ThirdLevel ThirdLevel;
+
+    [FieldOffset(8)]
+    public int High;
+
+    [FieldOffset(12)]
+    public int Low;
+}
+
+[StructLayout(LayoutKind.Explicit, Size = 16)]
+public struct ThirdLevel
+{
+    [FieldOffset(0)]
+    public object? Object;
+
+    [FieldOffset(8)]
+    public int High;
+
+    [FieldOffset(12)]
+    public int Low;
+}
+
+public class Test_NestedStructsWithExplicitLayout_Case02 {
+    private int Run(int value)
+    {
+        var x = new FirstLevel();
+        x.Low = value;
+        return x.SecondLevel.ThirdLevel.Low;
+    }
+
+    public static int Main ()
+    {
+        try
+        {
+            var expectedResult = 13;
+
+            var testInstance = new Test_NestedStructsWithExplicitLayout_Case02();
+            var result = testInstance.Run(expectedResult);
+
+            if (result == expectedResult)
+            {
+                Console.WriteLine("PASS: types were loaded correctly");
+                return 100;
+            }
+
+            Console.WriteLine("FAIL: invalid value");
+            return 103;
+        }
+        catch (TypeLoadException e)
+        {
+            Console.WriteLine("FAIL: type was not loaded correctly");
+            return 101;
+        }
+        catch (Exception e)
+        {
+            Console.WriteLine("FAIL: unknown error");
+            return 102;
+        }
+    }
+}
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case02.csproj b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case02.csproj
new file mode 100644 (file)
index 0000000..7ab33ec
--- /dev/null
@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="case02.cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case03.cs b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case03.cs
new file mode 100644 (file)
index 0000000..57fd85b
--- /dev/null
@@ -0,0 +1,62 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.InteropServices;
+
+[StructLayout(LayoutKind.Explicit)]
+public struct FirstLevel
+{
+    [FieldOffset(0)]
+    public object? ConflictingObjectField;
+
+    [FieldOffset(0)]
+    public SecondLevel SecondLevel;
+}
+
+[StructLayout(LayoutKind.Explicit)]
+public struct SecondLevel
+{
+    [FieldOffset(0)]
+    public ThirdLevel ThirdLevel;
+
+    [FieldOffset(8)]
+    public long Value;
+}
+
+[StructLayout(LayoutKind.Explicit)]
+public struct ThirdLevel
+{
+    [FieldOffset(0)]
+    public long ConflictingValueTypeField;
+}
+
+public class Test_NestedStructsWithExplicitLayout_Case06 {
+    private void Run()
+    {
+        var x = new FirstLevel();
+        x.ConflictingObjectField = new object();
+    }
+
+    public static int Main ()
+    {
+        try
+        {
+            var test = new Test_NestedStructsWithExplicitLayout_Case06();
+            test.Run();
+        }
+        catch (TypeLoadException e)
+        {
+            Console.WriteLine("PASS: object and non-object field overlap was detected");
+            return 100;
+        }
+        catch (Exception e)
+        {
+            Console.WriteLine("FAIL: unexpected exception type");
+            return 102;
+        }
+
+        Console.WriteLine("FAIL: object and non-object field overlap was not detected");
+        return 101;
+    }
+}
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case03.csproj b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case03.csproj
new file mode 100644 (file)
index 0000000..7308df2
--- /dev/null
@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="case03.cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case04.cs b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case04.cs
new file mode 100644 (file)
index 0000000..7e2cfc4
--- /dev/null
@@ -0,0 +1,62 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.InteropServices;
+
+[StructLayout(LayoutKind.Explicit)]
+public struct FirstLevel
+{
+    [FieldOffset(0)]
+    public object? ConflictingObjectField;
+
+    [FieldOffset(0)]
+    public SecondLevel SecondLevel;
+}
+
+[StructLayout(LayoutKind.Explicit)]
+public struct SecondLevel
+{
+    [FieldOffset(0)]
+    public ThirdLevel ThirdLevel;
+
+    [FieldOffset(8)]
+    public long Value;
+}
+
+[StructLayout(LayoutKind.Explicit)]
+public struct ThirdLevel
+{
+    [FieldOffset(6)]
+    public short ConflictingValueTypeField;
+}
+
+public class Test_NestedStructsWithExplicitLayout_Case07 {
+    private void Run()
+    {
+        var x = new FirstLevel();
+        x.ConflictingObjectField = new object();
+    }
+
+    public static int Main ()
+    {
+        try
+        {
+            var test = new Test_NestedStructsWithExplicitLayout_Case07();
+            test.Run();
+        }
+        catch (TypeLoadException e)
+        {
+            Console.WriteLine("PASS: object and non-object field overlap was detected");
+            return 100;
+        }
+        catch (Exception e)
+        {
+            Console.WriteLine("FAIL: unexpected exception type");
+            return 102;
+        }
+
+        Console.WriteLine("FAIL: object and non-object field overlap was not detected");
+        return 101;
+    }
+}
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case04.csproj b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case04.csproj
new file mode 100644 (file)
index 0000000..6b6ec60
--- /dev/null
@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="case04.cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case05.cs b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case05.cs
new file mode 100644 (file)
index 0000000..9a4d8de
--- /dev/null
@@ -0,0 +1,52 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.InteropServices;
+
+[StructLayout(LayoutKind.Explicit)]
+public struct FirstLevel
+{
+    [FieldOffset(0)]
+    public object? ConflictingObjectField;
+
+    [FieldOffset(0)]
+    public SecondLevel SecondLevel;
+}
+
+[StructLayout(LayoutKind.Explicit)]
+public struct SecondLevel
+{
+    [FieldOffset(0)]
+    public long ConflictingValueTypeField;
+}
+
+public class Test_NestedStructsWithExplicitLayout_Case05 {
+    private void Run()
+    {
+        var x = new FirstLevel();
+        x.ConflictingObjectField = new object();
+    }
+
+    public static int Main ()
+    {
+        try
+        {
+            var test = new Test_NestedStructsWithExplicitLayout_Case05();
+            test.Run();
+        }
+        catch (TypeLoadException e)
+        {
+            Console.WriteLine("PASS: object and non-object field overlap was detected");
+            return 100;
+        }
+        catch (Exception e)
+        {
+            Console.WriteLine("FAIL: unexpected exception type");
+            return 102;
+        }
+
+        Console.WriteLine("FAIL: object and non-object field overlap was not detected");
+        return 101;
+    }
+}
diff --git a/src/tests/Loader/classloader/explicitlayout/NestedStructs/case05.csproj b/src/tests/Loader/classloader/explicitlayout/NestedStructs/case05.csproj
new file mode 100644 (file)
index 0000000..e843149
--- /dev/null
@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="case05.cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/Loader/classloader/explicitlayout/misc/array_segment.cs b/src/tests/Loader/classloader/explicitlayout/misc/array_segment.cs
new file mode 100644 (file)
index 0000000..16efed5
--- /dev/null
@@ -0,0 +1,57 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.InteropServices;
+
+// Structs come from from System.Net.Sockets tests
+internal struct FakeArraySegment
+{
+       public byte[] Array;
+       public int Offset;
+       public int Count;
+
+       public ArraySegment<byte> ToActual()
+       {
+               ArraySegmentWrapper wrapper = default(ArraySegmentWrapper);
+               wrapper.Fake = this;
+               return wrapper.Actual;
+       }
+}
+
+[StructLayout(LayoutKind.Explicit)]
+internal struct ArraySegmentWrapper
+{
+       [FieldOffset(0)] public ArraySegment<byte> Actual;
+       [FieldOffset(0)] public FakeArraySegment Fake;
+}
+
+public class Test_ExplicitLayoutWithArraySegment
+{
+       private void Run()
+    {
+               var fakeArraySegment = new FakeArraySegment() { Array = new byte[10], Offset = 0, Count = 10 };
+               ArraySegment<byte> internalBuffer = fakeArraySegment.ToActual();
+    }
+
+       public static int Main()
+       {
+               try
+               {
+                       var testInstance = new Test_ExplicitLayoutWithArraySegment();
+                       testInstance.Run();
+               }
+               catch (TypeLoadException e)
+               {
+                       Console.WriteLine("FAIL: Caught TypeLoadException: " + e.Message);
+                       return 101;
+               }
+               catch (Exception e)
+               {
+                       Console.WriteLine("FAIL: Caught unexpected exception: " + e.Message);
+                       return 101;
+               }
+
+               return 100;
+       }
+}
diff --git a/src/tests/Loader/classloader/explicitlayout/misc/array_segment.csproj b/src/tests/Loader/classloader/explicitlayout/misc/array_segment.csproj
new file mode 100644 (file)
index 0000000..451b532
--- /dev/null
@@ -0,0 +1,8 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="array_segment.cs" />
+  </ItemGroup>
+</Project>