[release/6.0.4xx-xcode14.1] [Foundation] Fix random memory access / m…

…emory corruption in NSFastEnumerator. Fixes #xamarin/maccore@1347. (#16943)

The NSFastEnumerator protocol in Objective-C works like this:

1. A selector is called on the collection in question, where we give the
   native method a pointer to a state structure. Upon return, the state
   structure will have two important pointers: one to a C array with the
   pointers to enumerate, and another pointer that points to a value
   determining whether the collection was modified since enumeration started.
2. In the original managed implementation, we'd store the state (a struct) as
   an instance field of NSFastEnumerator.

This works fine... most of the time. Unfortunately most of the time isn't good
enough, because this may happen:

a. The native iterator function might store a pointer into the state structure
   itself as the pointer to check for a modified collection.
b. The GC runs, and moves memory around.

Now suddenly the pointer we have to read to check if the collection was
modified is pointing to the previous location of the state structure, which
could be anything or anywhere, and the app subsequently tries to summon Murphy
from his pantheon in the sky (admittedly with limited success).

The fix is to use native memory (which the GC won't move around) to store the
enumeration state.

Also optimize memory usage a little bit by allocating one big blob for both
the state and the array of pointers we pass to the native iterator method.

Fixes xamarin/maccore#1347.
Fixes xamarin/maccore#2606.

Backport of #16920.

src/Foundation/NSFastEnumerationState.cs

@@ -7,6 +7,8 @@
 // Copyright 2015, Xamarin Inc.
 //
 
+#nullable enable
+
 using System;
 using System.Collections.Generic;
 using System.Runtime.InteropServices;
@@ -17,12 +19,33 @@ namespace Foundation {
 	[StructLayout (LayoutKind.Sequential)]
 	internal struct NSFastEnumerationState {
 		nint state;
-		internal IntPtr itemsPtr;
-		internal IntPtr mutationsPtr;
+		unsafe internal IntPtr* itemsPtr;
+		unsafe internal IntPtr* mutationsPtr;
 		nint extra1;
 		nint extra2;
 		nint extra3;
 		nint extra4;
 		nint extra5;
+
+		// An array where the enumerator might store stuff.
+		// This isn't part of the native declaration of NSFastEnumerationState,
+		// we've added it to simplify our enumeration code.
+		internal const int ArrayLength = 16;
+		internal IntPtr array1;
+		IntPtr array2;
+		IntPtr array3;
+		IntPtr array4;
+		IntPtr array5;
+		IntPtr array6;
+		IntPtr array7;
+		IntPtr array8;
+		IntPtr array9;
+		IntPtr array10;
+		IntPtr array11;
+		IntPtr array12;
+		IntPtr array13;
+		IntPtr array14;
+		IntPtr array15;
+		IntPtr array16;
 	}
 }

src/Foundation/NSFastEnumerator.cs

@@ -17,15 +17,14 @@
 namespace Foundation {
 	internal class NSFastEnumerator {
 		[DllImport (Messaging.LIBOBJC_DYLIB, EntryPoint="objc_msgSend")]
-		public extern static nuint objc_msgSend (IntPtr receiver, IntPtr selector, ref NSFastEnumerationState arg1, IntPtr[] arg2, nuint arg3);
+		public unsafe extern static nuint objc_msgSend (IntPtr receiver, IntPtr selector, NSFastEnumerationState* arg1, IntPtr* arg2, nuint arg3);
 	}
 
 	internal class NSFastEnumerator<T> : IEnumerator<T>
 		where T: class, INativeObject
 	{
-		NSFastEnumerationState state;
+		unsafe NSFastEnumerationState* state;
 		NSObject collection;
-		IntPtr[] array;
 		nuint count;
 		IntPtr mutationValue;
 		nuint current;
@@ -34,30 +33,46 @@ internal class NSFastEnumerator<T> : IEnumerator<T>
 		public NSFastEnumerator (NSObject collection)
 		{
 			this.collection = collection;
+
+			unsafe {
+				// Create one blob of native memory that holds both our NSFastEnumerationState and the array of pointers we pass to the enumerator.
+				//
+				// Note that we *must* pass native memory to the countByEnumeratingWithState:objects:count: method
+				// (and not a field on the NSFastEnumerator instance), because:
+				// * The pointers in the state (NSFastEnumerationState.mutationsPtr / NSFastEnumerationState.itemsPtr) might point back into the structure.
+				// * We access those pointers using unsafe code (in a way the GC doesn't see).
+				// * If the GC happens to move the NSFastEnumerator instance in memory, it won't update these pointers.
+				// * The next time we read these pointers, we'll read random memory, and thus get random results.
+				// * Ref: https://github.com/xamarin/maccore/issues/2606.
+				// * It would probably also work to create a pinned GCHandle to the NSFastEnumerator structure (instead of allocating native memory), but that doesn't seem easier on the GC.
+				state = (NSFastEnumerationState*) Marshal.AllocHGlobal (sizeof (NSFastEnumerationState));
+				// Zero-initialize
+				*state = default (NSFastEnumerationState);
+			}
 		}
 
 		void Fetch ()
 		{
-			if (array == null)
-				array = new IntPtr [16];
-			count = NSFastEnumerator.objc_msgSend (collection.Handle, Selector.GetHandle ("countByEnumeratingWithState:objects:count:"), ref state, array, (nuint) array.Length);
-			if (!started) {
-				started = true;
-				mutationValue = Marshal.ReadIntPtr (state.mutationsPtr);
+			unsafe {
+				count = NSFastEnumerator.objc_msgSend (collection.Handle, Selector.GetHandle ("countByEnumeratingWithState:objects:count:"), state, &state->array1, (nuint) NSFastEnumerationState.ArrayLength);
+				if (!started) {
+					started = true;
+					mutationValue = *state->mutationsPtr;
+				}
 			}
 			current = 0;
 		}
 
-		void VerifyNonMutated ()
+		unsafe void VerifyNonMutated ()
 		{
-			if (mutationValue != Marshal.ReadIntPtr (state.mutationsPtr))
-				throw new InvalidOperationException ("Collection was modified"); 
+			if (mutationValue != *state->mutationsPtr)
+				throw new InvalidOperationException ("Collection was modified");
 		}
 
 #region IEnumerator implementation
 		bool System.Collections.IEnumerator.MoveNext ()
 		{
-			if (array == null || current == count - 1) {
+			if (!started || current == count - 1) {
 				Fetch ();
 				if (count == 0)
 					return false;
@@ -70,7 +85,9 @@ bool System.Collections.IEnumerator.MoveNext ()
 
 		void System.Collections.IEnumerator.Reset ()
 		{
-			state = new NSFastEnumerationState ();
+			unsafe {
+				*state = new NSFastEnumerationState ();
+			}
 			started = false;
 		}
 
@@ -85,14 +102,21 @@ object System.Collections.IEnumerator.Current {
 #region IDisposable implementation
 		void IDisposable.Dispose ()
 		{
-			// Nothing to do
+			unsafe {
+				Marshal.FreeHGlobal ((IntPtr) state);
+				state = null;
+			}
 		}
 #endregion
 
 #region IEnumerator<T> implementation
-		public T Current {
+		public unsafe T Current {
 			get {
-				return Runtime.GetINativeObject<T> (Marshal.ReadIntPtr (state.itemsPtr, IntPtr.Size * (int) current), false);
+				IntPtr ptr;
+				unsafe {
+					ptr = state->itemsPtr [(int) current];
+				}
+				return Runtime.GetINativeObject<T> (ptr, false);
 			}
 		}
 #endregion