Free what is allocated, restore what is saved

[doomedraven]

What this is

Fifth PR in the review series. Resource leaks, one wrong free(), two poisoned log handles, and the get_lasterrors/set_lasterrors discipline.

Back to no intended change in behaviour-log contents — #209 was the one that changes output. Three small behaviour changes are listed at the bottom.

Are these related?

Independent of each other. Two files share a shape (DebuggerOutput and StringsOutput are the same function twice) but nothing here is coupled.

The coupled case is still the NDEBUG PR near the end of the series: you cannot define NDEBUG without simultaneously converting the VirtualProtect inside assert() at alloc.c:85 and the pre_tramp bound check at hooking_32.c:267,378 / hooking_64.c:542,826 into real runtime checks.

The fixes

CAPE/CAPE.c MapFile — free() of a stack address

BOOL MapFile(HANDLE hFile, unsigned char **Buffer, DWORD *FileSize)
...
    free(Buffer);   // Buffer is unsigned char **

Three error paths do this. Buffer is the address of the caller's stack variable, not the allocation. Passing it to the CRT allocator is heap corruption at best and an immediate abort at worst. It fires on any failed or short ReadFile.

→ free(*Buffer); *Buffer = NULL;

CAPE/Output.c DebuggerOutput — the largest leak in the tree

FullPathName = GetResultsPath("debugger");   // calloc + CreateDirectory syscall
OutputFilename = (char*)calloc(MAX_PATH, sizeof(BYTE));
...
PathAppend(FullPathName, OutputFilename);
free(OutputFilename);
if (!DebuggerLog) { ...open it... }
// FullPathName never freed

All of that ran on every call, and DebuggerOutput fires one to four times per single-stepped instruction. A one-million-step trace does roughly a million CreateDirectory syscalls and leaks about 260 MB.

The path is only needed while opening the log, so it now lives inside the if (!DebuggerLog) block and is freed there.

Two more problems in the same function:

• A failed CreateFile leaves DebuggerLog == INVALID_HANDLE_VALUE. Every later call then does WriteFile(INVALID_HANDLE_VALUE, ...) forever, and the open is never retried. Reset to NULL.
• Both error returns skipped va_end.

StringsOutput is the same function with different names, plus one extra wrinkle: StringsFile is a global that DumpStrings (CAPE.c:1833) reads later, so it has to persist — but it was reallocated and the previous value leaked on every call. Now built once, with the old value freed on replacement.

hook_tls.c, log.c, pipe.c — GetResultsPath results never freed

LogTls and LogTls13 rebuilt and leaked the tlsdump path for every captured secret. log.c:1470 leaks once at init. pipe.c:193 leaks per pipe() call in standalone mode — 260 bytes each.

While in there, all four gain the NULL check GetResultsPath has always needed. It returns 0 on allocation failure, on a path longer than MAX_PATH, and on a CreateDirectory error, and every caller handed the result straight to PathAppend.

Last-error discipline

Site	Problem
hook_sleep.c NtDelayExecution	Two early returns after get_lasterrors with no matching set_lasterrors
hook_misc.c SetupDiGetClassDevsA	The restore sits inside if (ClassGuid), so a NULL ClassGuid skips it
hook_misc.c SetupDiGetClassDevsW	Same, plus the capture happens before the original call, so the restore overwrites the error the API set
hook_misc.c WNetGetProviderNameW	The anti-VM path fills in a lasterror_t and never applies it

The WNetGetProviderNameW one is the visible bug: the handler fakes an ERROR_NO_NETWORK return to hide VirtualBox's network redirector, but GetLastError() still reported whatever the real call set. A sample that checks both sees the inconsistency. The set_lasterrors is placed after the free(), since HeapFree can clobber the last error itself.

Behaviour changes

1. DebuggerOutput and StringsOutput retry the log-file open after a failure instead of writing to INVALID_HANDLE_VALUE forever.
2. GetLastError() after WNetGetProviderNameW now agrees with the returned ERROR_NO_NETWORK.
3. The four GetResultsPath callers now bail out instead of calling PathAppend(NULL, ...) when the results directory cannot be created.

Deliberately not in this PR

• The P2 unchecked-allocation cluster (~25 sites across hook_file.c, hook_reg.c, hook_reg_native.c, hook_services.c, log.c). Those are all the malloc(32768 * sizeof(wchar_t)) path buffers, and the next PR replaces them with a per-thread scratch buffer — adding NULL checks now would be churn that PR then deletes, plus a guaranteed conflict on every one of those lines.
• hooking_32.c:844-855 — hookdata leaked on the HOOK_SAFEST retry and on both restore_protect paths. That one also leaves a stale pointer that poisons inside_hook, so it belongs with the hooking-engine changes.

Not verified by build

No MSVC here. CI has not run on #206–#209 — gh pr checks reports no checks on any branch. Either fork PRs need your approval before workflows run, or .github/workflows/msbuild.yml's runs-on: windows-2019 no longer schedules since GitHub retired that image. Tell me which and I will send a prerequisite PR bumping the runner and the actions/checkout@v3 / upload-artifact@v3 steps.

Series

• Fix logic defects that silently disable checks or corrupt output #206 — 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 output)
• this — leaks, wrong frees, last-error discipline
• next — hot-path allocations (TLS scratch buffers, kills the P2 cluster)
• then — hot-path algorithms (inside_hook ranges, lookup buckets, single backtrace walk)
• then — build configuration (NDEBUG + assert conversion + /O2 /Oy-, the coupled one)
• then — thread safety (hook_api lock, arena free list, breakpoint lists)

[doomedraven]

Correction to the CI note in the description above: the diagnosis there was wrong.

The MSBuild workflow is disabled_manually at the repository level, so it cannot run regardless of what the PR contains:

$ gh api repos/kevoreilly/capemon/actions/workflows
28093452  MSBuild  .github/workflows/msbuild.yml  disabled_manually

On top of that it targets the retired windows-2019 image (jobs queue for 24h and get cancelled by the timeout — visible in every PR Build Test run), and all three projects declare PlatformToolset=v141, which is not installed on windows-2022 or windows-2025.

#212 fixes the workflow file for all three. Re-enabling the workflow in Settings → Actions is the part that has to be done by hand.