[loader] Init MonoClass:sizes.element_size lazily (Fixes #43563) (#5559)

* [loader] If signaling recursive type initialize, don't double free

If we detect that a recursive type initialization is happening, we should leave
without removing the class from the init_pending_tld_id list.  Otherwise we get
a double-free when the outer recursive call finally finishes and tries to
remove the same element.

Also re-load the init_list from TLS since a recursive call to mono_class_init could
have modified it between the time the current mono_class_init call started and
when it is finishing.

* [loader] Improve error message when a field's type is a failed class

Add the class failure to the field failure error message.

* [tests] Check that runtime can represent recursive structs

Regression tests for https://bugzilla.xamarin.com/show_bug.cgi?id=43563

* [loader] Init MonoClass:sizes.element_size lazily (Fixes #43563)

Originally mono_bounded_array_class_get () would populate
MonoClass:sizes.element_size as soon as the MonoClass for the array was needed.
This is a problem because we cannot call mono_class_array_element_size () on
the element class on a valuetype until mono_class_layout_fields () finishes
initializing it.

That means that structs which contain an array of themselves would hit the
recursive initialization check in mono_class_setup_fields and the MonoClass for
the element class would be marked as failed to load.

Instead we rely on the MonoClass:size_inited bit to tell us whether the array
class has been initialized and if not, we set the array element size in
mono_class_layout_fields.

This is possible because MonoClass:sizes.element_size is only really needed to
know how to allocate space for an array.  When laying out a class that contains
an array we don't need the element size - they array is just a reference type.

Example:
```csharp
struct S {
    static S[][] foo;
}
```

Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=43563

diff --git a/mono/metadata/class.c b/mono/metadata/class.c
index 8df8cb531d4a88faed3a3591ff7afbd5a3a88453..f00d436281a5d0d1a05075f56870a49d37183875 100644 (file)
--- a/mono/metadata/class.c
+++ b/mono/metadata/class.c
@@ -1513,6 +1513,7 @@ mono_class_set_type_load_failure_causedby_class (MonoClass *klass, const MonoCla
  * Sets the following fields in \p klass:
  *  - all the fields initialized by mono_class_init_sizes ()
  *  - element_class/cast_class (for enums)
+ *  - sizes:element_size (for arrays)
  *  - field->type/offset for all fields
  *  - fields_inited
  *
@@ -1772,6 +1773,7 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_
        MonoClassField *field;
        gboolean blittable;
        int instance_size = base_instance_size;
+       int element_size = -1;
        int class_size, min_align;
        int *field_offsets;
        gboolean *fields_has_references;
@@ -2101,6 +2103,9 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_
        else if (klass->byval_arg.type == MONO_TYPE_PTR)
                instance_size = sizeof (MonoObject) + sizeof (gpointer);
 
+       if (klass->byval_arg.type == MONO_TYPE_SZARRAY || klass->byval_arg.type == MONO_TYPE_ARRAY)
+               element_size = mono_class_array_element_size (klass->element_class);
+
        /* Publish the data */
        mono_loader_lock ();
        if (klass->instance_size && !klass->image->dynamic) {
@@ -2131,6 +2136,9 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_
                        klass->fields [i].offset = field_offsets [i];
        }
 
+       if (klass->byval_arg.type == MONO_TYPE_SZARRAY || klass->byval_arg.type == MONO_TYPE_ARRAY)
+               klass->sizes.element_size = element_size;
+
        mono_memory_barrier ();
        klass->size_inited = 1;
        mono_loader_unlock ();
@@ -4880,7 +4888,7 @@ mono_class_init (MonoClass *klass)
        GSList *init_list = (GSList *)mono_native_tls_get_value (init_pending_tls_id);
        if (g_slist_find (init_list, klass)) {
                mono_class_set_type_load_failure (klass, "Recursive type definition detected");
-               goto leave;
+               goto leave_no_init_pending;
        }
        init_list = g_slist_prepend (init_list, klass);
        mono_native_tls_set_value (init_pending_tls_id, init_list);
@@ -5079,10 +5087,12 @@ mono_class_init (MonoClass *klass)
 
        goto leave;
 
- leave:
+leave:
+       init_list = mono_native_tls_get_value (init_pending_tls_id);
        init_list = g_slist_remove (init_list, klass);
        mono_native_tls_set_value (init_pending_tls_id, init_list);
 
+leave_no_init_pending:
        if (locked)
                mono_loader_unlock ();
 
@@ -6596,7 +6606,7 @@ mono_bounded_array_class_get (MonoClass *eclass, guint32 rank, gboolean bounded)
                /* element_size -1 is ok as this is not an instantitable type*/
                klass->sizes.element_size = -1;
        } else
-               klass->sizes.element_size = mono_class_array_element_size (eclass);
+               klass->sizes.element_size = -1;
 
        mono_class_setup_supertypes (klass);
 
@@ -8634,11 +8644,16 @@ handle_enum:
  * \param ac pointer to a \c MonoArrayClass
  *
  * \returns The size of single array element.
+ *
+ * LOCKING: Acquires the loader lock.
  */
 gint32
 mono_array_element_size (MonoClass *ac)
 {
        g_assert (ac->rank);
+       if (G_UNLIKELY (!ac->size_inited)) {
+               mono_class_setup_fields (ac);
+       }
        return ac->sizes.element_size;
 }
 

diff --git a/mono/metadata/loader.c b/mono/metadata/loader.c
index ea3a441b76bc897cc819a2ed25edfbf5d9f05732..5281a5d39b882e00e1a16c5dc39c23ce1aa22bf8 100644 (file)
--- a/mono/metadata/loader.c
+++ b/mono/metadata/loader.c
@@ -314,9 +314,17 @@ mono_field_from_token_checked (MonoImage *image, guint32 token, MonoClass **retk
                mono_class_init (k);
                if (retklass)
                        *retklass = k;
-               field = mono_class_get_field (k, token);
-               if (!field) {
-                       mono_error_set_bad_image (error, image, "Could not resolve field token 0x%08x", token);
+               if (mono_class_has_failure (k)) {
+                       MonoError causedby_error;
+                       error_init (&causedby_error);
+                       mono_error_set_for_class_failure (&causedby_error, k);
+                       mono_error_set_bad_image (error, image, "Could not resolve field token 0x%08x, due to: %s", token, mono_error_get_message (&causedby_error));
+                       mono_error_cleanup (&causedby_error);
+               } else {
+                       field = mono_class_get_field (k, token);
+                       if (!field) {
+                               mono_error_set_bad_image (error, image, "Could not resolve field token 0x%08x", token);
+                       }
                }
        }
 

diff --git a/mono/metadata/sgen-mono.c b/mono/metadata/sgen-mono.c
index 4cd30493fff80524e3b60b368623d7e2d35563c7..ac977c0e5902e4133b05074a7edab4b20680a20a 100644 (file)
--- a/mono/metadata/sgen-mono.c
+++ b/mono/metadata/sgen-mono.c
@@ -398,6 +398,7 @@ get_array_fill_vtable (void)
                klass.rank = 1;
                klass.instance_size = MONO_SIZEOF_MONO_ARRAY;
                klass.sizes.element_size = 1;
+               klass.size_inited = 1;
                klass.name = "array_filler_type";
 
                vtable->klass = &klass;

diff --git a/mono/tests/Makefile.am b/mono/tests/Makefile.am
index a2e3287a244d35b96861038f826490ed2c75d48b..49e221628844a5486c061e4830cd4b740eedcc95 100755 (executable)
--- a/mono/tests/Makefile.am
+++ b/mono/tests/Makefile.am
@@ -517,7 +517,8 @@ TESTS_CS_SRC=               \
        runtime-invoke.gen.cs \
        imt_big_iface_test.cs \
        bug-58782-plain-throw.cs \
-       bug-58782-capture-and-throw.cs
+       bug-58782-capture-and-throw.cs \
+       recursive-struct-arrays.cs
 
 if AMD64
 TESTS_CS_SRC += async-exc-compilation.cs finally_guard.cs finally_block_ending_in_dead_bb.cs

diff --git a/mono/tests/recursive-struct-arrays.cs b/mono/tests/recursive-struct-arrays.cs
new file mode 100644 (file)
index 0000000..dd1c8b2
--- /dev/null
+++ b/mono/tests/recursive-struct-arrays.cs
@@ -0,0 +1,65 @@
+using System;
+
+/* Test that the runtime can represent value types that have array fields that
+ * recursively refer to the same value type */
+
+struct S1 {
+       static S1[][] foo;
+}
+
+struct S2 {
+       static S2[] foo;
+}
+
+struct S3a {
+       static S3b[] foo;
+}
+
+struct S3b {
+       static S3a[][] foo;
+}
+
+struct P<X> where X : struct {
+       static P<X>[][] foo;
+}
+
+public struct S4
+{
+       private static S4[][] foo;
+
+       public static readonly S4 West = new S4(-1, 0);
+       public static readonly S4 East = new S4(1, 0);
+       public static readonly S4 North = new S4(0, 1);
+       public static readonly S4 South = new S4(0, -1);
+       public static readonly S4[] Directions = { North, South, East, West };
+
+       public readonly int x;
+       public readonly int z;
+
+       public S4(int x, int z)
+       {
+               this.x = x;
+               this.z = z;
+       }
+
+       public override string ToString()
+       {
+               return string.Format("[{0}, {1}]", x, z);
+       }
+}
+
+
+class Program {
+       static int Main() {
+               Console.WriteLine (typeof (S1).Name);
+               Console.WriteLine (typeof (S2).Name);
+               Console.WriteLine (typeof (S3a).Name);
+               Console.WriteLine (typeof (S3b).Name);
+               foreach (var s4 in S4.Directions) {
+                       Console.WriteLine (s4);
+               }
+               Console.WriteLine (typeof (P<S1>).Name);
+               Console.WriteLine (typeof (P<int>).Name);
+               return 0;
+       }
+}