Harden memory layer: fix char sizing, ref structs, count guards, ReadString advance

Second-review fixes, each covered by a regression test in MemoryHardeningTests:

- MarshalCache: special-case char (Size=2; Marshal.SizeOf reports 1/ANSI but the
  blittable path reads a 2-byte UTF-16 unit). TypeRequiresMarshal now also trips on
  RuntimeHelpers.IsReferenceOrContainsReferences<T>() so reference-carrying structs
  route to the marshal path instead of throwing in MemoryMarshal.Read. Document that
  the MarshalAs scan is top-level only.
- MemoryBase.Read<T>(count): reject negative count (ArgumentOutOfRangeException) and
  guard elementSize*count overflow. Same overflow guard on Write<T>(values).
- MemoryBase.ReadString: advance by bytes actually read, not the requested amount, so
  a partial read no longer skips the unread tail of the window.
- ExternalReader: default to a minimal access set (not AllAccess, which over-requests
  and fails on protected processes); wrap Process.MainModule in try/catch so a
  bitness-mismatched or protected target yields ImageBase=Zero instead of throwing.
- NativeMethods: WaitForSingleObject and CreateRemoteThread's threadId are DWORD (uint),
  not int — the signatures no longer sign-flip.

Deferred: hoisting the identical ExternalReader/InProcessReader byte-IO into MemoryBase
(cosmetic; skipped to avoid colliding with concurrent Phase 3 edits).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

WhiteMagic/ExternalReader.cs

@@ -15,13 +15,23 @@ public sealed class ExternalReader : MemoryBase
     private readonly IntPtr _imageBase;
     private bool _disposed;
 
+    /// <summary>
+    /// The default access rights: enough to read, write, allocate, query, run a remote
+    /// thread, and wait on it. This deliberately omits <see cref="ProcessAccess.AllAccess"/>,
+    /// which over-requests and makes <c>OpenProcess</c> fail on protected processes where
+    /// these narrower rights would succeed.
+    /// </summary>
+    public const ProcessAccess DefaultAccess =
+        ProcessAccess.VmRead | ProcessAccess.VmWrite | ProcessAccess.VmOperation
+        | ProcessAccess.QueryInformation | ProcessAccess.CreateThread | ProcessAccess.Synchronize;
+
     /// <summary>
     /// Opens a process for external memory access.
     /// </summary>
     /// <param name="process">The target process.</param>
     /// <param name="desiredAccess">The access rights to request. Defaults to
-    /// <see cref="ProcessAccess.AllAccess"/>.</param>
-    public ExternalReader(Process process, ProcessAccess desiredAccess = ProcessAccess.AllAccess)
+    /// <see cref="DefaultAccess"/>.</param>
+    public ExternalReader(Process process, ProcessAccess desiredAccess = DefaultAccess)
     {
         _handle = NativeMethods.OpenProcess(desiredAccess, false, process.Id);
         if (_handle.IsInvalid)
@@ -31,7 +41,16 @@ public sealed class ExternalReader : MemoryBase
                 $"OpenProcess failed for PID {process.Id}: error {error}");
         }
 
-        _imageBase = process.MainModule?.BaseAddress ?? IntPtr.Zero;
+        // Process.MainModule throws Win32Exception for a bitness-mismatched or protected
+        // target; a missing image base must not sink the whole reader.
+        try
+        {
+            _imageBase = process.MainModule?.BaseAddress ?? IntPtr.Zero;
+        }
+        catch (System.ComponentModel.Win32Exception)
+        {
+            _imageBase = IntPtr.Zero;
+        }
     }
 
     /// <inheritdoc />

WhiteMagic/MarshalCache.cs

@@ -19,10 +19,19 @@ public static class MarshalCache<T>
     public static readonly uint SizeU;
 
     /// <summary>
