Stop allocating 64 KB per path on the hooked call paths

[doomedraven]

Normalising a path needs a WIDE_STRING_LIMIT-sized buffer, and every hooked call took a fresh one from the allocator. NtQueryInformationFile takes two, NtSetInformationFile takes three, handle_new_file takes two, and half of them are callocs that zero all 64 KB first. On a sample doing sustained file or registry I/O that is tens of thousands of 64 KB allocate/zero/free cycles a second, none of which outlive the call that made them.

This replaces them with a small per-thread pool.

What it does

void *path_scratch_acquire(void);   // 64 KB, first character zeroed
void path_scratch_release(void *);  // NULL-safe

Four slots per thread, allocated lazily. Past four live buffers, acquire falls back to malloc and release frees it, so exhaustion degrades to the previous behaviour instead of failing. The slot claim is interlocked, because a structured exception handler can run on top of a hook on the same thread and would otherwise be able to claim a slot between the test and the set.

Each thread's pool lives in a lookup.h table keyed by thread id (LOOKUP_THREAD), as suggested in review. No other locking: an entry is only touched by the thread whose id it carries, and lookup_add() publishes it with a CAS.

• Not TLS: TlsGetValue() clears the thread's last error on every successful call. DeleteFileW, RemoveDirectoryW and both MoveFileWithProgress* ALT hooks release after the original call and after loq() has restored the error, so a failed call would return FALSE with GetLastError() == 0. lookup_get() touches no thread state.
• Not hook_info_t: hook_info() can return the shared static tmphookinfo, and five hooks memcpy the whole hook_info_t back after the original call.
• Cost: one list walk per acquire and per release instead of a TEB read. g_hook_info is keyed the same way, and enter_hook() already walks it on every hooked call.

37 sites converted:

file	sites
hook_file.c	24
log.c	6 registry-path buffers + 2 in %F/%Z handling
misc.c	2
pipe.c	2
hook_process.c, hook_special.c	1 each

Why not zeroing the whole buffer

Only the first character is zeroed. That is what the callocs were actually providing: path_from_object_attributes() (misc.c:1159) returns without writing anything when the object is NULL or the name fails our_isbadreadptr, and its callers then rely on the buffer reading as an empty string. Every consumer in these paths treats the buffer as a string and stops at the terminator — none read past it.

Pre-existing bugs fixed on the way

• hook_file.c new_file_path_ascii() and new_file_path_unicode() never freed their buffer at all. 32 KB and 64 KB leaked per dropped-file notification, which is one per new file the sample writes.
• pipe.c _pipe_sprintf() allocated before the if (s == NULL) return -1; in its %F handler, so a NULL argument leaked 64 KB.
• misc.c is_image_base_remapped() and prevent_module_reloading() dereferenced both of their allocations without checking either.
• The six registry-path buffers in log.c now check for NULL before use. allocsize there is bounded against the slot size by a C_ASSERT.

Trade-off

This converts transient allocation into retained memory per thread id. A thread that nests four deep holds 256 KB, and nothing frees it when the thread exits: DllMain handles only process attach/detach, and the module is unlinked from the PEB, so DLL_THREAD_DETACH never reaches it. A thread handed a recycled id inherits the previous owner's slots, so the total is bounded by distinct thread ids rather than by threads created. Lazy slot allocation keeps the common one- or two-buffer case at 64–128 KB per id, but a sample running very many concurrent path-touching threads will hold more resident memory than before. A slot whose owner died holding it stays busy, so the next owner of that id reaches the heap fallback one slot sooner.

I do not have a measurement to justify four rather than two — three is the deepest simultaneous use in the tree (NtSetInformationFile), so four is that plus one. PATH_SCRATCH_SLOTS is a single constant if it is the wrong balance. Happy to drop it to three, or to cache only the first two slots and leave the rest heap-backed, if you would rather trade some of the win back for a lower ceiling.

Deliberately not in this PR

18 of the converted sites do not check the result of the allocation and never did — they pass it straight into path_from_handle() or ensure_absolute_unicode_path(). This PR does not change that. It makes the failure rarer (a slot is only allocated once per thread) but it does not fix it.

Adding those guards means 18 bespoke control-flow decisions about what a hook should do and what it should return when it cannot get a buffer, which is a different kind of change from a mechanical allocation swap, and not one I want to bundle into an unbuildable PR. It wants its own review.

Testing

Not compiled and not run. No MSVC available, and the MSBuild workflow is disabled_manually at the repo level, so nothing in this series has been through a compiler. #212 fixes the workflow file but re-enabling it needs Settings → Actions.

What was done instead:

