InlineArray size validation does not handle all cases

[DaZombieKiller]

Description

The size of an InlineArray is supposed to be limited to 1MiB, but the example below slips through:

[InlineArray(2)]
struct BigArray
{
    BigElement element;
}

[StructLayout(LayoutKind.Sequential, Size = int.MaxValue)]
struct BigElement
{
}

Reproduction Steps

Given the type definitions from earlier:

unsafe { Console.WriteLine(sizeof(BigArray)); }

Expected behavior

The runtime should throw a TypeLoadException.

Actual behavior

No exception occurs, and sizeof(BigArray) returns -2.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

[jkotas]

cc @VSadov

[stephentoub]

Why is that InlineArray limitation there in the first place?

[kotlarmilos]

The limit has been chosen arbitrarily: #83776 (comment). @VSadov, please let me know if you decide to change the limit so that we can make the necessary adjustments in the Mono runtime.

[VSadov]

This is likely just a bug with size check not covering one of scenarios or not checking for overflows. Possibly just on one runtime.
I do not think this is a reason for changing the actual limit.

The idea is that once an inline array is too large to be safely used on stack, the heap-only use does not have enough advantage over regular arrays.
Another concern that even in borderline cases the behavior could be fragile and expose differences in codegen strategy between JITs and modes (i.e. Release/Debug) and different stack size limits between platforms and lead to random stack overflows.

Thus the limit is 1MiB. More than that is likely a mistake or a misuse of the feature.

[stephentoub]

Thus the limit is 1MiB

But does the C# compiler know about this limit?

[VSadov]

But does the C# compiler know about this limit?

I am not sure C# compiler knows sizes of structs in general. If that is the case it can`t enforce limits.

[stephentoub]

Then how can we consider it "mistake or misuse" if the compiler is using these automatically as an implementation detail but can't and doesn't check whether it'll trip over the undocumented limit?

[tannergooding]

the heap-only use does not have enough advantage over regular arrays.

There is notably significant advantage in certain types of perf oriented code. Removing that indirection can be very important and can allow developers to codify their own powerful types that have large amounts of inline data. For example, it could be used to build specialized types of ECS.

The current limit is in fact arbitrary, and there are platforms where it might overflow sooner. There are likewise platforms where it might overflow later. This is no different than stackalloc or other types of custom functionality which can incur the same.

I think its ultimately better to just not restrict it, outside of disallowing overflow, and to allow users to freely use it as they desire. If there is concern over "too large" types, then having an analyzer that warns for users specifying over a certain size seems like a reasonable middleground.

[jkotas]

Then how can we consider it "mistake or misuse" if the compiler is using these automatically as an implementation detail but can't and doesn't check whether it'll trip over the undocumented limit?

We have number of similar internal runtime implementation limits. These limits vary between runtime flavors and architectures. For the specific internal uses of InlineArray by the compiler, I would expect that the (artificially complicated) code is much more likely to hit other internal runtime implementation limits than the current InlineArray limit.

[stephentoub]

I don't understand what the limit is there to help with, though. What happens if it's removed? The reasoning cites differences like stack sizes showing up, but that's the case even with the limit in place.

(artificially complicated) code

The generated IL is artificially complicated? Then let's simplify it.

[jkotas]

I don't understand what the limit is there to help with, though. What happens if it's removed?

Allowing sizes to go all the value to MaxValue is a corner case bug farm. I would be ok with making the limit higher, but I do not think we should remove it. For example, I would be ok with making the limit same as the field offset limit.

The generated IL is artificially complicated? Then let's simplify it.

I meant artificially complicated user code.

[tannergooding]

For example, I would be ok with making the limit same as the field offset limit.

What is the current limit here?

Is this (the InlineArray limit) something that would be worth exposing a property for? We expose Array.MaxLength for example, would having InlineArrayAttribute.MaxLength be similarly reasonable to help ensure these edges can be avoided?

[VSadov]

I do not think it is possible to have no limit. There are limits on max field offsets (effectively size of structs), stack size limits. Some of that would depend on platform/runtime/64bit.

Would it be better to just let the user randomly running into one of these?

With ordinary structs running into limits is rare, since you'd have to define every field, and you need a lot of fields.
With ValueArray it is much easier to just pick a big number and end up with something that works sometimes.

[stephentoub]

Would it be better to just let the user randomly running into one of these?

