Remove or document Unsafe.AsPointer uses in core libraries (#99146)

* Remove or document Unsafe.AsPointer uses in System.Memory

- Also add internal helper method Unsafe.IsOpportunisticallyAligned
- And cast a pointer to IntPtr instead of int for a unit test

* Fix typo and cast size to unsigned

* Address feedback

* Changes for the rest of the core libraries

- Other than MethodHandle code for NativeAOT
- Add Unsafe.OpportunisticMisalignment

* Fix compile errors

* Add full CS8500 comment & fix compile error

Author: hamarb123

src/coreclr/System.Private.CoreLib/src/System/ArgIterator.cs

@@ -24,7 +24,7 @@ private struct SigPointer
         private int _remainingArgs;             // # of remaining args.
 
 #if TARGET_WINDOWS // Native Varargs are not supported on Unix
-        // ArgIterator is a ref struct. It does not require pinning.
+        // ArgIterator is a ref struct. It does not require pinning, therefore Unsafe.AsPointer is safe.
         // This method null checks the this pointer as a side-effect.
         private ArgIterator* ThisPtr => (ArgIterator*)Unsafe.AsPointer(ref _argCookie);
 

src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs

@@ -865,7 +865,9 @@ public static unsafe IReadOnlyDictionary<string, object> GetConfigurationVariabl
                 Configurations = new Dictionary<string, object>()
             };
 
-            _EnumerateConfigurationValues(Unsafe.AsPointer(ref context), &ConfigCallback);
+#pragma warning disable CS8500 // takes address of managed type
+            _EnumerateConfigurationValues(&context, &ConfigCallback);
+#pragma warning restore CS8500
             return context.Configurations!;
         }
 

src/coreclr/nativeaot/Common/src/Internal/Runtime/CompilerHelpers/StartupCodeHelpers.cs

@@ -206,11 +206,12 @@ private static unsafe object[] InitializeStatics(IntPtr gcStaticRegionStart, int
                 nint blockAddr = MethodTable.SupportsRelativePointers ? (nint)ReadRelPtr32(pBlock) : *pBlock;
                 if ((blockAddr & GCStaticRegionConstants.Uninitialized) == GCStaticRegionConstants.Uninitialized)
                 {
+#pragma warning disable CS8500 // takes address of managed type
                     object? obj = null;
                     RuntimeImports.RhAllocateNewObject(
                         new IntPtr(blockAddr & ~GCStaticRegionConstants.Mask),
                         (uint)GC_ALLOC_FLAGS.GC_ALLOC_PINNED_OBJECT_HEAP,
-                        Unsafe.AsPointer(ref obj));
+                        &obj);
                     if (obj == null)
                     {
                         RuntimeExceptionHelpers.FailFast("Failed allocating GC static bases");
@@ -232,7 +233,8 @@ private static unsafe object[] InitializeStatics(IntPtr gcStaticRegionStart, int
                     Unsafe.Add(ref rawSpineData, currentBase) = obj;
 
                     // Update the base pointer to point to the pinned object
-                    *pBlock = *(IntPtr*)Unsafe.AsPointer(ref obj);
+                    *pBlock = *(IntPtr*)&obj;
+#pragma warning restore CS8500
                 }
 
                 currentBase++;

src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs

@@ -695,7 +695,9 @@ public static unsafe IReadOnlyDictionary<string, object> GetConfigurationVariabl
                 Configurations = new Dictionary<string, object>()
             };
 
-            RuntimeImports.RhEnumerateConfigurationValues(Unsafe.AsPointer(ref context), &ConfigCallback);
+#pragma warning disable CS8500 // takes address of managed type
+            RuntimeImports.RhEnumerateConfigurationValues(&context, &ConfigCallback);
+#pragma warning restore CS8500
             return context.Configurations!;
         }
 
@@ -830,7 +832,9 @@ static T[] AllocateNewUninitializedArray(int length, bool pinned)
                     throw new OverflowException();
 
                 T[]? array = null;
-                RuntimeImports.RhAllocateNewArray(MethodTable.Of<T[]>(), (uint)length, (uint)flags, Unsafe.AsPointer(ref array));
+#pragma warning disable CS8500 // takes address of managed type
+                RuntimeImports.RhAllocateNewArray(MethodTable.Of<T[]>(), (uint)length, (uint)flags, &array);
+#pragma warning restore CS8500
                 if (array == null)
                     throw new OutOfMemoryException();
 
