Simplify and improve integer overflow checks in Interop (#21732)

- Delete unnecessary CheckStringLength calls for result of string.Length. Managed strings are guaranteed to be under 2GB bytes, so these checks were unnecessary.
- Add `checked(...)` around buffer size computations that may hit potential integer overflow. It does not look like any of these would cause a bug that would lead to buffer overrun, but it is better to catch these early.

diff --git a/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs b/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
index 09aa503..9761044 100644 (file)
--- a/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
+++ b/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
@@ -178,8 +178,7 @@ namespace System.Runtime.InteropServices
                 return string.Empty;
             }
 
-            byte* pByte = (byte*)ptr.ToPointer();
-            return Encoding.UTF8.GetString(pByte, byteLen);
+            return Encoding.UTF8.GetString((byte*)ptr, byteLen);
         }
 
         public static int SizeOf(object structure)
@@ -971,10 +970,11 @@ namespace System.Runtime.InteropServices
                 return IntPtr.Zero;
             }
 
-            int nb = (s.Length + 1) * SystemMaxDBCSCharSize;
+            long lnb = (s.Length + 1) * (long)SystemMaxDBCSCharSize;
+            int nb = (int)lnb;
 
             // Overflow checking
-            if (nb < s.Length)
+            if (nb != lnb)
             {
                 throw new ArgumentOutOfRangeException(nameof(s));
             }
@@ -1224,10 +1224,11 @@ namespace System.Runtime.InteropServices
                 return IntPtr.Zero;
             }
 
-            int nb = (s.Length + 1) * SystemMaxDBCSCharSize;
+            long lnb = (s.Length + 1) * (long)SystemMaxDBCSCharSize;
+            int nb = (int)lnb;
 
             // Overflow checking
-            if (nb < s.Length)
+            if (nb != lnb)
             {
                 throw new ArgumentOutOfRangeException(nameof(s));
             }

