From 1169fdb994411b52e705825bd4ed9b30b05ffc3c Mon Sep 17 00:00:00 2001 From: Kevin Bataille Date: Wed, 22 Jul 2026 17:18:48 +0200 Subject: [PATCH] Address review findings for thread control and process discovery - Drop the false WOW64 claim from GetContext32/SetContext32 docs and guard them for 32-bit targets only.\n- Make FrozenThread dispose the thread handles it owns; make Freeze(predicate) dispose filtered-out threads.\n- Pass the already-validated handle through GetThreadById instead of opening a second one.\n- Add no-progress guard to MemoryBase.EnumerateRegions.\n- Dispose unmatched Process candidates in ApplicationFinder.OpenProcess.\n- Clean up RemoteThreadExecutor allocation formatting. --- WhiteMagic/Execution/RemoteThreadExecutor.cs | 3 +- WhiteMagic/MemoryBase.cs | 6 ++- WhiteMagic/Process/ApplicationFinder.cs | 13 ++++-- WhiteMagic/Thread/FrozenThread.cs | 13 +++--- WhiteMagic/Thread/RemoteThread.cs | 16 +++++-- WhiteMagic/Thread/ThreadFactory.cs | 46 +++++++++++++++++--- 6 files changed, 78 insertions(+), 19 deletions(-) diff --git a/WhiteMagic/Execution/RemoteThreadExecutor.cs b/WhiteMagic/Execution/RemoteThreadExecutor.cs index 425ad2b..d16f5ae 100644 --- a/WhiteMagic/Execution/RemoteThreadExecutor.cs +++ b/WhiteMagic/Execution/RemoteThreadExecutor.cs @@ -509,5 +509,6 @@ public sealed class RemoteThreadExecutor } } - return IntPtr.Zero; } + return IntPtr.Zero; + } } diff --git a/WhiteMagic/MemoryBase.cs b/WhiteMagic/MemoryBase.cs index d53b8a4..001f8c0 100644 --- a/WhiteMagic/MemoryBase.cs +++ b/WhiteMagic/MemoryBase.cs @@ -308,7 +308,11 @@ public abstract class MemoryBase : IDisposable yield break; yield return new MemoryRegion(info); - address = info.BaseAddress + (nint)info.RegionSize; + IntPtr next = info.BaseAddress + (nint)info.RegionSize; + if (next.ToInt64() <= address.ToInt64()) + yield break; + + address = next; } } diff --git a/WhiteMagic/Process/ApplicationFinder.cs b/WhiteMagic/Process/ApplicationFinder.cs index d8a97a4..1a0b470 100644 --- a/WhiteMagic/Process/ApplicationFinder.cs +++ b/WhiteMagic/Process/ApplicationFinder.cs @@ -41,12 +41,19 @@ public static class ApplicationFinder if (candidates.Length > 1) { + string list = string.Join(", ", candidates.Select(p => $"{p.ProcessName}:{p.Id}")); + foreach (Process candidate in candidates) + candidate.Dispose(); + throw new InvalidOperationException( - $"Process name '{processName}' is ambiguous ({candidates.Length} matches): " + - string.Join(", ", candidates.Select(p => $"{p.ProcessName}:{p.Id}"))); + $"Process name '{processName}' is ambiguous ({candidates.Length} matches): {list}"); } - return candidates[0]; + Process result = candidates[0]; + for (int i = 1; i < candidates.Length; i++) + candidates[i].Dispose(); + + return result; } /// diff --git a/WhiteMagic/Thread/FrozenThread.cs b/WhiteMagic/Thread/FrozenThread.cs index 25987e4..2718b4a 100644 --- a/WhiteMagic/Thread/FrozenThread.cs +++ b/WhiteMagic/Thread/FrozenThread.cs @@ -7,7 +7,7 @@ namespace WhiteMagic.Thread; /// /// A disposable scope that tracks a set of threads frozen by . /// Disposing the scope resumes exactly those threads, in reverse order, even if the guarded -/// body throws. +/// body throws, and then disposes the underlying thread handles. /// public sealed class FrozenThread : IDisposable { @@ -23,8 +23,7 @@ public sealed class FrozenThread : IDisposable public IEnumerable Threads => _threads; /// - /// Resumes the frozen threads in reverse order. The original call is responsible for - /// disposing the instances afterwards. + /// Resumes the frozen threads in reverse order, then disposes every thread handle. /// public void Dispose() { @@ -41,9 +40,13 @@ public sealed class FrozenThread : IDisposable } catch { - // Resume-on-dispose is best-effort; callers keep the thread handles so - // they can diagnose or recover separately. + // Resume-on-dispose is best-effort; the handle is still disposed below. } } + + foreach (RemoteThread thread in _threads) + { + thread.Dispose(); + } } } diff --git a/WhiteMagic/Thread/RemoteThread.cs b/WhiteMagic/Thread/RemoteThread.cs index d8bd8f4..86b68a5 100644 --- a/WhiteMagic/Thread/RemoteThread.cs +++ b/WhiteMagic/Thread/RemoteThread.cs @@ -139,11 +139,17 @@ public sealed class RemoteThread : IDisposable } /// - /// Reads the 32-bit native context of the thread. Valid for 32-bit targets or - /// WOW64 threads selected by a 64-bit caller. + /// Reads the 32-bit native context of the thread. Valid only for 32-bit targets. /// public void GetContext32(out Context32 context) { + if (_memory.Is64Bit) + { + context = default; + throw new InvalidOperationException( + "Use GetContext64 for 64-bit targets; GetContext32 is valid for 32-bit targets only."); + } + context = new Context32 { ContextFlags = ContextFlags.X86Full }; if (!NativeMethods.GetThreadContext(_handle, ref context)) { @@ -153,10 +159,14 @@ public sealed class RemoteThread : IDisposable } /// - /// Writes the 32-bit native context of the thread. + /// Writes the 32-bit native context of the thread. Valid only for 32-bit targets. /// public void SetContext32(ref Context32 context) { + if (_memory.Is64Bit) + throw new InvalidOperationException( + "Use SetContext64 for 64-bit targets; SetContext32 is valid for 32-bit targets only."); + if (!NativeMethods.SetThreadContext(_handle, ref context)) { int error = Marshal.GetLastPInvokeError(); diff --git a/WhiteMagic/Thread/ThreadFactory.cs b/WhiteMagic/Thread/ThreadFactory.cs index 7faf73e..d54b9fb 100644 --- a/WhiteMagic/Thread/ThreadFactory.cs +++ b/WhiteMagic/Thread/ThreadFactory.cs @@ -86,7 +86,13 @@ public sealed class ThreadFactory if (threadId <= 0) throw new ArgumentException("Thread ID must be positive.", nameof(threadId)); - SafeMemoryHandle handle = NativeMethods.OpenThread(ThreadAccess.QueryInformation, false, threadId); + const ThreadAccess requiredAccess = + ThreadAccess.SuspendResume | + ThreadAccess.GetContext | + ThreadAccess.SetContext | + ThreadAccess.QueryInformation; + + SafeMemoryHandle handle = NativeMethods.OpenThread(requiredAccess, false, threadId); if (handle.IsInvalid) { int error = Marshal.GetLastPInvokeError(); @@ -115,12 +121,13 @@ public sealed class ThreadFactory $"Thread {threadId} does not belong to process {_memory.ProcessId}."); } - // Open a handle with the rights the public RemoteThread surface needs. - return new RemoteThread(_memory, threadId); + // Ownership of the validated handle transfers to the RemoteThread. + return new RemoteThread(_memory, threadId, handle); } - finally + catch { handle.Dispose(); + throw; } } @@ -202,12 +209,39 @@ public sealed class ThreadFactory } /// - /// Suspends all threads selected by . + /// Suspends all target threads selected by . /// public FrozenThread Freeze(Func predicate) { ArgumentNullException.ThrowIfNull(predicate); - return Freeze(Enumerate().Where(predicate)); + + var selected = new List(); + try + { + foreach (RemoteThread thread in Enumerate()) + { + try + { + if (predicate(thread)) + selected.Add(thread); + else + thread.Dispose(); + } + catch + { + thread.Dispose(); + throw; + } + } + + return Freeze(selected); + } + catch + { + foreach (RemoteThread thread in selected) + thread.Dispose(); + throw; + } } private long GetCreationTime(int threadId)