diff options
author | Jan Kotas <jkotas@microsoft.com> | 2019-01-02 03:12:27 -1000 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-01-02 03:12:27 -1000 |
commit | a5b1c68d4bb8a14042e93acb8f1032db4703b943 (patch) | |
tree | 3fa2dbae724b7e00b14eece0e18a0002ea3cf7cb | |
parent | ccaba224b69cc3c334e73226e592dde5864afc14 (diff) | |
download | coreclr-a5b1c68d4bb8a14042e93acb8f1032db4703b943.tar.gz coreclr-a5b1c68d4bb8a14042e93acb8f1032db4703b943.tar.bz2 coreclr-a5b1c68d4bb8a14042e93acb8f1032db4703b943.zip |
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.
-rw-r--r-- | src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs | 13 | ||||
-rw-r--r-- | src/System.Private.CoreLib/src/System/StubHelpers.cs | 36 | ||||
-rw-r--r-- | src/vm/ilmarshalers.cpp | 11 |
3 files changed, 24 insertions, 36 deletions
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 09aa5033b1..9761044927 100644 --- 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 82b28863fa..4980101ddc 100644 --- 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 ba341f1560..8b6b15e398 100644 --- 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); |