-    /// <see langword="true"/> when <typeparamref name="T"/> has at least one field
-    /// decorated with <see cref="MarshalAsAttribute"/>, meaning it cannot be copied
-    /// via a simple pointer dereference.
+    /// <see langword="true"/> when <typeparamref name="T"/> cannot be copied through the
+    /// blittable <see cref="System.Runtime.InteropServices.MemoryMarshal"/> path and must
+    /// use <see cref="Marshal.PtrToStructure"/>/<see cref="Marshal.StructureToPtr"/> instead.
+    /// This is the case when a top-level field carries <see cref="MarshalAsAttribute"/>, or
+    /// when <typeparamref name="T"/> contains a managed reference
+    /// (<see cref="System.Runtime.CompilerServices.RuntimeHelpers.IsReferenceOrContainsReferences{T}"/>).
     /// </summary>
+    /// <remarks>
+    /// The <see cref="MarshalAsAttribute"/> check inspects only top-level fields; a
+    /// <see cref="MarshalAsAttribute"/> on a field of a nested struct is not detected.
+    /// Reference-containing nested structs are still caught, because the reference check
+    /// propagates through nested value types.
+    /// </remarks>
     public static readonly bool TypeRequiresMarshal;
 
     /// <summary><see langword="true"/> when <typeparamref name="T"/> is <see cref="IntPtr"/>.</summary>
@@ -46,6 +55,13 @@ public static class MarshalCache<T>
             Size = 1;
             RealType = typeof(T);
         }
+        else if (typeof(T) == typeof(char))
+        {
+            // Marshal.SizeOf(char) is 1 (ANSI), but the blittable path reads/writes a
+            // char as a 2-byte UTF-16 code unit. Size must match the blittable width.
+            Size = 2;
+            RealType = typeof(T);
+        }
         else if (typeof(T).IsEnum)
         {
             Type underlying = typeof(T).GetEnumUnderlyingType();
@@ -62,8 +78,11 @@ public static class MarshalCache<T>
         SizeU = (uint)Size;
         IsIntPtr = RealType == typeof(IntPtr);
 
-        TypeRequiresMarshal =
+        bool hasMarshalAsField =
             RealType.GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic)
                 .Any(f => f.GetCustomAttributes(typeof(MarshalAsAttribute), true).Length != 0);
+
+        TypeRequiresMarshal =
+            hasMarshalAsField || System.Runtime.CompilerServices.RuntimeHelpers.IsReferenceOrContainsReferences<T>();
     }
 }

WhiteMagic/MemoryBase.cs

@@ -76,12 +76,16 @@ public abstract class MemoryBase : IDisposable
     /// returns fewer bytes than expected.</returns>
     public T[] Read<T>(IntPtr address, int count, bool isRelative = false) where T : struct
     {
+        ArgumentOutOfRangeException.ThrowIfNegative(count);
+
         if (isRelative)
             address = GetAbsolute(address);
 
         int elementSize = MarshalCache<T>.Size;
-        int totalSize = elementSize * count;
-        byte[] raw = ReadBytes(address, totalSize);
+        long totalSize = (long)elementSize * count;
+        ArgumentOutOfRangeException.ThrowIfGreaterThan(totalSize, int.MaxValue, nameof(count));
+
+        byte[] raw = ReadBytes(address, (int)totalSize);
         int actualCount = Math.Min(count, raw.Length / elementSize);
 
         var result = new T[actualCount];
@@ -124,7 +128,9 @@ public abstract class MemoryBase : IDisposable
             return true;
 
         int elementSize = MarshalCache<T>.Size;
-        int totalSize = elementSize * values.Length;
+        long total = (long)elementSize * values.Length;
+        ArgumentOutOfRangeException.ThrowIfGreaterThan(total, int.MaxValue, nameof(values));
+        int totalSize = (int)total;
         byte[] raw = new byte[totalSize];
 
         Span<byte> span = raw;
@@ -181,8 +187,10 @@ public abstract class MemoryBase : IDisposable
             }
 
             accumulated.Add(chunk);
-            address += take;
-            remaining -= take;
+            // Advance by the bytes actually read, not the amount requested: a partial
+            // read (chunk.Length < take) must not skip the unread tail of the window.
+            address += chunk.Length;
+            remaining -= chunk.Length;
         }
 
         int totalLength = 0;

