Give each thread its own BSON buffer and stop holding g_mutex across all of loq()

[doomedraven]

Supersedes #162. Prerequisite for #164.

g_bson and g_istr are process-wide statics, so loq() has to hold g_mutex from entry until the record is flushed. Every hooked API call on every thread serializes against every other one, and the lock covers the expensive part (format parsing, string conversion, BSON building) as well as the cheap part.

This gives each thread its own serialization context and narrows the lock to the two regions that actually touch shared state.

What changes

	before	after
g_bson / g_istr	file-static, one per process	per-thread, in a lookup.h table keyed by thread id (LOOKUP_THREAD)
g_mutex hold	entire body of loq()	region A (logtbl_explained, last_api_logged, lastlog reset, one-time schema record) and region B (output buffer + lastlog dedup)
sizing pass, emit pass, log_string, log_wstring, log_buffer	under the lock	unlocked, thread-local memory only

One file, log.c, +93/−20.

Why a thread-id lookup table

The first revision used TlsAlloc/TlsGetValue. After Kev's comment on #216 it uses LOOKUP_THREAD from lookup.h (8f40674), which was added to replace exactly this kind of per-thread state. It is the same thread-id-keyed scheme hook_info() uses for g_hook_info.

Not TlsAlloc/TlsGetValue. It works, but it uses up a process TLS index, and TlsGetValue() clears the thread's last error on every successful call. Every call in the first revision sat inside loq()'s get_lasterrors/set_lasterrors window, so that was not a bug here, but it is a trap for the next caller.

Not __declspec(thread). Static TLS is resolved by the loader. It works when capemon is injected with LoadLibrary, which is the normal path — hook_com.c:43 and hook_tls.c:44-48 already use it on capemon today and detonations are fine. But ReflectiveInjectDllViaThread() in loader/loader/Loader.c:646 maps the image by hand and nothing processes the TLS directory there.

Not the TEB NtTib.ArbitraryUserPointer slot (fs:[0x14] / gs:[0x28]). It is not free. ntdll's loader parks the FullDllName pointer there across its NtMapViewOfSection call so the debugger can see the module name being mapped. capemon hooks NtMapViewOfSection (hooks.c:127, :975, :1065, :1358) and LdrLoadDll (hooks.c:98 and five more). Sequence:

ntdll!LdrpMapViewOfSection
  saves ArbitraryUserPointer
  writes &FullDllName into it
  calls NtMapViewOfSection  ->  capemon hook  ->  loq()
                                                    reads ArbitraryUserPointer
                                                    gets a PWSTR
                                                    bson_init() writes over the loader's string

That is memory corruption on every module load.

Not a DLL_THREAD_DETACH destructor. capemon.c:632 calls hide_module_from_peb() during DLL_PROCESS_ATTACH, and misc.c:1072-1095 unlinks the module from InLoadOrderModuleList, InInitializationOrderModuleList, InMemoryOrderModuleList and the hash table. The loader walks those lists to dispatch thread notifications, so DllMain is never entered again and a TLS callback would never fire either.

Contexts are therefore never freed. A thread that is given a dead thread's id inherits that context, so retained memory is bounded by the number of distinct thread ids that have logged, not by the number of threads created. That is safe because loq() starts every record with bson_init(), which zeroes the whole bson struct; a record the dead thread left half-built only leaks its buffer. Each context is 312 bytes on x64 and 168 on x86, because the bson struct carries a 32-entry size_t stack; the first revision's "~40 bytes" was wrong. The BSON payload is still allocated and released per call by bson_init/bson_destroy, and freeing contexts at teardown would race with threads still inside loq().

g_bson/g_istr expand to ctx->bson_obj/ctx->istr_buf, where ctx is a local that loq() and each serializing helper fetch once. A record therefore costs one table walk in loq() plus one per helper call (roughly one per logged argument), instead of one TLS read per field access. The list holds one entry per thread id that has logged; hook_info() already walks g_hook_info, keyed the same way, on every hooked call. A function that uses g_bson without a ctx in scope fails to compile.

Behaviour preserved

The bounded-spin acquisition added in ada31ca is kept verbatim, factored into loq_lock() so both regions use the same shape: one cheap TryEnterCriticalSection, then up to 100 SwitchToThread retries, then drop the record rather than stall a hooked API. Both regions are exit-balanced; a failed region-B acquisition does bson_destroy() before bailing.

The one observable difference under concurrency: because the lock is released between the explanation record and the flush, another thread's record can land in the lastlog dedup slot in between. lastlog is already a single global slot shared across threads, so which record occupies it was already non-deterministic — this widens the window, it does not create the interleaving.

Testing

Not compiled and not run. There is no MSVC on the machine this was written on, and the MSBuild workflow is disabled_manually at the repo level, so no PR in this series has been through a compiler. Re-enabling it needs Settings → Actions; #212 fixes the workflow file but cannot flip that switch.

The TlsAlloc harness from the first revision no longer applies. The context code is now a single LOOKUP_THREAD call, and the table mechanics are the ones exercised by the harness described on #216 (recycled thread ids, ASan/UBSan/TSan). For this revision, checked by inspection with comments stripped: every function that uses g_bson/g_istr declares ctx before its first use, loq() assigns ctx before it serializes anything, and nothing after exit: touches it. The MSVC build, the loq() restructuring and a detonation run are still unverified.

Are these related?

No, in the sense that matters for review: this is self-contained and can be merged, reverted or deferred on its own.

Contrast with the NDEBUG case in #211, which genuinely is coupled — the assert() removals there are only correct because the same PR defines NDEBUG, and splitting them would leave the tree in a state where the asserts have side effects that vanish.

No textual conflicts with the rest of the series: git merge-tree of this branch against each of #206–#214 and #216 is clean, and the ctx check above passes on each merged log.c.

#217 is merged (df8fc4d). With it, allocation failure on a thread's first log returns NULL and loq() drops the record, as the TlsAlloc revision did, and lookup payloads, and so this context, are pointer-aligned on x64.

Rebased onto current capemon (df8fc4d: a65ba12 plus the #217 merge). The rebase did not change either commit; git range-diff shows = for both.

Why a new PR rather than a fix on #162

#162 cannot compile: log.c:88-89 on that branch defines g_bson with a GNU statement expression, which cl.exe rejects. It also still declares static __declspec(thread) thread_log_context_t* g_tls_ctx_cache, the exact construct its own description argues is unsafe, and it hooks DLL_THREAD_DETACH for cleanup that never runs. Its branch does not rebase onto current capemon cleanly. Rebuilding it as a single clean commit was less work than untangling it.

#164 is restacked onto this revision (5cf5060).

Series

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

Draft, as with the rest of the series — none of it has been through a compiler yet.