• Pairing audited mechanically: for every path_scratch_acquire() the enclosing function is checked for a matching path_scratch_release() and for any surviving raw free() of the same variable. All 37 balanced, no raw frees left. The one long span (NtDeviceIoControlFile, acquire at :722, release at :927) has nine intervening jumps, all goto generic_log, which lands before the release.
• Checked that no converted buffer is stored anywhere outliving its release, and that the two-free sites (is_protected_objattr, _pipe_sprintf) are mutually exclusive paths.
• The pool itself (first revision, TLS-backed; path_scratch_acquire()/path_scratch_release() are unchanged since) was extracted into a standalone harness, compiled gcc -std=c99 -Wall -Wextra -O2 clean against POSIX stubs, and stress-tested: 16 threads × 20 000 iterations, nesting 1–7 deep to exercise the heap fallback, releasing in order and in reverse, verifying every live buffer is distinct and that a full-width sentinel written to one is never clobbered by another. PASS. Under ASan the only report is the retained slots themselves, which is the design.
• The LOOKUP_THREAD revision was checked in a POSIX harness against the real lookup.c with recycled thread ids: 2000 threads over 32 ids → 33 entries / 132 slots, all reachable (the TLS revision left 1604 slots unreachable after 400 threads). TSan's only reports are lookup_get()'s plain loads against lookup_add()'s CAS, the same pattern as g_hook_info; none on the pool's own fields.

None of that covers the MSVC build or a detonation.

Are these related?

No — this is self-contained and can be merged, reverted or deferred on its own. It uses upstream lookup.h as is. #217 (merged) made lookup_add() NULL-safe, so an allocation failure on a thread's first acquire falls back to the heap instead of faulting inside lookup_add(); nothing here needs it to build.

Contrast with the NDEBUG case in #211, which genuinely is coupled: the assert() removals there are only correct because the same PR defines NDEBUG.

Textual conflicts to expect (trial merges against df8fc4d and every open branch in the series): only hook_process.c with #209, on adjacent lines — #209 rewrites a LOQ_ntstatus call and this PR replaces the free(fname) after it. Keep both. The overlaps listed in earlier revisions of this description now merge cleanly.

Series

#206, #207, #208, #209, #210, #211, #212, #213, #214, #215, #164, this one, #217 (merged).

[kevoreilly]

I really like this idea of having a path_scratch_acquire() function to replace the mallocs but I would like to explore whether the thread-based scratch buffer can be achieved without using the Tls apis (e.g. TlsAlloc) but instead use the internal lookup functions (lookup.h) keyed with the thread id instead.

[doomedraven]

Yes, LOOKUP_THREAD drops straight in. Only scratch_context() changes; acquire/release are untouched:

static lookup_t g_path_scratch;

static path_scratch_t *scratch_context(BOOL create)
{
	if (create)
		return LOOKUP_THREAD(&g_path_scratch, path_scratch_t);

	return (path_scratch_t *)lookup_get(&g_path_scratch, (ULONG_PTR)GetCurrentThreadId(), NULL);
}

It also fixes two problems in the TLS version:

1. TlsGetValue() clears the thread's last error on every successful call. DeleteFileW, RemoveDirectoryW and both MoveFileWithProgress* ALT hooks release after the original call and after loq() has restored the error, so a failed call returns FALSE with GetLastError() == 0. lookup_get() touches no thread state.
2. Nothing runs at thread exit (DllMain only handles process attach/detach, and the module is unlinked from the PEB), so every dead thread's TLS context and slots leak. Keyed by TID, a thread handed a recycled id inherits the previous owner's slots, so retained memory is bounded by distinct TIDs rather than by threads created. A slot whose owner died holding it just stays busy.

Kept in its own table rather than hook_info_t: hook_info() can return the static tmphookinfo, and five hooks memcpy the whole hook_info_t back after the original call.

Cost is one list walk per acquire and per release instead of a TEB read, over a list no longer than g_hook_info, which enter_hook() already walks on every hooked call.

Two things in lookup.c, independent of this PR, are split out into #217: lookup_add() doesn't check calloc(), so OOM on a thread's first acquire would fault in lookup_add instead of falling back to the heap; and on x64 entry_t.data sits at offset 20, so every pointer in every payload is 4 mod 8 (UBSan flags the path_scratch_t accesses as misaligned). #217 adds the NULL check and widens entry_t.size to ULONG_PTR, which moves data to offset 24 without changing sizeof(entry_t). Neither PR depends on the other.

Checked in a POSIX harness (pool code verbatim, real lookup.c, recycled TIDs), not the MSVC build: 2000 threads over 32 TIDs → 33 entries / 132 slots, all reachable; the TLS version left 1604 slots unreachable after 400 threads. TSan's only reports are lookup_get's plain loads against lookup_add's CAS, the same pattern as g_hook_info, none on the pool's own fields.

Happy to push this to the branch, and update the trade-off section of the description, if you're good with it.