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

The type loader has silent integer overflows #97412

Closed
VSadov opened this issue Jan 23, 2024 · 4 comments · Fixed by #104393
Closed

The type loader has silent integer overflows #97412

VSadov opened this issue Jan 23, 2024 · 4 comments · Fixed by #104393
Labels
area-TypeSystem-coreclr in-pr There is an active PR which will close this issue when it is merged runtime-coreclr specific to the CoreCLR runtime
Milestone

Comments

@VSadov
Copy link
Member

VSadov commented Jan 23, 2024

From: #95193 (comment)

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.

@VSadov VSadov added area-VM-coreclr runtime-coreclr specific to the CoreCLR runtime labels Jan 23, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 23, 2024
@lambdageek lambdageek added area-TypeSystem-coreclr and removed area-VM-coreclr untriaged New issue has not been triaged by the area owner labels Jan 25, 2024
@lambdageek lambdageek added this to the 9.0.0 milestone Jan 25, 2024
@lambdageek
Copy link
Member

/cc @steveisok

@MichalStrehovsky
Copy link
Member

Is this really an integer overflow in the type loader? Spec for the IL sizeof instruction says the result is unsigned int32. The code generated by Roslyn treats it like it was signed so it gets formatted as signed but it wasn't.

The signedness of the value disappears on the IL evaluation stack so it's not bad IL, but still kind of wrong.

Cc @jaredpar

@jaredpar
Copy link
Member

Spec for the IL sizeof instruction says the result is unsigned int32

The spec for C# says it's a signed int. 🤷

That decision was so long ago, hard to say why the two disagreed here. Wonder if that was due to CLS compliance and wanting to avoid unsigned types in the language. That's just speculation. But either way, it's very explicit in the language that it's done this way.

@jkotas
Copy link
Member

jkotas commented Jul 25, 2024

I agree that the runtime is following the IL spec and the problem is technically in C#. I think we should make the runtime fit C# that is our primary user experience.

@steveisok steveisok modified the milestones: 9.0.0, 10.0.0 Aug 8, 2024
@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.
Labels
area-TypeSystem-coreclr in-pr There is an active PR which will close this issue when it is merged runtime-coreclr specific to the CoreCLR runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants