Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error out when struct size is bigger than int.MaxValue #104393

Merged
merged 20 commits into from
Aug 21, 2024

Conversation

fanyang-mono
Copy link
Member

@fanyang-mono fanyang-mono commented Jul 3, 2024

Fixes: #97412

sizeof() in C# returns a signed integer. This PR is to adjust the runtime to match with that.

Added a check for struct size and a test.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 3, 2024
@jkotas
Copy link
Member

jkotas commented Jul 3, 2024

Do we need a test?

@fanyang-mono
Copy link
Member Author

fanyang-mono commented Jul 9, 2024

@jkotas This PR is ready to be reviewed again.

@MichalStrehovsky MichalStrehovsky self-requested a review as a code owner July 11, 2024 15:06
@MichalStrehovsky
Copy link
Member

I've pushed out a change that makes further progress in the handling within the managed type system, but the test probably still won't compile because BigArray with size of int.MaxValue gets its size bumped past int.MaxValue through alignment after we're done with layout (the final size needs to be divisible by pointer size and int.MaxValue isn't.

CoreCLR is fine with this overflowing but the managed type system isn't. I'm not sure if the overflow is an actual problem but fixing it doesn't look exactly trivial.

@fanyang-mono
Copy link
Member Author

@MichalStrehovsky Is there a way to stop the new test from being compiled by NativeAOT?

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky Is there a way to stop the new test from being compiled by NativeAOT?

Only by placing <NativeAotIncompatible>true</NativeAotIncomplatible> in the project file. It would be good not to disable all existing ManagedSequential type loader testing just because the new test doesn't work (i.e. put it in a separate project file).

Note that this is the managed type system that is shared with crossgen2. Might want to run crossgen2 outerloop testing to see whether this also needs to be removed from precompilation there.

The managed type system doesn't handle this test because the size of the BigArray struct is actually over 2 GB when boxed (this is also true on JIT-based CoreCLR). If a 2 GB overflow is problematic, is the 2 GB overflow of that one not problematic?

@fanyang-mono
Copy link
Member Author

@MichalStrehovsky Based on Jan's comment from earlier. Loading BigArray itself should continue to work. Why do we need to align BigArray to pointer size? If that's needed, sizeof(BigArray) would return a bigger value than int.MaxValue. Is that correct?

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky Based on Jan's comment from earlier. Loading BigArray itself should continue to work. Why do we need to align BigArray to pointer size? If that's needed, sizeof(BigArray) would return a bigger value than int.MaxValue. Is that correct?

The size of it gets aligned when boxing it, because it's 8 bytes larger (MethodTable pointer) and aligned. On CoreCLR today, doing:

BigArray.Instance = new BigArray();

long before = GC.GetAllocatedBytesForCurrentThread();
BigArray.Instance = new BigArray();
Console.WriteLine(GC.GetAllocatedBytesForCurrentThread() - before);

[StructLayout(LayoutKind.Sequential, Size = int.MaxValue)]
struct BigArray
{
    public static object Instance;
}

Will print 2,147,483,664, which is larger than int.MaxValue.

@fanyang-mono
Copy link
Member Author

/azp list

Copy link

CI/CD Pipelines for this repository:

@MichalStrehovsky
Copy link
Member

The size of it gets aligned when boxing it, because it's 8 bytes larger (MethodTable pointer) and aligned. On CoreCLR today, doing:

We chatted with Fan on Teams about this and apparently the program I posted only works on a Release build (also without debugger attached) and throws InvalidProgram otherwise. On .NET 8 CoreCLR-JIT. Curious.

@fanyang-mono
Copy link
Member Author

@MichalStrehovsky CoreCLR and Mono side of work has been completed and approved. The remaining piece of work is related to NativeAOT/Crossgen2. I am going to transfer the ownership of this PR to you, since you will be working on that.

Additionally, this could probably be merged for .NET9, as I don't think that this is a breaking change. @jkotas Please correct me, if I were wrong.

@MichalStrehovsky
Copy link
Member

The remaining piece of work is related to NativeAOT/Crossgen2. I am going to transfer the ownership of this PR to you, since you will be working on that.

I'll not have time to look at this any time soon. Might be better to just block the test on an issue and not block this PR.

Additionally, this could probably be merged for .NET9, as I don't think that this is a breaking change.

Throwing an exception in a situation that didn't throw before is the definition of a breaking change.

@fanyang-mono
Copy link
Member Author

Discussed with @jkotas offline. He suggested to wait for .NET10 to merge it, together with the NativeAOT change, when it is ready.

There is no rush. @MichalStrehovsky

@fanyang-mono
Copy link
Member Author

I plan to merge this PR when main is .NET10. The managed type system part used by NativeAOT and crossgen can be worked on later during .NET10 release.

@fanyang-mono
Copy link
Member Author

/azp run runtime-coreclr crossgen2

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono
Copy link
Member Author

/azp run runtime-coreclr crossgen2

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono
Copy link
Member Author

/azp run runtime-coreclr crossgen2

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fanyang-mono
Copy link
Member Author

CI failures are unrelated to this PR.

@fanyang-mono fanyang-mono merged commit 423faed into dotnet:main Aug 21, 2024
166 of 173 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The type loader has silent integer overflows
4 participants