diff --git a/src/System.Private.CoreLib/src/System/StubHelpers.cs b/src/System.Private.CoreLib/src/System/StubHelpers.cs
index 82b2886..4980101 100644 (file)
--- a/src/System.Private.CoreLib/src/System/StubHelpers.cs
+++ b/src/System.Private.CoreLib/src/System/StubHelpers.cs
@@ -24,8 +24,7 @@ namespace System.StubHelpers
         // character set. It is only guaranteed to be larger or equal to cbLength, don't depend on the exact value.
         unsafe internal static byte[] DoAnsiConversion(string str, bool fBestFit, bool fThrowOnUnmappableChar, out int cbLength)
         {
-            byte[] buffer = new byte[(str.Length + 1) * Marshal.SystemMaxDBCSCharSize];
-            Debug.Assert(buffer.Length != 0);
+            byte[] buffer = new byte[checked((str.Length + 1) * Marshal.SystemMaxDBCSCharSize)];
             fixed (byte* bufferPtr = &buffer[0])
             {
                 cbLength = str.ConvertToAnsi(bufferPtr, buffer.Length, fBestFit, fThrowOnUnmappableChar);
@@ -61,8 +60,6 @@ namespace System.StubHelpers
                 return IntPtr.Zero;
             }
 
-            StubHelpers.CheckStringLength(strManaged.Length);
-
             int nb;
             byte* pbNativeBuffer = (byte*)pNativeBuffer;
 
@@ -71,18 +68,18 @@ namespace System.StubHelpers
                 // If we are marshaling into a stack buffer or we can accurately estimate the size of the required heap
                 // space, we will use a "1-pass" mode where we convert the string directly into the unmanaged buffer.
 
-                // + 1 for the null character from the user
-                nb = (strManaged.Length + 1) * Marshal.SystemMaxDBCSCharSize;
+                // + 1 for the null character from the user.  + 1 for the null character we put in.
+                nb = checked((strManaged.Length + 1) * Marshal.SystemMaxDBCSCharSize + 1);
 
                 // Use the pre-allocated buffer (allocated by localloc IL instruction) if not NULL, 
                 // otherwise fallback to AllocCoTaskMem
                 if (pbNativeBuffer == null)
                 {
-                    // + 1 for the null character we put in
-                    pbNativeBuffer = (byte*)Marshal.AllocCoTaskMem(nb + 1);
+                    pbNativeBuffer = (byte*)Marshal.AllocCoTaskMem(nb);
                 }
 
-                nb = strManaged.ConvertToAnsi(pbNativeBuffer, nb + 1, 0 != (flags & 0xFF), 0 != (flags >> 8));
+                nb = strManaged.ConvertToAnsi(pbNativeBuffer, nb,
+                    fBestFit: 0 != (flags & 0xFF), fThrowOnUnmappableChar: 0 != (flags >> 8));
             }
             else
             {
@@ -91,7 +88,8 @@ namespace System.StubHelpers
                 // wasting memory on systems with multibyte character sets where the buffer we end up with is often much
                 // smaller than the upper bound for the given managed string.
 
-                byte[] bytes = AnsiCharMarshaler.DoAnsiConversion(strManaged, 0 != (flags & 0xFF), 0 != (flags >> 8), out nb);
+                byte[] bytes = AnsiCharMarshaler.DoAnsiConversion(strManaged,
+                    fBestFit: 0 != (flags & 0xFF), fThrowOnUnmappableChar: 0 != (flags >> 8), out nb);
 
                 // + 1 for the null character from the user.  + 1 for the null character we put in.
                 pbNativeBuffer = (byte*)Marshal.AllocCoTaskMem(nb + 2);
@@ -128,7 +126,6 @@ namespace System.StubHelpers
             {
                 return IntPtr.Zero;
             }
-            StubHelpers.CheckStringLength(strManaged.Length);
 
             int nb;
             byte* pbNativeBuffer = (byte*)pNativeBuffer;
@@ -221,8 +218,6 @@ namespace System.StubHelpers
             }
             else
             {
-                StubHelpers.CheckStringLength(strManaged.Length);
-
                 byte trailByte;
                 bool hasTrailByte = strManaged.TryGetTrailByte(out trailByte);
 
@@ -348,10 +343,8 @@ namespace System.StubHelpers
 
             cch = strManaged.Length;
 
-            StubHelpers.CheckStringLength(cch);
-
             // length field at negative offset + (# of characters incl. the terminator) * max ANSI char size
-            int nbytes = sizeof(uint) + ((cch + 1) * Marshal.SystemMaxDBCSCharSize);
+            int nbytes = checked(sizeof(uint) + ((cch + 1) * Marshal.SystemMaxDBCSCharSize));
 
             pNative = (byte*)Marshal.AllocCoTaskMem(nbytes);
             int* pLength = (int*)pNative;
@@ -406,14 +399,10 @@ namespace System.StubHelpers
                 return IntPtr.Zero;
             }
 
-            int length = strManaged.Length;
-
-            StubHelpers.CheckStringLength(length);
-
             byte[] bytes = null;
             int nb = 0;
 
-            if (length > 0)
+            if (strManaged.Length > 0)
             {
                 bytes = AnsiCharMarshaler.DoAnsiConversion(strManaged, 0 != (flags & 0xFF), 0 != (flags >> 8), out nb);
             }
