Skip to content

Commit

Permalink
[mono] When creating an array type, don't do full initialization of t…
Browse files Browse the repository at this point in the history
…he element type (#85828)

Consider code like this:

```csharp
public class Node<T>
{
    public Node<T>[][] Children;
}


Console.WriteLine (typeof(Node<int>));
```

In this case, when we're JITing ``ldtoken class Node`1<int32>`` we first have to parse the type ``Node`1<int32>`` and initialize it.
When we're initializing it, we need to resolve the types of all the fields in the class in order to to figure out if its instance size.

To resolve the field types we have to parse the types of all the fields, and we eventually end up parsing the type ``Node`1<!0>[][]`` which ends up here:

https://github.com/dotnet/runtime/blob/558345d16cf76525d0c7fdbafbfd3a2457142b39/src/mono/mono/metadata/metadata.c#L4023-L4027

When we get to line 4027 the second time (recursively), `etype` is ``Node`1<!0>`` as a `MonoType*`.  And we want to set `type->data.klass` to its corresponding `MonoClass*`.   That ends up calling `mono_class_from_mono_type_internal`, which does this:

https://github.com/dotnet/runtime/blob/558345d16cf76525d0c7fdbafbfd3a2457142b39/src/mono/mono/metadata/class.c#L2214-L2215

When we call `mono_class_create_array` we end up here:

https://github.com/dotnet/runtime/blob/558345d16cf76525d0c7fdbafbfd3a2457142b39/src/mono/mono/metadata/class-init.c#L1186-L1187

with `eclass` equal to ``Node`1<!0>` which we try to initialize and we get a TLE because we're going to try to initialize the same generic type definition ``Node`1`` twice.

Compare with this other class:

```csharp
public unsafe class Node2<T>
{
    public Node2<T>[] Children;
}
```

In this case we only end up calling `mono_class_from_mono_type_internal` on ``Node2`1<int32>`` (not an array).  And that branch does not do any initialization - it just returns `type->data.klass` (for ``Node2`1<!0>`` - which is seen as a gtd at this point) or `mono_class_create_generic_inst (type->data.generic_class)` (for ``Node2`1<int32>`` which is seen later when `ldtoken` inflates the gtd)) - without any extra initialization.


---

It seems the reason we get into trouble is because `mono_class_create_array` does more work than other `MonoClass` creation functions - it tries to initialize the `MonoClass` a little bit (for example by setting `MonoClass:has_references`) which needs at least a little bit of the element class to be initialized.

But note that the code has this:

https://github.com/dotnet/runtime/blob/558345d16cf76525d0c7fdbafbfd3a2457142b39/src/mono/mono/metadata/class-init.c#L1186-L1189

I feel fairly certain we don't need the full class initialization if we're going to immediately initialize the field size information.

Fixes #85821
  • Loading branch information
lambdageek authored May 22, 2023
1 parent ed33e6c commit bdf5df3
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/mono/mono/metadata/class-init.c
Original file line number Diff line number Diff line change
Expand Up @@ -1183,8 +1183,10 @@ mono_class_create_bounded_array (MonoClass *eclass, guint32 rank, gboolean bound

mono_class_setup_supertypes (klass);

if (mono_class_is_ginst (eclass))
mono_class_init_internal (eclass);
// NOTE: this is probably too aggressive if eclass is not a valuetype. It looks like we
// only need the size info in order to set MonoClass:has_references for this array type -
// and for that we only need to setup the fields of the element type if it's not a reference
// type.
if (!eclass->size_inited)
mono_class_setup_fields (eclass);
mono_class_set_type_load_failure_causedby_class (klass, eclass, "Could not load array element type");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<OutputType>Exe</OutputType>
</PropertyGroup>
<ItemGroup>
<Compile Include="repro.cs" />
</ItemGroup>
</Project>
53 changes: 53 additions & 0 deletions src/tests/Loader/classloader/regressions/GitHub_85821/repro.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
using System;

/* Regression test for https://github.com/dotnet/runtime/issues/85821
* ensure that self-referencing generic instances are initialized correctly and don't TLE
*/

public class Program
{
public static int Main()
{
var test = typeof(Node<double>);
var fit = test.GetField("Children").FieldType;
if (fit == null)
return 101;

var test2 = typeof(Node2<int>);
var fit2 = test2.GetField("Children").FieldType;
if (fit2 == null)
return 102;

var test3 = typeof(NodeVT<double>);
var fit3 = test3.GetField("Children").FieldType;
if (fit3 == null)
return 103;

var test4 = typeof(NodeVT2<int>);
var fit4 = test4.GetField("Children").FieldType;
if (fit4 == null)
return 104;

return 100;
}
}

public class Node<T>
{
public Node<T>[] Children;
}

public class Node2<T>
{
public Node2<T>[][] Children;
}

public struct NodeVT<T>
{
public NodeVT<T>[] Children;
}

public struct NodeVT2<T>
{
public NodeVT2<T>[][] Children;
}

0 comments on commit bdf5df3

Please sign in to comment.