diff --git a/WhiteMagic/ExternalReader.cs b/WhiteMagic/ExternalReader.cs index b0d6826..62d2979 100644 --- a/WhiteMagic/ExternalReader.cs +++ b/WhiteMagic/ExternalReader.cs @@ -15,13 +15,23 @@ public sealed class ExternalReader : MemoryBase private readonly IntPtr _imageBase; private bool _disposed; + /// + /// The default access rights: enough to read, write, allocate, query, run a remote + /// thread, and wait on it. This deliberately omits , + /// which over-requests and makes OpenProcess fail on protected processes where + /// these narrower rights would succeed. + /// + public const ProcessAccess DefaultAccess = + ProcessAccess.VmRead | ProcessAccess.VmWrite | ProcessAccess.VmOperation + | ProcessAccess.QueryInformation | ProcessAccess.CreateThread | ProcessAccess.Synchronize; + /// /// Opens a process for external memory access. /// /// The target process. /// The access rights to request. Defaults to - /// . - public ExternalReader(Process process, ProcessAccess desiredAccess = ProcessAccess.AllAccess) + /// . + 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; + } } /// diff --git a/WhiteMagic/MarshalCache.cs b/WhiteMagic/MarshalCache.cs index 0b34953..49f1b59 100644 --- a/WhiteMagic/MarshalCache.cs +++ b/WhiteMagic/MarshalCache.cs @@ -19,10 +19,19 @@ public static class MarshalCache public static readonly uint SizeU; /// - /// when has at least one field - /// decorated with , meaning it cannot be copied - /// via a simple pointer dereference. + /// when cannot be copied through the + /// blittable path and must + /// use / instead. + /// This is the case when a top-level field carries , or + /// when contains a managed reference + /// (). /// + /// + /// The check inspects only top-level fields; a + /// 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. + /// public static readonly bool TypeRequiresMarshal; /// when is . @@ -46,6 +55,13 @@ public static class MarshalCache 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 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(); } } diff --git a/WhiteMagic/MemoryBase.cs b/WhiteMagic/MemoryBase.cs index 2b518fd..7e52137 100644 --- a/WhiteMagic/MemoryBase.cs +++ b/WhiteMagic/MemoryBase.cs @@ -76,12 +76,16 @@ public abstract class MemoryBase : IDisposable /// returns fewer bytes than expected. public T[] Read(IntPtr address, int count, bool isRelative = false) where T : struct { + ArgumentOutOfRangeException.ThrowIfNegative(count); + if (isRelative) address = GetAbsolute(address); int elementSize = MarshalCache.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.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 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; diff --git a/WhiteMagic/Native/NativeMethods.cs b/WhiteMagic/Native/NativeMethods.cs index bdc07e2..d6b3fe9 100644 --- a/WhiteMagic/Native/NativeMethods.cs +++ b/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); /// Sets a 64-bit thread context (AMD64). [LibraryImport("kernel32.dll", SetLastError = true)] @@ -128,9 +128,10 @@ internal static partial class NativeMethods IntPtr hModule, [MarshalAs(UnmanagedType.LPStr)] string lpProcName); - /// Waits until a thread exits and retrieves its exit code. + /// Waits until an object is signaled or the timeout elapses. Returns a + /// WAIT_* status (DWORD); WAIT_FAILED is 0xFFFFFFFF. [LibraryImport("kernel32.dll", SetLastError = true)] - internal static partial int WaitForSingleObject( + internal static partial uint WaitForSingleObject( SafeMemoryHandle handle, uint milliseconds); } diff --git a/WhiteMagicTest/MemoryHardeningTests.cs b/WhiteMagicTest/MemoryHardeningTests.cs new file mode 100644 index 0000000..acf96c2 --- /dev/null +++ b/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; + +/// +/// 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. +/// +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) ────────────────── + + [Fact] + public void MarshalCache_char_size_is_two_bytes() + { + Assert.Equal(2, MarshalCache.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(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(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.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(() => reader.Read(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(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); + } + + /// + /// A that serves bytes from an in-memory buffer and + /// caps every read to maxChunk bytes, to exercise partial-read handling. + /// The address is treated as a zero-based index into the buffer. + /// + 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 bytes, bool isRelative = false) + => throw new NotSupportedException(); + + public override void Dispose() { } + } +} + +/// +/// A struct that carries a managed reference. +/// reports , so it cannot travel the blittable read path. +/// +[StructLayout(LayoutKind.Sequential)] +public struct StructWithReference +{ + public int Id; + public string Name; +}