Audit source-generated marshallers for integer overflow bugs

[jkotas]

For example, here:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/ArrayMarshaller.cs

Line 69 in bcded44

int spaceToAllocate = Math.Max(array.Length * _sizeOfNativeElement, 1);

What happens when this multiplication overflows?

[ghost]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

---

For example, here:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/ArrayMarshaller.cs

Line 69 in bcded44

int spaceToAllocate = Math.Max(array.Length * _sizeOfNativeElement, 1);

What happens when this multiplication overflows?

Author:	jkotas
Assignees:	-
Labels:	area-System.Runtime.InteropServices
Milestone:	-

[AaronRobinsonMSFT]

Should we be wrapping most arithmetic operations in checked?

e.g., checked(array.Length * _sizeOfNativeElement)

I believe this would use the mul.ovf vs mul. Do we have official .NET guidance on this sort of thing?

[jkotas]

Should we be wrapping most arithmetic operations in checked?

Yes. And/or do buffer size calculations using nuints if it is appropriate.

Do we have official .NET guidance on this sort of thing?

There is a lot of generic guidance about integer overflows. I do not know about a good .NET specific guidance. @GrabYourPitchforks ?

[AaronRobinsonMSFT]

There is a lot of generic guidance about integer overflows.

Yeah I get the general problem. My first thought was that .NET always checked this by default - didn't realize this wasn't the case. The second part of this query is after reading the JIT code for mul.ovf it does have some overhead. That begs the question should we be doing a check upfront to determine if we should throw ourselves and avoid the checked approach or should we always use the checked and rely on the JIT.

[jkotas]

I think it is fine to use checked and depend on the JIT. checked is only necessary for the initial buffer size calculation. Once the buffer is allocated, it is typically not necessary to use checked since you know that it is not going to overflow.

[tannergooding]

This might also be able to drive some work for the JIT to handle cases where checked is used but no overflow is possible. The JIT doesn't really optimize those cases at all today

I'd agree that using checked is desirable here, however. Especially if its just a "one time" cost. The check is often ins; jcc rather than an explicit cmp, jcc, ins or other more complex sequence

[GrabYourPitchforks]

I noticed this in the string marshallers as well the other day, e.g.:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/Utf8StringMarshaller.cs

Line 48 in 6ca8c9b

int maxByteCount = Encoding.UTF8.GetMaxByteCount(str.Length) + 1;

I think the worst that will happen here is a runtime exception since somebody somewhere will detect the overflow, but I don't think we want to rely on the good graces of the implementation to handle error checking on our behalf.