@@ -1015,7 +1004,6 @@ namespace System.StubHelpers
             else
             {
                 // marshal the object as Unicode string (UnmanagedType.LPWStr)
-                StubHelpers.CheckStringLength(pManagedHome.Length);
 
                 int allocSize = (pManagedHome.Length + 1) * 2;
                 pNativeHome = Marshal.AllocCoTaskMem(allocSize);
@@ -1050,7 +1038,7 @@ namespace System.StubHelpers
                 StubHelpers.CheckStringLength(pManagedHome.Capacity);
 
                 // marshal the object as Ansi string (UnmanagedType.LPStr)
-                int allocSize = (pManagedHome.Capacity * Marshal.SystemMaxDBCSCharSize) + 4;
+                int allocSize = checked((pManagedHome.Capacity * Marshal.SystemMaxDBCSCharSize) + 4);
                 pNativeHome = Marshal.AllocCoTaskMem(allocSize);
 
                 byte* ptr = (byte*)pNativeHome;
@@ -1074,7 +1062,7 @@ namespace System.StubHelpers
             else
             {
                 // marshal the object as Unicode string (UnmanagedType.LPWStr)
-                int allocSize = (pManagedHome.Capacity * 2) + 4;
+                int allocSize = checked((pManagedHome.Capacity * 2) + 4);
                 pNativeHome = Marshal.AllocCoTaskMem(allocSize);
 
                 byte* ptr = (byte*)pNativeHome;

diff --git a/src/vm/ilmarshalers.cpp b/src/vm/ilmarshalers.cpp
index ba341f1..8b6b15e 100644 (file)
--- a/src/vm/ilmarshalers.cpp
+++ b/src/vm/ilmarshalers.cpp
@@ -316,12 +316,11 @@ void ILWSTRMarshaler::EmitCheckManagedStringLength(ILCodeStream* pslILEmit)
 {
     STANDARD_VM_CONTRACT;
 
+    // Note: The maximum size of managed string is under 2GB bytes. This cannot overflow.
     pslILEmit->EmitCALL(METHOD__STRING__GET_LENGTH, 1, 1);
     pslILEmit->EmitLDC(1);
     pslILEmit->EmitADD();
     pslILEmit->EmitDUP();
-    pslILEmit->EmitCALL(METHOD__STUBHELPERS__CHECK_STRING_LENGTH, 1, 0);
-    pslILEmit->EmitDUP();
     pslILEmit->EmitADD();           // (length+1) * sizeof(WCHAR)
 }
 
@@ -895,12 +894,12 @@ void ILCSTRBufferMarshaler::EmitConvertSpaceCLRToNative(ILCodeStream* pslILEmit)
     // stack: capacity
 
     pslILEmit->EmitLDSFLD(pslILEmit->GetToken(MscorlibBinder::GetField(FIELD__MARSHAL__SYSTEM_MAX_DBCS_CHAR_SIZE)));
-    pslILEmit->EmitMUL();
+    pslILEmit->EmitMUL_OVF();
 
     // stack: capacity_in_bytes
 
     pslILEmit->EmitLDC(1);  
-    pslILEmit->EmitADD();
+    pslILEmit->EmitADD_OVF();
 
     // stack: offset_of_secret_null
 
@@ -909,7 +908,7 @@ void ILCSTRBufferMarshaler::EmitConvertSpaceCLRToNative(ILCodeStream* pslILEmit)
     pslILEmit->EmitSTLOC(dwTmpOffsetOfSecretNull); // make sure the stack is empty for localloc
 
     pslILEmit->EmitLDC(3);
-    pslILEmit->EmitADD();
+    pslILEmit->EmitADD_OVF();
 
     // stack: alloc_size_in_bytes
     ILCodeLabel *pAllocRejoin = pslILEmit->NewCodeLabel(); 
@@ -2194,7 +2193,7 @@ void ILCSTRMarshaler::EmitConvertContentsCLRToNative(ILCodeStream* pslILEmit)
 
         // (String.Length + 2) * GetMaxDBCSCharByteSize()
         pslILEmit->EmitLDSFLD(pslILEmit->GetToken(MscorlibBinder::GetField(FIELD__MARSHAL__SYSTEM_MAX_DBCS_CHAR_SIZE)));
-        pslILEmit->EmitMUL();
+        pslILEmit->EmitMUL_OVF();
 
         // BufSize = (String.Length + 2) * GetMaxDBCSCharByteSize()
         pslILEmit->EmitSTLOC(dwBufSize);