@@ -857,7 +861,9 @@ public static unsafe T[] AllocateArray<T>(int length, bool pinned = false)
                 throw new OverflowException();
 
             T[]? array = null;
-            RuntimeImports.RhAllocateNewArray(MethodTable.Of<T[]>(), (uint)length, (uint)flags, Unsafe.AsPointer(ref array));
+#pragma warning disable CS8500 // takes address of managed type
+            RuntimeImports.RhAllocateNewArray(MethodTable.Of<T[]>(), (uint)length, (uint)flags, &array);
+#pragma warning restore CS8500
             if (array == null)
                 throw new OutOfMemoryException();
 

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

@@ -350,9 +350,11 @@ private CompilationResult CompileMethodInternal(IMethodNode methodCodeNodeNeedin
             IntPtr exception;
             IntPtr nativeEntry;
             uint codeSize;
+#pragma warning disable CS8500 // takes address of managed type
             var result = JitCompileMethod(out exception,
-                    _jit, (IntPtr)Unsafe.AsPointer(ref _this), _unmanagedCallbacks,
+                    _jit, (IntPtr)(&_this), _unmanagedCallbacks,
                     ref methodInfo, (uint)CorJitFlag.CORJIT_FLAG_CALL_GETJITFLAGS, out nativeEntry, out codeSize);
+#pragma warning restore CS8500
             if (exception != IntPtr.Zero)
             {
                 if (_lastException != null)

src/installer/tests/Assets/Projects/HostApiInvokerApp/HostRuntimeContract.cs

@@ -46,8 +46,9 @@ private static void Test_get_runtime_property(string[] args)
 
             static string GetProperty(string name, host_runtime_contract contract)
             {
-                Span<byte> nameSpan = stackalloc byte[Encoding.UTF8.GetMaxByteCount(name.Length)];
-                byte* namePtr = (byte*)Unsafe.AsPointer(ref MemoryMarshal.GetReference(nameSpan));
+                int nameSize = Encoding.UTF8.GetMaxByteCount(name.Length);
+                byte* namePtr = stackalloc byte[nameSize];
+                Span<byte> nameSpan = new Span<byte>(namePtr, nameSize);
                 int nameLen = Encoding.UTF8.GetBytes(name, nameSpan);
                 nameSpan[nameLen] = 0;
 
@@ -86,8 +87,9 @@ public static void Test_bundle_probe(string[] args)
 
             unsafe static void Probe(host_runtime_contract contract, string path)
             {
-                Span<byte> pathSpan = stackalloc byte[Encoding.UTF8.GetMaxByteCount(path.Length)];
-                byte* pathPtr = (byte*)Unsafe.AsPointer(ref MemoryMarshal.GetReference(pathSpan));
+                int pathSize = Encoding.UTF8.GetMaxByteCount(path.Length);
+                byte* pathPtr = stackalloc byte[pathSize];
+                Span<byte> pathSpan = new Span<byte>(pathPtr, pathSize);
                 int pathLen = Encoding.UTF8.GetBytes(path, pathSpan);
                 pathSpan[pathLen] = 0;
 

src/libraries/Common/src/System/Number.NumberBuffer.cs

@@ -29,7 +29,9 @@ internal unsafe ref struct NumberBuffer
             public bool IsNegative;
             public bool HasNonZeroTail;
             public NumberBufferKind Kind;
-            public Span<byte> Digits;
+            public byte* DigitsPtr;
+            public int DigitsLength;
+            public readonly Span<byte> Digits => new Span<byte>(DigitsPtr, DigitsLength);
 
             public NumberBuffer(NumberBufferKind kind, byte* digits, int digitsLength) : this(kind, new Span<byte>(digits, digitsLength))
             {
@@ -48,7 +50,8 @@ public NumberBuffer(NumberBufferKind kind, Span<byte> digits)
                 IsNegative = false;
                 HasNonZeroTail = false;
                 Kind = kind;
-                Digits = digits;
+                DigitsPtr = (byte*)Unsafe.AsPointer(ref MemoryMarshal.GetReference(digits)); // Safe since memory must be fixed
+                DigitsLength = digits.Length;
 #if DEBUG
                 Digits.Fill(0xCC);
 #endif
@@ -83,13 +86,6 @@ public void CheckConsistency()
             }
 #pragma warning restore CA1822
 
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            public byte* GetDigitsPointer()
-            {
-                // This is safe to do since we are a ref struct
-                return (byte*)Unsafe.AsPointer(ref MemoryMarshal.GetReference(Digits));
-            }
-
             //
             // Code coverage note: This only exists so that Number displays nicely in the VS watch window. So yes, I know it works.
             //

src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs

@@ -424,14 +424,7 @@ public void MultipleCallsToGetSpan()
                 Assert.True(span.Length >= 256);
                 Span<T> newSpan = output.GetSpan();
                 Assert.Equal(span.Length, newSpan.Length);
-
-                unsafe
-                {
-                    void* pSpan = Unsafe.AsPointer(ref MemoryMarshal.GetReference(span));
-                    void* pNewSpan = Unsafe.AsPointer(ref MemoryMarshal.GetReference(newSpan));
-                    Assert.Equal((IntPtr)pSpan, (IntPtr)pNewSpan);
-                }
-
+                Assert.Equal(0, Unsafe.ByteOffset(ref MemoryMarshal.GetReference(span), ref MemoryMarshal.GetReference(newSpan)));
                 Assert.Equal(span.Length, output.GetSpan().Length);
             }
             finally

src/libraries/System.Memory/tests/MemoryMarshal/CreateFromPinnedArray.cs

@@ -190,15 +190,16 @@ public static unsafe void CreateFromPinnedArrayVerifyPinning()
             int[] pinnedArray = { 90, 91, 92, 93, 94, 95, 96, 97, 98 };
             GCHandle pinnedGCHandle = GCHandle.Alloc(pinnedArray, GCHandleType.Pinned);
 
+            // Unsafe.AsPointer is used to ensure we catch if the GC moves the memory
             Memory<int> pinnedMemory = MemoryMarshal.CreateFromPinnedArray(pinnedArray, 0, 2);
             void* pinnedPtr = Unsafe.AsPointer(ref MemoryMarshal.GetReference(pinnedMemory.Span));
             void* memoryHandlePinnedPtr = pinnedMemory.Pin().Pointer;
 
             GC.Collect();
             GC.Collect(2);
 
-            Assert.Equal((int)pinnedPtr, (int)Unsafe.AsPointer(ref MemoryMarshal.GetReference(pinnedMemory.Span)));
-            Assert.Equal((int)memoryHandlePinnedPtr, (int)pinnedGCHandle.AddrOfPinnedObject().ToPointer());
+            Assert.Equal((IntPtr)pinnedPtr, (IntPtr)Unsafe.AsPointer(ref MemoryMarshal.GetReference(pinnedMemory.Span)));
+            Assert.Equal((IntPtr)memoryHandlePinnedPtr, pinnedGCHandle.AddrOfPinnedObject());
 
             pinnedGCHandle.Free();
         }

src/libraries/System.Memory/tests/MemoryMarshal/GetArrayDataReference.cs

@@ -43,7 +43,7 @@ public static unsafe void GetArrayDataReference_EmptyInput_ReturnsRefToWhereFirs
 
             ref int theRef = ref MemoryMarshal.GetArrayDataReference(theArray);
 
-            Assert.True(Unsafe.AsPointer(ref theRef) != null);
+            Assert.False(Unsafe.IsNullRef(ref theRef));
             Assert.True(Unsafe.AreSame(ref theRef, ref MemoryMarshal.GetReference(theArray.AsSpan())));
 
             ref int theMdArrayRef = ref Unsafe.As<byte, int>(ref MemoryMarshal.GetArrayDataReference((Array)theArray)); // szarray passed to generalized Array helper

src/libraries/System.Memory/tests/MemoryPool/MemoryPool.cs

@@ -52,6 +52,7 @@ public static void MemoryPoolSpan()
                 {
                     unsafe
                     {
+                        // Unsafe.AsPointer is safe here since it's pinned
                         void* pSpan = Unsafe.AsPointer(ref MemoryMarshal.GetReference(sp));
                         Assert.Equal((IntPtr)newMemoryHandle.Pointer, (IntPtr)pSpan);
                     }
@@ -77,6 +78,7 @@ public static void MemoryPoolPin(int elementIndex)
                 {
                     unsafe
                     {
+                        // Unsafe.AsPointer is safe here since it's pinned
                         void* pSpan = Unsafe.AsPointer(ref MemoryMarshal.GetReference(sp.Slice(elementIndex)));
                         Assert.Equal((IntPtr)pSpan, ((IntPtr)newMemoryHandle.Pointer));
                     }
@@ -112,6 +114,7 @@ public static void MemoryPoolPinOffsetAtEnd()
             {
                 unsafe
                 {
+                    // Unsafe.AsPointer is safe here since it's pinned
                     void* pSpan = Unsafe.AsPointer(ref MemoryMarshal.GetReference(sp.Slice(elementIndex)));
                     Assert.Equal((IntPtr)pSpan, ((IntPtr)newMemoryHandle.Pointer));
                 }
@@ -219,11 +222,7 @@ public static void MemoryPoolTryGetArray()
                 unsafe
                 {
                     Assert.True(MemoryMarshal.TryGetArray(memory, out arraySegment));
-                    fixed (int* pArray = arraySegment.Array)
-                    {
-                        void* pSpan = Unsafe.AsPointer(ref MemoryMarshal.GetReference(memory.Span));
-                        Assert.Equal((IntPtr)pSpan, (IntPtr)pArray);
-                    }
+                    Assert.Equal(0, Unsafe.ByteOffset(ref MemoryMarshal.GetArrayDataReference(arraySegment.Array), ref MemoryMarshal.GetReference(memory.Span)));
                 }
             }
         }

src/libraries/System.Memory/tests/ReadOnlySpan/AsSpan.cs

@@ -91,6 +91,7 @@ static unsafe void Validate(string text, int start, int length, ReadOnlySpan<cha
                 Assert.Equal(length, span.Length);
                 fixed (char* pText = text)
                 {
+                    // Unsafe.AsPointer is safe here since it's pinned (since text and span should be the same string)
                     char* expected = pText + start;
                     void* actual = Unsafe.AsPointer(ref MemoryMarshal.GetReference(span));
                     Assert.Equal((IntPtr)expected, (IntPtr)actual);

src/libraries/System.Memory/tests/TestHelpers.cs

@@ -37,7 +37,7 @@ public static unsafe void ValidateNonNullEmpty<T>(this Span<T> span)
             Assert.True(span.IsEmpty);
 
             // Validate that empty Span is not normalized to null
-            Assert.True(Unsafe.AsPointer(ref MemoryMarshal.GetReference(span)) != null);
+            Assert.False(Unsafe.IsNullRef(ref MemoryMarshal.GetReference(span)));
         }
 
         public delegate void AssertThrowsAction<T>(Span<T> span);
@@ -98,7 +98,7 @@ public static unsafe void ValidateNonNullEmpty<T>(this ReadOnlySpan<T> span)
             Assert.True(span.IsEmpty);
 
             // Validate that empty Span is not normalized to null
-            Assert.True(Unsafe.AsPointer(ref MemoryMarshal.GetReference(span)) != null);
+            Assert.False(Unsafe.IsNullRef(ref MemoryMarshal.GetReference(span)));
         }
 
         public delegate void AssertThrowsActionReadOnly<T>(ReadOnlySpan<T> span);

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs

@@ -1120,6 +1120,14 @@ protected unsafe void WriteEvent(int eventId, long arg1, byte[]? arg2)
             }
         }
 
+        // Returns the object as a IntPtr - safe when only used for logging
+        internal static unsafe nint ObjectIDForEvents(object? o)
+        {
+#pragma warning disable CS8500 // takes address of managed type
+            return *(nint*)&o;
+#pragma warning restore CS8500
+        }
+
 #pragma warning restore 1591
 
         /// <summary>

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.Threading.NativeSinks.cs

@@ -94,7 +94,7 @@ private void ContentionLockCreated(nint LockID, nint AssociatedObjectID, ushort
 
         [NonEvent]
         [MethodImpl(MethodImplOptions.NoInlining)]
-        public void ContentionLockCreated(Lock lockObj) => ContentionLockCreated(lockObj.LockIdForEvents, lockObj.ObjectIdForEvents);
+        public void ContentionLockCreated(Lock lockObj) => ContentionLockCreated(lockObj.LockIdForEvents, ObjectIDForEvents(lockObj));
 
         [Event(81, Level = EventLevel.Informational, Message = Messages.ContentionStart, Task = Tasks.Contention, Opcode = EventOpcode.Start, Version = 2, Keywords = Keywords.ContentionKeyword)]
         private void ContentionStart(
@@ -115,7 +115,7 @@ public void ContentionStart(Lock lockObj) =>
                 ContentionFlagsMap.Managed,
                 DefaultClrInstanceId,
                 lockObj.LockIdForEvents,
-                lockObj.ObjectIdForEvents,
+                ObjectIDForEvents(lockObj),
                 lockObj.OwningThreadId);
 
         [Event(91, Level = EventLevel.Informational, Message = Messages.ContentionStop, Task = Tasks.Contention, Opcode = EventOpcode.Stop, Version = 1, Keywords = Keywords.ContentionKeyword)]
@@ -360,7 +360,7 @@ private void WaitHandleWaitStart(
         public unsafe void WaitHandleWaitStart(
             WaitHandleWaitSourceMap waitSource = WaitHandleWaitSourceMap.Unknown,
             object? associatedObject = null) =>
-            WaitHandleWaitStart(waitSource, *(nint*)Unsafe.AsPointer(ref associatedObject));
+            WaitHandleWaitStart(waitSource, ObjectIDForEvents(associatedObject));
 
         [Event(302, Level = EventLevel.Verbose, Message = Messages.WaitHandleWaitStop, Task = Tasks.WaitHandleWait, Opcode = EventOpcode.Stop, Version = 0, Keywords = Keywords.WaitHandleKeyword)]
         public void WaitHandleWaitStop(ushort ClrInstanceID = DefaultClrInstanceId)

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.Threading.cs

@@ -107,7 +107,7 @@ private unsafe void ContentionLockCreated(nint LockID, nint AssociatedObjectID,
 
         [NonEvent]
         [MethodImpl(MethodImplOptions.NoInlining)]
-        public void ContentionLockCreated(Lock lockObj) => ContentionLockCreated(lockObj.LockIdForEvents, lockObj.ObjectIdForEvents);
+        public void ContentionLockCreated(Lock lockObj) => ContentionLockCreated(lockObj.LockIdForEvents, ObjectIDForEvents(lockObj));
 
         [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern", Justification = "Parameters to this method are primitive and are trimmer safe")]
         [Event(81, Level = EventLevel.Informational, Message = Messages.ContentionStart, Task = Tasks.Contention, Opcode = EventOpcode.Start, Version = 2, Keywords = Keywords.ContentionKeyword)]
@@ -146,7 +146,7 @@ public void ContentionStart(Lock lockObj) =>
                 ContentionFlagsMap.Managed,
                 DefaultClrInstanceId,
                 lockObj.LockIdForEvents,
-                lockObj.ObjectIdForEvents,
+                ObjectIDForEvents(lockObj),
                 lockObj.OwningThreadId);
 
         [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern", Justification = "Parameters to this method are primitive and are trimmer safe")]
@@ -557,7 +557,7 @@ private unsafe void WaitHandleWaitStart(
         public unsafe void WaitHandleWaitStart(
             WaitHandleWaitSourceMap waitSource = WaitHandleWaitSourceMap.Unknown,
             object? associatedObject = null) =>
-            WaitHandleWaitStart(waitSource, *(nint*)Unsafe.AsPointer(ref associatedObject));
+            WaitHandleWaitStart(waitSource, ObjectIDForEvents(associatedObject));
 
         [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern", Justification = "Parameters to this method are primitive and are trimmer safe")]
         [Event(302, Level = EventLevel.Verbose, Message = Messages.WaitHandleWaitStop, Task = Tasks.WaitHandleWait, Opcode = EventOpcode.Stop, Version = 0, Keywords = Keywords.WaitHandleKeyword)]