At least for stack size, you can already run into it, with or without the current IA limit.

[VSadov]

What is the current limit here?

FIELD_OFFSET_LAST_REAL_OFFSET is ((1 << 27) - 1 ) - 6 on CoreCLR.
It is an arbitrary huge number and there may not be an agreement between runtimes on exact value.

FIELD_OFFSET_LAST_REAL_OFFSET what I used originally as a limit and we did not like it for the above reasons - arbitrary implementation detail, potentially platform specific, clearly too large,

[tannergooding]

arbitrary implementation detail

This is no different than the 1MB currently selected.

potentially platform specific

This is no different than things like Array.MaxLength or other limits that can be hit. There are platform or runtime specific behaviors in some cases. Even with InlineArray and the current 1MB, it is just as platform specific on whether that will work on not. By default on MacOS, such a limit will always overflow background threads (which default to 512KB).

These limits exist, but they are designed to be high enough that almost no code will ever need to consider them. But even with these very high limits, people still do hit them and then we have to decide if its worth changing or adjusting it later (and in some cases we can't change it later).

clearly too large

While 134,217,726 bytes (roughly 127.99 MB) may be significantly larger than normally "necessary", I wouldn't say its clearly too large. Something like string has a current limit of 2,147,483,582 bytes (roughly 2047.99 MB). Array has a limit of 0x7FFF_FFC7 elements (and so can potentially be many GB).

There are clearly cases for users to have inline buffers that large in other types and while they may primarily use things like T[] or string to handle many of these, there are still specialized scenarios that could easily warrant having larger buffers, especially as trailing data.

It's been discussed in other threads how having the ability to define something like Array<T, TMetadata> where it is logically int length; TMetadata metadata; T item0, ..., itemN; would be beneficial. For example it could allow us to implement List<T> that would allow bounds checks to be safely elided since we could prevent the tear between int _size and T[] _items.

Another example is that its common to do instancing in games/graphics and so you may have n sequential Matrix4x4. A Matrix4x4 is 64-bytes, so you get up to 16k instances before you hit the limit. Some games can easily instance 16k meshes and so this unnecessarily forces you to break up your data structures.

Users can likewise define structs that are bigger than 128 MB and so while the field offset can't be greater than 128 MB, it seems like it generally "just works" provided its the trailing field and therefore the offset to the next field isn't greater than 127.99 MB.

---

It really seems better to just not give this a real limit (outside preventing overflow) and to let users hit it themselves as they would if they did [StructLayout(LayoutKind.Sequential, Size = 1024 * 1024 * 256)], which is that they hit a TypeLoad exception if it isn't the last field in a struct. This doesn't unnecessarily restrict things while also avoiding many of those edge case bugs.

// This is fine today
[StructLayout(LayoutKind.Sequential, Size = 1024 * 1024 * 256)]
struct S1 { }

// This is fine today
struct S2 { public int x; public S1 s; }

// This fails today
struct S3 { public int x; public S1 s; public int y; }

// This fails today
class C { public S2 s; }

S2 is then still fully usable with NativeMemory and other scenarios, without users having to do even worse or more complex code. Allowing InlineArray just makes this simpler, cleaner, and ultimately safer for the users already doing dangerous or things that are out of the norm; while still failing in the same cases the alternative already would.

[VSadov]

I think we need to get this to some conclusion.
My understanding is that so far we want some kind of limit (lest there will be implementation specific and undocumented limits anyways).
However we did not agree on what the limit should be - whether it is 1Mb, 1Gb or something like ((1 << 27) - 1 ) - 6

[MichalStrehovsky]

My understanding is that so far we want some kind of limit (lest there will be implementation specific and undocumented limits anyways).

Can someone explain why [InlineArray(int.MaxValue)] struct Foo { byte b } is bad and must be banned, but [StructLayout(LayoutKind.Sequential, Size = int.MaxValue)] struct Foo { } or unsafe struct Foo { fixed byte _bytes[int.MaxValue]; } loads just fine? The implementation limitations we have get enforced in different places but there doesn't seem to be a limit around type size itself. What's different here? Why can't this be fixed by adding an overflow check?

I recently used this in an app to get a framebuffer of 640x480 pixels into the .bss segment of the executable (on native AOT). It hit the 1 MB limit because on native AOT it got enforced even for sequential layout (that's a separate bug I have a PR for). I replaced it with a fixed array because that one doesn't have a made up limit.

[tannergooding]

I still don't think it should be banned and called out many of the same scenarios you did above.

I strongly believe the ecosystem is better off making this similar to other similar cases and not arbitrarily limiting this to something we think is "reasonable" (especially something as small as 1MB). Allowing this be up towards int.MaxValue with overflow detection, should largely just work.

[MichalStrehovsky]

I still don't think it should be banned and called out many of the same scenarios you did above.

Yep, my question was for @VSadov and @jkotas who are asking for a limit. You and I are on the same page of not understanding why is this different from other cases that have been possible for 20 years.

Adding a limit now is a breaking change because we shipped this way. Not "breaking 20 years of written code"-level, but breaking an LTS release nonetheless. The fact we enforce it on Auto is kind of meaningless because that's not the popular layout.

[jkotas]

Yep, my question was for @VSadov and @jkotas who are asking for a limit.

I have shared my opinion in #95193 (comment). If you would like to allow the full range to int.MaxValue, feel free to go for it. I do not see value in it.

Adding a limit now is a breaking change

The type loader has silent integer overflows and it is allowing types that the runtime is not able to represent correctly (e.g. the type at the top of the issue). It is bug that we need to fix. It is not ok to have silent integer overflows in the type loader that lead to incorrect behavior. Fixing this issue can break some of the existing code that is trying to push the limits. We can file a breaking change notice for this fix if we consider it to be impactful enough.

These silent integer overflows are present in handling of LayoutKind.Sequential as well. For example, this:

Console.WriteLine(sizeof(X));

struct X
{
   byte x;
   BigArray a;
}

[StructLayout(LayoutKind.Sequential, Size = int.MaxValue)] 
struct BigArray
{
}

prints -2147483648.

[MichalStrehovsky]

It is bug that we need to fix. It is not ok to have silent integer overflows in the type loader that lead to incorrect behavior. Fixing this issue can break some of the existing code that is trying to push the limits. We can file a breaking change notice for this fix if we consider it to be impactful enough.

If you consider unrestricted StructLayout Size equally problematic and we'd fix both at the same time, I can get on board with that.

I think the limit will have to be high enough that we don't feel bad if we break someone whose code worked for 20 years. I would put it wherever we have implementation limitations, not document the specific number, and don't try to keep it in sync with Mono. FIELD_OFFSET_LAST_REAL_OFFSET should be the absolute minimum, but we could probably push to INT32_MAX - FIELD_OFFSET_LAST_REAL_OFFSET; maybe less if there's GC pointers in the InlineArray (that one could be just capped at FIELD_OFFSET_LAST_REAL_OFFSET for simplicity; we already enforce the 1 MB cap there anyway so that part is not a breaking change).

[VSadov]

After thinking about this for a while I think making the limit the same as FIELD_OFFSET_LAST_REAL_OFFSET is the least controversial.

• we need some limit
• @MichalStrehovsky example with static inline array used as a framebuffer is a convincing example that 1Mb is too small.
• if user constructs an equivalent struct by hand struct S1{byte i0, byte i2, byte i3, . . . } the limit will be FIELD_OFFSET_LAST_REAL_OFFSET

Even though FIELD_OFFSET_LAST_REAL_OFFSET is an implementation detail, we already have it, so we do not need to invent another one, and it seems large enough.

[lambdageek]

(Mono in .NET 8 throws a TLE on the sample above; no fix needed there)

[lambdageek]

Although Mono doesn't like a struct BigElement with a size bigger than 1Mb. We should consider increasing.

using System.Runtime.InteropServices;
using System.Runtime.CompilerServices;

unsafe
{
    Console.WriteLine (sizeof (BigElement)); // this works in CoreCLR even if the size of BigElement is greater than 1 << 27
    Console.WriteLine (sizeof(/*BigArray*/ BigStruct)); // this throws on CoreCLR if small_element is past MaxFieldOffset
}

struct BigStruct
{
    BigElement element;
    byte small_element;
}

[StructLayout(LayoutKind.Sequential, Size = MaxFieldOffset.Value)]
struct BigElement
{
}

static class MaxFieldOffset {
        public const int Value = 1 << 20 + 1; /* 1 << 20 works in Mono; upto  ((1<<27) - 1) - 6 works with CoreCLR */
}