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

Possible overflow exception when casting IntPtr/UIntPtr to nint/nuint #661

Closed
elachlan opened this issue Aug 19, 2022 · 7 comments
Closed
Labels
bug Something isn't working

Comments

@elachlan
Copy link
Contributor

Found this comment in Winforms, which might help sway us towards wanting to use nint/nuint over IntPtr/UIntPtr.

// It is particularly dangerous to cast to/from IntPtr on 64 bit platforms as casts are checked.
// Doing so leads to hard-to-find overflow exceptions. We've mitigated this historically by trying
// to first cast to long when casting out of IntPtr and first casting to int when casting into an
// IntPtr. That pattern, unfortunately, is difficult to audit and has negative performance implications,
// particularly on 32bit.
//
// Using the new nint/nuint types allows casting to follow with the default project settings, which
// is unchecked. Casting works just like casting between other intrinsic types (short, int, long, etc.).

https://github.com/dotnet/winforms/blob/179df82381b5df175d043c2190306354f8b46cae/src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Message.cs#L39-L50

@elachlan elachlan added the bug Something isn't working label Aug 19, 2022
@AArnott
Copy link
Member

AArnott commented Aug 26, 2022

first cast to long when casting out of IntPtr

Which casts are problematic? If casting from IntPtr to long is ok, which cast is not ok? What values in particular lead to the overflow exception?

first casting to int when casting into an IntPtr

What type is being cast to int before casting to IntPtr? That's always a 4-byte value whereas IntPtr is 8 bytes in 64-bit processes, so casting through int would seem to risk corrupting the value.

Do any of these casts relate to converting between IntPtr and UIntPtr? If so, why should such casts work if they would otherwise overflow? While the memory may be identical, the way that memory is interpreted into a value is not.

@AArnott
Copy link
Member

AArnott commented Aug 26, 2022

FWIW I tried this:

IntPtr maxSigned = (IntPtr)long.MaxValue;
UIntPtr maxUnsigned = (UIntPtr)ulong.MaxValue;

UIntPtr signedToUnsigned = (UIntPtr)(long)maxSigned; // intermediate casts required to compile
IntPtr unsignedToSigned = (IntPtr)(ulong)maxUnsigned;

nint maxSigned2 = (nint)long.MaxValue; // compiler emits overflow warnings
nuint maxUnsigned2 = (nuint)ulong.MaxValue;

nuint signedToUnsigned2 = (nuint)maxSigned2;
nint unsignedToSigned2 = (nint)maxUnsigned2;

nint signedConverted = maxSigned;
nuint unsignedConverted = maxUnsigned;

This didn't throw at runtime for net472 or net6.

@dorssel
Copy link

dorssel commented Aug 29, 2022

This depends on the value of CheckForOverflowUnderflow in your project file. I'm quite sure that both:

IntPtr unsignedToSigned = (IntPtr)(ulong)maxUnsigned;
nint unsignedToSigned2 = (nint)maxUnsigned2;

throw when CheckForOverflowUnderflow is true on 64-bit, and these:

nint maxSigned2 = (nint)long.MaxValue; // compiler emits overflow warnings
nuint maxUnsigned2 = (nuint)ulong.MaxValue;

also on 32-bit platforms.

If you just want a (possibly truncated) bit-copy, then explicitly using unchecked, e.g.

IntPtr unsignedToSigned = unchecked((IntPtr)maxUnsigned);

will never throw.

@AArnott
Copy link
Member

AArnott commented Aug 31, 2022

Thanks, @dorssel. I don't think it's that simple though. The checked/unchecked switch in the project only impacts code in that particular compilation. The IntPtr explicit cast operators themselves contain an already baked behavior regarding the checked nature of the cast that cannot vary with a customer project.
I heard something along the lines of .NET 6 and earlier making these checked casts, but .NET 7 removing the checked part of it. Not sure though.

Casting long.MaxValue to IntPtr on a 32-bit process should throw an overflow exception, IMO. I'm trying to find out where exceptions are thrown that shouldn't be.

@dorssel
Copy link

dorssel commented Aug 31, 2022

Ah, yes. You are right.

.NET 6:
https://github.com/dotnet/runtime/blob/55fb7ef977e7d120dc12f0960edcff0739d7ee0e/src/libraries/System.Private.CoreLib/src/System/IntPtr.cs#L49-L57

Same still for .NET 7 preview.
I learned something today, thanks!

@jnm2
Copy link
Contributor

jnm2 commented Aug 31, 2022

I heard something along the lines of .NET 6 and earlier making these checked casts, but .NET 7 removing the checked part of it. Not sure though.

More on this at dotnet/csharplang#4665 (comment).

@AArnott
Copy link
Member

AArnott commented Sep 7, 2022

I'm going to close this issue as I don't see any issue to fix in CsWin32. If anyone can point to something specifically (ideally with example code that's broken) we can reactivate.

@AArnott AArnott closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants