Make the shared state in the hook path actually shared-safe

[doomedraven]

Thread safety. Hook installation and DLL load/unload notifications run on
whatever thread the target happens to be using, and five pieces of state on
those paths are read-modify-written with no synchronization.

What is fixed

1. tmphookinfo is one global for the whole process

hooking.c:322-323. Thread A claims it inside
New_NtAllocateVirtualMemory (:339-342); thread B's next non-alloc hook
resets the claim at :345-347 while A is still inside its hook. hook_info()
(:413) then hands A the wrong hook_info_t, so current_hook,
stack_pointer and return_address cross between threads.

Both are now __declspec(thread). That is what the invariant always wanted:
the recursion this guards against is inherently per-thread. hook_tls.c and
hook_com.c already use __declspec(thread), so there is precedent in this
DLL for it working under injection.

hook_thread.c:668 — the matching extern gets the same storage class.
NtTerminateThread's cross-thread clear becomes a no-op for other threads,
which is correct: a thread-local claim dies with its thread. Self-termination
still clears, because there the ids match.

2. The rate limiter writes a shared FILETIME and two shared counters

hooking.c:324 — FILETIME ft; was file-scope, written by every thread
passing through any hook. Now a local.

h->counter++ and h->rate_counter++ are non-atomic increments on state
shared by every thread calling that API. Both become InterlockedIncrement,
and the comparisons use the returned value rather than re-reading the field
(which was a second race on its own).

3. The x64 hookdata arena hands out the same slot twice

hooking_64.c:883-931. alloc_hookdata_near walks the arena list, bumps
curr->NextFreeOffset and inserts at the head of g_hook_arenas — all
unsynchronized. Two concurrent installs get the same slot and the second
trampoline overwrites the first.

The body is now behind an SRW lock.

Note

The lock is around the allocator, not around hook_api, and that is
deliberate. hook_api calls GetModuleHandleW, which takes the loader
lock. set_hooks_dll is itself called from the DLL load notification
callback, which runs holding the loader lock. A lock around hook_api
would give: thread B holds loader lock, wants install lock; thread A holds
install lock, wants loader lock. Deadlock.

The locked region here calls only pNtAllocateVirtualMemory, so no
inversion is possible.

4. is_hooked is a check-then-set

hooking_32.c:646 / hooking_64.c:1021:

if (h->is_hooked != 0)
    return 0;

is_hooked is not set until the end of a long installation, so two
threads can both pass this and install over each other.

Replaced with an atomic claim (0 → -1, "installing") in a thin hook_api
wrapper around the renamed body. The claim is released unless the body
reached is_hooked = 1 — delay-loaded DLLs depend on a later set_hooks_dll
retrying a failed install, so a failed attempt must not leave the hook
permanently claimed. The x86 HOOK_SAFEST retry at :854 now calls the body
directly, since the caller already holds the claim.

5. dll_ranges is mutated while readers iterate it

misc.c:906-945. is_in_dll_range is read from the hook path once per
backtrace frame, while:

• loaded_dlls++ is a non-atomic RMW,
• add_dll_range does check-then-act against itself,
• remove_dll_range moves the tail entry into the hole before
  decrementing the count, so a reader can see start from one module and
  end from another.

Writers are now serialized with an SRW lock. Readers stay lock-free and are
made correct by store ordering instead:

end is the validity marker. The test is addr < end on unsigned values,
so a slot with end == 0 matches nothing. Insert writes start then
end; removal clears end first. A reader sees the old entry, or a slot
that matches nothing — never a mixture of two modules.

This means removal leaves a hole rather than swapping the tail down. Holes
are reused by the next insert, so the array does not grow without bound; the
only cost is that loaded_dlls no longer shrinks, so the reader loop may
step over a few dead slots.

Are these related?

No. Five independent sites, five independent mechanisms, no shared state
between them. Any one can be dropped without affecting the others — they are
batched only because they are the same class of defect.

The exception in this series remains #211, where defining NDEBUG would have
deleted a VirtualProtect call and the only pre_tramp overflow guard, so
the assert rewrite and the NDEBUG definition had to be one commit. Nothing
like that applies here.

Deliberately not included

Site	Why not
CAPE/Output.c:51-59,283,337 — DebugBuffer, PipeBuffer, DebuggerLine, StringsLine are process-global scratch written from every thread, producing interleaved log lines	Needs a real lock, and the functions holding it call CreateFile/WriteFile, which are hooked. log.c:723 already demonstrates that failure mode: it calls pipe(), which blocks on CallNamedPipeW(NMPWAIT_WAIT_FOREVER) while holding g_mutex. The right fix is per-thread buffers, which belongs with the TLS scratch-buffer work
lookup.c:60-67 — real TOCTOU for LOOKUP_SHARED, two threads end up on divergent copies of "shared" state	LOOKUP_SHARED and lookup_get_or_create have zero callers in the tree. Adding synchronization to unreachable code is not worth the risk
hooking.c:357 — h->hook_disabled is latched on a rate spike and never cleared, because the early return at :357 precedes the reset at :379, silently turning api-rate-cap into a permanent api-cap	A logic defect, not a race. Fixing it needs a new field in hook_t to distinguish rate-disabled from cap-disabled

Testing

Not compiled, not run. No MSVC available, and see #212 for why CI has
never executed on any PR in this series.

This is the change in the series I would least want merged unverified. Races
do not reproduce on demand, so "it ran fine once" proves very little — but a
deadlock or a broken hook install shows up immediately, and neither of those
has been observed because nothing has been executed at all. Specific things
worth checking on a real run:

• hooks still install (any silent regression in the is_hooked claim shows
  up as missing API records across the board);
• no hang at process start under a sample that loads DLLs from multiple
  threads — that is where a loader-lock inversion would appear;
• __declspec(thread) behaves under whatever injection method is in use.
  Static TLS in a LoadLibrary-ed DLL is fine on Vista+, but a manually
  mapped DLL without _tls_used processing would fault on first access.
  hook_tls.c already relies on this, so it should be settled, but it is
  the assumption with the widest blast radius in this PR.

Series

• CI: update the MSBuild workflow to runner images that still exist #212 — CI (prerequisite)
• Fix logic defects that silently disable checks or corrupt output #206 — one-to-three-line logic defects
• Fix memory-safety defects in the API hook handlers #207 — memory safety, hook handlers
• Bound the fixed-size buffers in the core #208 — memory safety, core
• Report what the API actually produced (changes behaviour-log output) #209 — log-record correctness (changes behaviour-log output)
• Free what is allocated, restore what is saved #210 — leaks, wrong frees, last-error discipline
• Define NDEBUG in Release and build it optimized #211 — build configuration (NDEBUG, /O2)
• Stop doing expensive work to answer cheap questions on the hot path #213 — hot-path early-outs
• this PR — thread safety
• remaining: hot-path allocations (TLS scratch buffers), which would also
  supersede the CAPE/Output.c item above