WhiteMagic/Native/NativeMethods.cs

@@ -85,7 +85,7 @@ internal static partial class NativeMethods
         IntPtr startAddress,
         IntPtr parameter,
         ThreadCreationFlags creationFlags,
-        out int threadId);
+        out uint threadId);
 
     /// <summary>Sets a 64-bit thread context (AMD64).</summary>
     [LibraryImport("kernel32.dll", SetLastError = true)]
@@ -128,9 +128,10 @@ internal static partial class NativeMethods
         IntPtr hModule,
         [MarshalAs(UnmanagedType.LPStr)] string lpProcName);
 
-    /// <summary>Waits until a thread exits and retrieves its exit code.</summary>
+    /// <summary>Waits until an object is signaled or the timeout elapses. Returns a
+    /// <c>WAIT_*</c> status (DWORD); <c>WAIT_FAILED</c> is <c>0xFFFFFFFF</c>.</summary>
     [LibraryImport("kernel32.dll", SetLastError = true)]
-    internal static partial int WaitForSingleObject(
+    internal static partial uint WaitForSingleObject(
         SafeMemoryHandle handle,
         uint milliseconds);
 }

WhiteMagicTest/MemoryHardeningTests.cs

@@ -0,0 +1,189 @@
+using System.Diagnostics;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+using System.Text;
+using WhiteMagic;
+using WhiteMagic.Native;
+
+namespace WhiteMagicTest;
+
+/// <summary>
+/// Regression tests for the edge-case defects found in the second review pass:
+/// char sizing, reference-containing structs, unchecked counts, the ReadString
+/// partial-chunk skip, and ExternalReader construction against awkward targets.
+/// Each test fails against the pre-fix code.
+/// </summary>
+public class MemoryHardeningTests
+{
+    private static ExternalReader OpenSelf()
+    {
+        return new ExternalReader(
+            Process.GetCurrentProcess(),
+            ProcessAccess.VmRead | ProcessAccess.VmWrite | ProcessAccess.VmOperation | ProcessAccess.QueryInformation);
+    }
+
+    // ── char sizing (MarshalCache / MemoryBase.Read<char>) ──────────────────
+
+    [Fact]
+    public void MarshalCache_char_size_is_two_bytes()
+    {
+        Assert.Equal(2, MarshalCache<char>.Size);
+    }
+
+    [Fact]
+    public void Read_char_writes_and_reads_back()
+    {
+        using var reader = OpenSelf();
+        char slot = '\0';
+        GCHandle pin = GCHandle.Alloc(slot, GCHandleType.Pinned);
+        try
+        {
+            IntPtr addr = pin.AddrOfPinnedObject();
+            Assert.True(reader.Write(addr, 'Z'));
+            Assert.Equal('Z', reader.Read<char>(addr));
+        }
+        finally
+        {
+            pin.Free();
+        }
+    }
+
+    [Fact]
+    public void Read_char_array_writes_and_reads_back()
+    {
+        using var reader = OpenSelf();
+        char[] slot = new char[4];
+        GCHandle pin = GCHandle.Alloc(slot, GCHandleType.Pinned);
+        try
+        {
+            IntPtr addr = pin.AddrOfPinnedObject();
+            char[] expected = ['w', 'o', 'w', '!'];
+            Assert.True(reader.Write(addr, expected));
+            Assert.Equal(expected, reader.Read<char>(addr, 4));
+        }
+        finally
+        {
+            pin.Free();
+        }
+    }
+
+    // ── reference-containing structs route to the marshal path ──────────────
+
+    [Fact]
+    public void MarshalCache_flags_struct_with_reference_field_as_marshal_required()
+    {
+        // Has a string field but no [MarshalAs]; the blittable path (MemoryMarshal.Read)
+        // throws for a reference-containing T, so the cache must route it to the marshal
+        // path. A struct with a managed reference cannot be pinned, so the routing flag —
+        // not a live round-trip — is the regression guard here.
+        Assert.True(MarshalCache<StructWithReference>.TypeRequiresMarshal);
+    }
+
+    // ── unchecked count guards ──────────────────────────────────────────────
+
+    [Fact]
+    public void Read_array_with_negative_count_throws_argument_out_of_range()
+    {
+        using var reader = OpenSelf();
+        int dummy = 0;
+        GCHandle pin = GCHandle.Alloc(dummy, GCHandleType.Pinned);
+        try
+        {
+            Assert.Throws<ArgumentOutOfRangeException>(() => reader.Read<int>(pin.AddrOfPinnedObject(), -1));
+        }
+        finally
+        {
+            pin.Free();
+        }
+    }
+
+    [Fact]
+    public void Read_array_with_zero_count_returns_empty()
+    {
+        using var reader = OpenSelf();
+        int dummy = 0;
+        GCHandle pin = GCHandle.Alloc(dummy, GCHandleType.Pinned);
+        try
+        {
+            Assert.Empty(reader.Read<int>(pin.AddrOfPinnedObject(), 0));
+        }
+        finally
+        {
+            pin.Free();
+        }
+    }
+
+    // ── ReadString partial-chunk advance ────────────────────────────────────
+
+    [Fact]
+    public void ReadString_advances_by_actual_bytes_when_reads_are_partial()
+    {
+        // The reader serves at most 3 bytes per call. The string is longer than one
+        // chunk with the terminator well past it. If ReadString advanced by the
+        // requested count instead of the bytes actually returned, it would skip
+        // data and truncate the result.
+        byte[] data = Encoding.ASCII.GetBytes("ABCDEFGHIJ\0");
+        var reader = new PartialReader(data, maxChunk: 3);
+
+        string result = reader.ReadString(IntPtr.Zero, Encoding.ASCII, maxLength: 64);
+
+        Assert.Equal("ABCDEFGHIJ", result);
+    }
+
+    [Fact]
+    public void ReadString_stops_at_null_across_partial_chunks()
+    {
+        byte[] data = Encoding.ASCII.GetBytes("hi\0garbage");
+        var reader = new PartialReader(data, maxChunk: 1);
+
+        string result = reader.ReadString(IntPtr.Zero, Encoding.ASCII, maxLength: 64);
+
+        Assert.Equal("hi", result);
+    }
+
+    // ── ExternalReader construction ─────────────────────────────────────────
+
+    [Fact]
+    public void ExternalReader_opens_self_with_default_access()
+    {
+        // The default access set must be small enough to open a normal process.
+        using var reader = new ExternalReader(Process.GetCurrentProcess());
+        Assert.False(reader.Handle.IsInvalid);
+    }
+
+    /// <summary>
+    /// A <see cref="MemoryBase"/> that serves bytes from an in-memory buffer and
+    /// caps every read to <c>maxChunk</c> bytes, to exercise partial-read handling.
+    /// The address is treated as a zero-based index into the buffer.
+    /// </summary>
+    private sealed class PartialReader(byte[] data, int maxChunk) : MemoryBase
+    {
+        public override IntPtr ImageBase => IntPtr.Zero;
+        public override SafeMemoryHandle Handle => null!;
+
+        public override byte[] ReadBytes(IntPtr address, int count, bool isRelative = false)
+        {
+            int start = (int)address;
+            if (start < 0 || start >= data.Length || count <= 0)
+                return [];
+            int n = Math.Min(Math.Min(count, maxChunk), data.Length - start);
+            return data[start..(start + n)];
+        }
+
+        public override int WriteBytes(IntPtr address, ReadOnlySpan<byte> bytes, bool isRelative = false)
+            => throw new NotSupportedException();
+
+        public override void Dispose() { }
+    }
+}
+
+/// <summary>
+/// A struct that carries a managed reference. <see cref="RuntimeHelpers.IsReferenceOrContainsReferences{T}"/>
+/// reports <see langword="true"/>, so it cannot travel the blittable read path.
+/// </summary>
+[StructLayout(LayoutKind.Sequential)]
+public struct StructWithReference
+{
+    public int Id;
+    public string Name;
+}