Skip to content

Commit

Permalink
Cleanup memory copy helpers (dotnet#27307)
Browse files Browse the repository at this point in the history
- Merge RuntimeImports that has just a few FCalls into Buffer.
- Delete assembly implementation of memcpy for ARM. The implementation was outdated and it was used only on Windows ARM and only for fraction of memcpy callsites.
- Delete unnecessary PInvoke into SysStringLen. We have a managed equivalent on System.Runtime.InteropServices.Marshal.
- Delete large amount of redundant code between SecureString.Windows.cs and SecureString.Unix.cs, and switch SecureString to use Spans
- Fold null check into IsWin32Atom
  • Loading branch information
jkotas committed Oct 21, 2019
1 parent e062585 commit 17cbbee
Show file tree
Hide file tree
Showing 31 changed files with 456 additions and 2,227 deletions.
3 changes: 0 additions & 3 deletions src/System.Private.CoreLib/System.Private.CoreLib.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -415,9 +415,6 @@
<Compile Include="$(CommonPath)\NotImplemented.cs" />
<Compile Include="$(CommonPath)\System\SR.cs" />
</ItemGroup>
<ItemGroup>
<Compile Include="src\System\Runtime\RuntimeImports.cs" />
</ItemGroup>
<Import Project="shared\System.Private.CoreLib.Shared.projitems" />
<PropertyGroup>
<CheckCDefines Condition="'$(CheckCDefines)'==''">true</CheckCDefines>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ internal static partial class Crypt32
internal const uint CRYPTPROTECTMEMORY_SAME_PROCESS = 0;

[DllImport(Libraries.Crypt32, CharSet = CharSet.Unicode, SetLastError = true)]
internal static extern bool CryptProtectMemory(SafeBSTRHandle pData, uint cbData, uint dwFlags);
internal static extern bool CryptProtectMemory(SafeBuffer pData, uint cbData, uint dwFlags);

[DllImport(Libraries.Crypt32, CharSet = CharSet.Unicode, SetLastError = true)]
internal static extern bool CryptUnprotectMemory(SafeBSTRHandle pData, uint cbData, uint dwFlags);
internal static extern bool CryptUnprotectMemory(SafeBuffer pData, uint cbData, uint dwFlags);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@ internal static partial class Interop
{
internal static partial class OleAut32
{
[DllImport(Libraries.OleAut32, CharSet = CharSet.Unicode)]
internal static extern SafeBSTRHandle SysAllocStringLen(IntPtr src, uint len);

[DllImport(Libraries.OleAut32, CharSet = CharSet.Unicode)]
internal static extern IntPtr SysAllocStringLen(string? src, int len);
}
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -1195,7 +1195,6 @@
<Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\Interop.Errors.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\OleAut32\Interop.SysAllocStringLen.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\OleAut32\Interop.SysFreeString.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\OleAut32\Interop.SysStringLen.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\Kernel32\Interop.CloseHandle.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\Kernel32\Interop.Constants.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\Kernel32\Interop.EventWaitHandle.cs" />
Expand All @@ -1210,7 +1209,6 @@
<Compile Include="$(MSBuildThisFileDirectory)Interop\Windows\Kernel32\Interop.WriteFile_SafeHandle_IntPtr.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Environment.Variables.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\IO\Win32Marshal.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Security\SafeBSTRHandle.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\Mutex.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\Semaphore.Windows.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\EventWaitHandle.Windows.cs" />
Expand Down
21 changes: 4 additions & 17 deletions src/System.Private.CoreLib/shared/System/Buffer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,6 @@ public static void SetByte(Array array, int index, byte value)
Unsafe.Add<byte>(ref array.GetRawArrayData(), index) = value;
}

// This is currently used by System.IO.UnmanagedMemoryStream
internal static unsafe void ZeroMemory(byte* dest, long len)
{
Debug.Assert((ulong)(len) == (nuint)(len));
ZeroMemory(dest, (nuint)(len));
}

// This method has different signature for x64 and other platforms and is done for performance reasons.
internal static unsafe void ZeroMemory(byte* dest, nuint len)
{
Expand Down Expand Up @@ -338,16 +331,10 @@ ref Unsafe.As<T, byte>(ref source),
else
{
// Non-blittable memmove

// Try to avoid calling RhBulkMoveWithWriteBarrier if we can get away
// with a no-op.
if (!Unsafe.AreSame(ref destination, ref source) && elementCount != 0)
{
RuntimeImports.RhBulkMoveWithWriteBarrier(
ref Unsafe.As<T, byte>(ref destination),
ref Unsafe.As<T, byte>(ref source),
elementCount * (nuint)Unsafe.SizeOf<T>());
}
BulkMoveWithWriteBarrier(
ref Unsafe.As<T, byte>(ref destination),
ref Unsafe.As<T, byte>(ref source),
elementCount * (nuint)Unsafe.SizeOf<T>());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@
using System.Threading;
using System.Threading.Tasks;

#pragma warning disable SA1121 // explicitly using type aliases instead of built-in types
#if BIT64
using nuint = System.UInt64;
#else
using nuint = System.UInt32;
#endif

namespace System.IO
{
/*
Expand Down Expand Up @@ -620,7 +627,7 @@ public override void SetLength(long value)
{
unsafe
{
Buffer.ZeroMemory(_mem + len, value - len);
Buffer.ZeroMemory(_mem + len, (nuint)(value - len));
}
}
Interlocked.Exchange(ref _length, value);
Expand Down Expand Up @@ -690,7 +697,7 @@ internal unsafe void WriteCore(ReadOnlySpan<byte> buffer)
// zero any memory in the middle.
if (pos > len)
{
Buffer.ZeroMemory(_mem + len, pos - len);
Buffer.ZeroMemory(_mem + len, (nuint)(pos - len));
}

// set length after zeroing memory to avoid race condition of accessing unzeroed memory
Expand Down Expand Up @@ -831,7 +838,7 @@ public override void WriteByte(byte value)
{
unsafe
{
Buffer.ZeroMemory(_mem + len, pos - len);
Buffer.ZeroMemory(_mem + len, (nuint)(pos - len));
}
}

Expand Down
10 changes: 5 additions & 5 deletions src/System.Private.CoreLib/shared/System/Number.BigInteger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ public static void Multiply(ref BigInteger lhs, ref BigInteger rhs, ref BigInteg
Debug.Assert(unchecked((uint)(maxResultLength)) <= MaxBlockCount);

// Zero out result internal blocks.
Buffer.ZeroMemory((byte*)(result.GetBlocksPointer()), maxResultLength * sizeof(uint));
Buffer.ZeroMemory((byte*)result.GetBlocksPointer(), (uint)maxResultLength * sizeof(uint));

int smallIndex = 0;
int resultStartIndex = 0;
Expand Down Expand Up @@ -1122,7 +1122,7 @@ public void Multiply(ref BigInteger value)
var result = new BigInteger(0);
Multiply(ref this, ref value, ref result);

Buffer.Memcpy((byte*)(GetBlocksPointer()), (byte*)(result.GetBlocksPointer()), (result._length) * sizeof(uint));
Buffer.Memcpy((byte*)GetBlocksPointer(), (byte*)result.GetBlocksPointer(), result._length * sizeof(uint));
_length = result._length;
}

Expand Down Expand Up @@ -1189,7 +1189,7 @@ public void SetUInt64(ulong value)
public void SetValue(ref BigInteger rhs)
{
int rhsLength = rhs._length;
Buffer.Memcpy((byte*)(GetBlocksPointer()), (byte*)(rhs.GetBlocksPointer()), rhsLength * sizeof(uint));
Buffer.Memcpy((byte*)GetBlocksPointer(), (byte*)rhs.GetBlocksPointer(), rhsLength * sizeof(uint));
_length = rhsLength;
}

Expand Down Expand Up @@ -1229,7 +1229,7 @@ public void ShiftLeft(uint shift)
_length += (int)(blocksToShift);

// Zero the remaining low blocks
Buffer.ZeroMemory((byte*)(GetBlocksPointer()), blocksToShift * sizeof(uint));
Buffer.ZeroMemory((byte*)GetBlocksPointer(), blocksToShift * sizeof(uint));
}
else
{
Expand Down Expand Up @@ -1262,7 +1262,7 @@ public void ShiftLeft(uint shift)
_blocks[writeIndex - 1] = block << (int)(remainingBitsToShift);

// Zero the remaining low blocks
Buffer.ZeroMemory((byte*)(GetBlocksPointer()), blocksToShift * sizeof(uint));
Buffer.ZeroMemory((byte*)GetBlocksPointer(), blocksToShift * sizeof(uint));

// Check if the terminating block has no set bits
if (_blocks[_length - 1] == 0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public static IntPtr StringToCoTaskMemAuto(string? s)

private static int GetSystemMaxDBCSCharSize() => 3;

private static bool IsWin32Atom(IntPtr ptr) => false;
private static bool IsNullOrWin32Atom(IntPtr ptr) => ptr == IntPtr.Zero;

internal static unsafe int StringToAnsiString(string s, byte* buffer, int bufferLength, bool bestFit = false, bool throwOnUnmappableChar = false)
{
Expand All @@ -48,5 +48,19 @@ internal static unsafe int StringToAnsiString(string s, byte* buffer, int buffer

return convertedBytes;
}

// Returns number of bytes required to convert given string to Ansi string. The return value includes null terminator.
internal static unsafe int GetAnsiStringByteCount(ReadOnlySpan<char> chars)
{
int byteLength = Encoding.UTF8.GetByteCount(chars);
return checked(byteLength + 1);
}

// Converts given string to Ansi string. The destination buffer must be large enough to hold the converted value, including null terminator.
internal static unsafe void GetAnsiStringBytes(ReadOnlySpan<char> chars, Span<byte> bytes)
{
int actualByteLength = Encoding.UTF8.GetBytes(chars, bytes);
bytes[actualByteLength] = 0;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,8 @@ private static unsafe int GetSystemMaxDBCSCharSize()
// Win32 has the concept of Atoms, where a pointer can either be a pointer
// or an int. If it's less than 64K, this is guaranteed to NOT be a
// pointer since the bottom 64K bytes are reserved in a process' page table.
// We should be careful about deallocating this stuff. Extracted to
// a function to avoid C# problems with lack of support for IntPtr.
// We have 2 of these methods for slightly different semantics for NULL.
private static bool IsWin32Atom(IntPtr ptr)
// We should be careful about deallocating this stuff.
private static bool IsNullOrWin32Atom(IntPtr ptr)
{
const long HIWORDMASK = unchecked((long)0xffffffffffff0000L);

Expand Down Expand Up @@ -82,5 +80,52 @@ internal static unsafe int StringToAnsiString(string s, byte* buffer, int buffer
buffer[nb] = 0;
return nb;
}

// Returns number of bytes required to convert given string to Ansi string. The return value includes null terminator.
internal static unsafe int GetAnsiStringByteCount(ReadOnlySpan<char> chars)
{
int byteLength;

if (chars.Length == 0)
{
byteLength = 0;
}
else
{
fixed (char* pChars = chars)
{
byteLength = Interop.Kernel32.WideCharToMultiByte(
Interop.Kernel32.CP_ACP, Interop.Kernel32.WC_NO_BEST_FIT_CHARS, pChars, chars.Length, null, 0, IntPtr.Zero, IntPtr.Zero);
if (byteLength <= 0)
throw new ArgumentException();
}
}

return checked(byteLength + 1);
}

// Converts given string to Ansi string. The destination buffer must be large enough to hold the converted value, including null terminator.
internal static unsafe void GetAnsiStringBytes(ReadOnlySpan<char> chars, Span<byte> bytes)
{
int byteLength;

if (chars.Length == 0)
{
byteLength = 0;
}
else
{
fixed (char* pChars = chars)
fixed (byte* pBytes = bytes)
{
byteLength = Interop.Kernel32.WideCharToMultiByte(
Interop.Kernel32.CP_ACP, Interop.Kernel32.WC_NO_BEST_FIT_CHARS, pChars, chars.Length, pBytes, bytes.Length, IntPtr.Zero, IntPtr.Zero);
if (byteLength <= 0)
throw new ArgumentException();
}
}

bytes[byteLength] = 0;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public static partial class Marshal

public static unsafe string? PtrToStringAnsi(IntPtr ptr)
{
if (ptr == IntPtr.Zero || IsWin32Atom(ptr))
if (IsNullOrWin32Atom(ptr))
{
return null;
}
Expand All @@ -64,7 +64,7 @@ public static unsafe string PtrToStringAnsi(IntPtr ptr, int len)

public static unsafe string? PtrToStringUni(IntPtr ptr)
{
if (ptr == IntPtr.Zero || IsWin32Atom(ptr))
if (IsNullOrWin32Atom(ptr))
{
return null;
}
Expand All @@ -88,7 +88,7 @@ public static unsafe string PtrToStringUni(IntPtr ptr, int len)

public static unsafe string? PtrToStringUTF8(IntPtr ptr)
{
if (ptr == IntPtr.Zero || IsWin32Atom(ptr))
if (IsNullOrWin32Atom(ptr))
{
return null;
}
Expand Down Expand Up @@ -915,13 +915,13 @@ public static int GetHRForLastWin32Error()

public static IntPtr /* IDispatch */ GetIDispatchForObject(object o) => throw new PlatformNotSupportedException();

public static void ZeroFreeBSTR(IntPtr s)
public static unsafe void ZeroFreeBSTR(IntPtr s)
{
if (s == IntPtr.Zero)
{
return;
}
RuntimeImports.RhZeroMemory(s, (UIntPtr)SysStringByteLen(s));
Buffer.ZeroMemory((byte*)s, SysStringByteLen(s));
FreeBSTR(s);
}

Expand All @@ -936,7 +936,7 @@ public static unsafe void ZeroFreeCoTaskMemUnicode(IntPtr s)
{
return;
}
RuntimeImports.RhZeroMemory(s, (UIntPtr)(string.wcslen((char*)s) * 2));
Buffer.ZeroMemory((byte*)s, (nuint)string.wcslen((char*)s) * sizeof(char));
FreeCoTaskMem(s);
}

Expand All @@ -946,7 +946,7 @@ public static unsafe void ZeroFreeCoTaskMemUTF8(IntPtr s)
{
return;
}
RuntimeImports.RhZeroMemory(s, (UIntPtr)string.strlen((byte*)s));
Buffer.ZeroMemory((byte*)s, (nuint)string.strlen((byte*)s));
FreeCoTaskMem(s);
}

Expand All @@ -956,7 +956,7 @@ public static unsafe void ZeroFreeGlobalAllocAnsi(IntPtr s)
{
return;
}
RuntimeImports.RhZeroMemory(s, (UIntPtr)string.strlen((byte*)s));
Buffer.ZeroMemory((byte*)s, (nuint)string.strlen((byte*)s));
FreeHGlobal(s);
}

Expand All @@ -966,7 +966,7 @@ public static unsafe void ZeroFreeGlobalAllocUnicode(IntPtr s)
{
return;
}
RuntimeImports.RhZeroMemory(s, (UIntPtr)(string.wcslen((char*)s) * 2));
Buffer.ZeroMemory((byte*)s, (nuint)string.wcslen((char*)s) * sizeof(char));
FreeHGlobal(s);
}

Expand Down
Loading

0 comments on commit 17cbbee

Please sign in to comment.