From 26ca0f46a6e03ce16dcf86325e4c5b21526d960a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 12 Oct 2026 08:54:39 +0200 Subject: [PATCH 1/2] mingw: reduce stack usage during process-tree cleanup Process-tree cleanup reserves a 64 KiB PID array on the stack regardless of the tree's size. Move that storage to the heap so memory use follows the collected tree, retaining the existing 16,384-process traversal cap. Assisted-by: GPT-6.1 Sol Signed-off-by: Johannes Schindelin --- compat/win32/exit-process.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/compat/win32/exit-process.h b/compat/win32/exit-process.h index 26004161bcbdc3..6c573065bba164 100644 --- a/compat/win32/exit-process.h +++ b/compat/win32/exit-process.h @@ -25,12 +25,12 @@ static int terminate_process_tree(HANDLE main_process, int exit_status) { HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); PROCESSENTRY32 entry; - DWORD pids[16384]; - int max_len = sizeof(pids) / sizeof(*pids), i, len, ret = 0; + DWORD *pids = NULL; + int max_len = 16384, i, len = 1, alloc = 0, ret = 0; pid_t pid = GetProcessId(main_process); + ALLOC_GROW(pids, len, alloc); pids[0] = (DWORD)pid; - len = 1; /* * Even if Process32First()/Process32Next() seem to traverse the @@ -54,8 +54,10 @@ static int terminate_process_tree(HANDLE main_process, int exit_status) for (i = len - 1; i >= 0; i--) { if (pids[i] == entry.th32ProcessID) break; - if (pids[i] == entry.th32ParentProcessID) + if (pids[i] == entry.th32ParentProcessID) { + ALLOC_GROW(pids, len + 1, alloc); pids[len++] = entry.th32ProcessID; + } } } while (len < max_len && Process32Next(snapshot, &entry)); @@ -75,6 +77,7 @@ static int terminate_process_tree(HANDLE main_process, int exit_status) if (!TerminateProcess(main_process, exit_status)) ret = -1; CloseHandle(main_process); + free(pids); return ret; } From 52eee2824a211516dd6c87e6ad73fd62ae0f03b4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 12 Oct 2026 09:41:24 +0200 Subject: [PATCH 2/2] mingw: avoid terminating unrelated processes during cleanup Process-tree cleanup can kill an older, unrelated orphan when Windows assigns the cleanup target a PID formerly held by the orphan's parent. Matching a parent PID alone does not prove ancestry; see https://github.com/git-for-windows/git/issues/6435. Use `NtQuerySystemInformation` with `SystemProcessInformation` to obtain creation times alongside PID and parent PID without opening processes during discovery. Reject candidates that predate their claimed parents. Record a precise UTC `FILETIME` immediately before each query attempt and use the successful attempt's cutoff. Reject entries whose recorded creation time is at or after that cutoff. Windows records the `KeQuerySystemTime()` value at process creation and leaves it unchanged when system time is adjusted, as documented here: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntddk/nf-ntddk-psgetprocesscreatetimequadpart These times use 100-nanosecond units, but that is not their accuracy. Microsoft documents one-clock-tick accuracy for `KeQuerySystemTime()` and microsecond accuracy for the precise clock in its API reference: https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdm/nf-wdm-kequerysystemtimeprecise Only terminate a descendant if `GetProcessTimes()` on its termination handle reports the exact creation time captured for that PID. This rejects replacements from PID reuse after capture. The root's held handle pins its PID. Assisted-by: GPT-6.1 Sol Signed-off-by: Johannes Schindelin --- Makefile | 1 + compat/mingw.c | 5 +- compat/win32/exit-process.h | 149 +++++++++++++++++++++------ t/meson.build | 1 + t/unit-tests/u-win32-process.c | 181 +++++++++++++++++++++++++++++++++ 5 files changed, 306 insertions(+), 31 deletions(-) create mode 100644 t/unit-tests/u-win32-process.c diff --git a/Makefile b/Makefile index c98be9155c54ae..18bf80e49bdd2e 100644 --- a/Makefile +++ b/Makefile @@ -1571,6 +1571,7 @@ CLAR_TEST_SUITES += u-strvec CLAR_TEST_SUITES += u-trailer CLAR_TEST_SUITES += u-urlmatch-normalization CLAR_TEST_SUITES += u-utf8-width +CLAR_TEST_SUITES += u-win32-process CLAR_TEST_PROG = $(UNIT_TEST_BIN)/unit-tests$(X) CLAR_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(CLAR_TEST_SUITES)) CLAR_TEST_OBJS += $(UNIT_TEST_DIR)/clar/clar.o diff --git a/compat/mingw.c b/compat/mingw.c index 9c5a5140790161..b58573cd2c2836 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2552,7 +2552,10 @@ int mingw_kill(pid_t pid, int sig) if (h) ret = exit_process(h, 128 + sig); else { - h = OpenProcess(PROCESS_TERMINATE, FALSE, pid); + h = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION | + PROCESS_TERMINATE, FALSE, pid); + if (!h) + h = OpenProcess(PROCESS_TERMINATE, FALSE, pid); if (!h) { errno = err_win_to_posix(GetLastError()); return -1; diff --git a/compat/win32/exit-process.h b/compat/win32/exit-process.h index 6c573065bba164..26fe4563efc10d 100644 --- a/compat/win32/exit-process.h +++ b/compat/win32/exit-process.h @@ -11,7 +11,87 @@ * For simplicity, these functions are marked as file-local. */ -#include +#include +#include "lazyload.h" + +/* The common record prefix; the Microsoft SDK hides some of these fields. */ +struct process_snapshot { + ULONG next_entry_offset, number_of_threads; + LARGE_INTEGER reserved[3]; + FILETIME creation_time; + LARGE_INTEGER user_time, kernel_time; + UNICODE_STRING image_name; + LONG base_priority; + ULONG_PTR pid, parent_pid; +}; + +static struct process_snapshot *get_process_snapshot(FILETIME *snapshot_time) +{ + char *buffer = NULL; + size_t size = 0, base; + ULONG needed; + NTSTATUS status; + DECLARE_PROC_ADDR(ntdll.dll, NTSTATUS, NTAPI, NtQuerySystemInformation, + SYSTEM_INFORMATION_CLASS, PVOID, ULONG, PULONG); + + (void)BUILD_ASSERT_OR_ZERO(offsetof(struct process_snapshot, pid) == + offsetof(SYSTEM_PROCESS_INFORMATION, UniqueProcessId)); + (void)BUILD_ASSERT_OR_ZERO(offsetof(struct process_snapshot, parent_pid) + + sizeof(ULONG_PTR) == offsetof(SYSTEM_PROCESS_INFORMATION, HandleCount)); + if (!INIT_PROC_ADDR(NtQuerySystemInformation)) { + warning_errno("could not load NtQuerySystemInformation"); + return NULL; + } + for (;;) { + needed = 0; + GetSystemTimePreciseAsFileTime(snapshot_time); + status = NtQuerySystemInformation(SystemProcessInformation, + buffer, (ULONG)size, &needed); + if (status >= 0) + return (void *)buffer; + /* STATUS_INFO_LENGTH_MISMATCH requests a larger buffer. */ + if (status != (NTSTATUS)0xc0000004) { + warning("could not enumerate processes (NTSTATUS %#lx)", + (unsigned long)status); + break; + } + base = needed > size ? needed : size; + size = st_add(base, base / 4 + 1); + if (!needed || size > ULONG_MAX) { + warning("could not size the process snapshot buffer"); + break; + } + buffer = xrealloc(buffer, size); + } + free(buffer); + return NULL; +} + +static const struct process_snapshot *next_process_snapshot( + const struct process_snapshot *entry) +{ + return entry->next_entry_offset ? + (const void *)((const char *)entry + entry->next_entry_offset) : + NULL; +} + +static int process_started_at(HANDLE process, const FILETIME *expected_time) +{ + FILETIME creation_time, exit_time, kernel_time, user_time; + + return GetProcessTimes(process, &creation_time, &exit_time, + &kernel_time, &user_time) && + !CompareFileTime(&creation_time, expected_time); +} + +static int process_is_child(const struct process_snapshot *entry, + DWORD parent_pid, const FILETIME *parent_time, + const FILETIME *snapshot_time) +{ + return entry->parent_pid == parent_pid && + CompareFileTime(&entry->creation_time, parent_time) >= 0 && + CompareFileTime(&entry->creation_time, snapshot_time) < 0; +} /* * Terminates the process corresponding to the process ID and all of its @@ -23,53 +103,62 @@ */ static int terminate_process_tree(HANDLE main_process, int exit_status) { - HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); - PROCESSENTRY32 entry; - DWORD *pids = NULL; + struct process_snapshot *snapshot = NULL; + const struct process_snapshot *entry; + FILETIME snapshot_time, exit_time, kernel_time, user_time; + struct { + DWORD pid; + FILETIME creation_time; + } *processes = NULL; int max_len = 16384, i, len = 1, alloc = 0, ret = 0; - pid_t pid = GetProcessId(main_process); - ALLOC_GROW(pids, len, alloc); - pids[0] = (DWORD)pid; + ALLOC_GROW(processes, len, alloc); + processes[0].pid = GetProcessId(main_process); + if (!processes[0].pid || + !GetProcessTimes(main_process, &processes[0].creation_time, + &exit_time, &kernel_time, &user_time)) + goto cleanup; + snapshot = get_process_snapshot(&snapshot_time); + if (!snapshot) + goto cleanup; /* - * Even if Process32First()/Process32Next() seem to traverse the - * processes in topological order (i.e. parent processes before - * child processes), there is nothing in the Win32 API documentation - * suggesting that this is guaranteed. - * - * Therefore, run through them at least twice and stop when no more - * process IDs were added to the list. + * Do not assume that the snapshot lists parents before their children. + * Repeat until no further descendants are found. */ for (;;) { int orig_len = len; - memset(&entry, 0, sizeof(entry)); - entry.dwSize = sizeof(entry); - - if (!Process32First(snapshot, &entry)) - break; - - do { + for (entry = snapshot; entry && len < max_len; + entry = next_process_snapshot(entry)) { for (i = len - 1; i >= 0; i--) { - if (pids[i] == entry.th32ProcessID) + if (processes[i].pid == entry->pid) break; - if (pids[i] == entry.th32ParentProcessID) { - ALLOC_GROW(pids, len + 1, alloc); - pids[len++] = entry.th32ProcessID; - } + if (!process_is_child(entry, processes[i].pid, + &processes[i].creation_time, + &snapshot_time)) + continue; + ALLOC_GROW(processes, len + 1, alloc); + processes[len].pid = (DWORD)entry->pid; + processes[len++].creation_time = entry->creation_time; + break; } - } while (len < max_len && Process32Next(snapshot, &entry)); + } if (orig_len == len || len >= max_len) break; } +cleanup: + free(snapshot); for (i = len - 1; i > 0; i--) { - HANDLE process = OpenProcess(PROCESS_TERMINATE, FALSE, pids[i]); + HANDLE process = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION | + PROCESS_TERMINATE, FALSE, processes[i].pid); if (process) { - if (!TerminateProcess(process, exit_status)) + /* A reopened PID must still identify the recorded process. */ + if (process_started_at(process, &processes[i].creation_time) && + !TerminateProcess(process, exit_status)) ret = -1; CloseHandle(process); } @@ -77,7 +166,7 @@ static int terminate_process_tree(HANDLE main_process, int exit_status) if (!TerminateProcess(main_process, exit_status)) ret = -1; CloseHandle(main_process); - free(pids); + free(processes); return ret; } diff --git a/t/meson.build b/t/meson.build index 729de1177e76af..5dee36944d8c2a 100644 --- a/t/meson.build +++ b/t/meson.build @@ -29,6 +29,7 @@ clar_test_suites = [ 'unit-tests/u-trailer.c', 'unit-tests/u-urlmatch-normalization.c', 'unit-tests/u-utf8-width.c', + 'unit-tests/u-win32-process.c', ] clar_sources = [ diff --git a/t/unit-tests/u-win32-process.c b/t/unit-tests/u-win32-process.c new file mode 100644 index 00000000000000..5a42aeb000b238 --- /dev/null +++ b/t/unit-tests/u-win32-process.c @@ -0,0 +1,181 @@ +#include "unit-test.h" + +#ifndef GIT_WINDOWS_NATIVE +void test_win32_process__cleanup(void) {} +void test_win32_process__identity(void) {} +#else +static DWORD handle_limit, peak_handle_count; +static int process_open_count; + +static HANDLE checked_open_process(DWORD access, BOOL inherit, DWORD pid) +{ + HANDLE handle = OpenProcess(access, inherit, pid); + DWORD count; + + cl_assert(GetProcessHandleCount(GetCurrentProcess(), &count)); + cl_assert(count <= handle_limit); + process_open_count++; + if (count > peak_handle_count) + peak_handle_count = count; + return handle; +} + +#define OpenProcess checked_open_process +#include "compat/win32/exit-process.h" +#undef OpenProcess + +static PROCESS_INFORMATION child_process; +static PROCESS_INFORMATION descendants[3]; +static HANDLE process; +static struct process_snapshot *snapshot; +static LPPROC_THREAD_ATTRIBUTE_LIST attributes; +static int attributes_initialized; + +void test_win32_process__cleanup(void) +{ + size_t i; + + FREE_AND_NULL(snapshot); + if (attributes_initialized) + DeleteProcThreadAttributeList(attributes); + FREE_AND_NULL(attributes); + attributes_initialized = 0; + if (process) { + cl_assert(CloseHandle(process)); + process = NULL; + } + for (i = 0; i <= ARRAY_SIZE(descendants); i++) { + PROCESS_INFORMATION *child = i < ARRAY_SIZE(descendants) ? + &descendants[i] : &child_process; + + if (!child->hProcess) + continue; + if (WaitForSingleObject(child->hProcess, 0) == WAIT_TIMEOUT) + cl_assert(TerminateProcess(child->hProcess, 0)); + cl_assert_equal_i(WaitForSingleObject(child->hProcess, 10000), + WAIT_OBJECT_0); + cl_assert_equal_i(exit_process(child->hProcess, 0), 0); + cl_assert(CloseHandle(child->hThread)); + child->hProcess = NULL; + } +} + +void test_win32_process__identity(void) +{ + STARTUPINFOW startup = { .cb = sizeof(startup) }; + STARTUPINFOEXW extended = { + .StartupInfo = { .cb = sizeof(extended) }, + }; + wchar_t path[32768]; + FILETIME parent_time, creation_time, later_time, earlier_time; + FILETIME snapshot_time; + FILETIME exit_time, kernel_time, user_time; + const struct process_snapshot *entry; + DWORD path_len, exit_code, count; + SIZE_T size = 0; + size_t i; + int ret; + + path_len = GetModuleFileNameW(NULL, path, ARRAY_SIZE(path)); + cl_assert(path_len && path_len < ARRAY_SIZE(path)); + cl_assert(GetProcessTimes(GetCurrentProcess(), &parent_time, &exit_time, + &kernel_time, &user_time)); + cl_assert(CreateProcessW(path, NULL, NULL, NULL, FALSE, + CREATE_SUSPENDED, NULL, NULL, + &startup, &child_process)); + cl_assert(GetProcessTimes(child_process.hProcess, &creation_time, + &exit_time, &kernel_time, &user_time)); + snapshot = get_process_snapshot(&snapshot_time); + cl_assert(snapshot); + for (entry = snapshot; entry; entry = next_process_snapshot(entry)) + if (entry->pid == child_process.dwProcessId) + break; + cl_assert(entry); + cl_assert_equal_i(CompareFileTime(&entry->creation_time, + &creation_time), 0); + cl_assert(process_is_child(entry, GetCurrentProcessId(), &parent_time, + &snapshot_time)); + cl_assert(process_started_at(child_process.hProcess, &creation_time)); + cl_assert(process_is_child(entry, GetCurrentProcessId(), + &creation_time, &snapshot_time)); + cl_assert(!process_is_child(entry, 0, &parent_time, &snapshot_time)); + later_time = creation_time; + if (!++later_time.dwLowDateTime) + later_time.dwHighDateTime++; + cl_assert(!process_started_at(child_process.hProcess, &later_time)); + cl_assert(!process_is_child(entry, GetCurrentProcessId(), &later_time, + &snapshot_time)); + cl_assert(!process_is_child(entry, GetCurrentProcessId(), &parent_time, + &creation_time)); + cl_assert(process_is_child(entry, GetCurrentProcessId(), &parent_time, + &later_time)); + earlier_time = creation_time; + if (!earlier_time.dwLowDateTime--) + earlier_time.dwHighDateTime--; + cl_assert(!process_is_child(entry, GetCurrentProcessId(), &parent_time, + &earlier_time)); + FREE_AND_NULL(snapshot); + + process = OpenProcess(PROCESS_TERMINATE, FALSE, + child_process.dwProcessId); + cl_assert(process); + cl_assert(!process_started_at(process, &creation_time)); + cl_assert_equal_i(GetLastError(), ERROR_ACCESS_DENIED); + ret = terminate_process_tree(process, 143); + process = NULL; + cl_assert_equal_i(ret, 0); + cl_assert_equal_i(WaitForSingleObject(child_process.hProcess, 10000), + WAIT_OBJECT_0); + cl_assert(GetExitCodeProcess(child_process.hProcess, &exit_code)); + cl_assert_equal_i(exit_code, 143); + + test_win32_process__cleanup(); + cl_assert(CreateProcessW(path, NULL, NULL, NULL, FALSE, + CREATE_SUSPENDED, NULL, NULL, + &startup, &child_process)); + cl_assert(!InitializeProcThreadAttributeList(NULL, 1, 0, &size)); + attributes = xmalloc(size); + extended.lpAttributeList = attributes; + for (i = 0; i < ARRAY_SIZE(descendants); i++) { + HANDLE parent = i == 1 ? descendants[0].hProcess : + child_process.hProcess; + + cl_assert(InitializeProcThreadAttributeList(attributes, 1, 0, + &size)); + attributes_initialized = 1; + cl_assert(UpdateProcThreadAttribute(attributes, 0, + PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, &parent, + sizeof(parent), NULL, NULL)); + cl_assert(CreateProcessW(path, NULL, NULL, NULL, FALSE, + CREATE_SUSPENDED | EXTENDED_STARTUPINFO_PRESENT, + NULL, NULL, &extended.StartupInfo, &descendants[i])); + DeleteProcThreadAttributeList(attributes); + attributes_initialized = 0; + } + process = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION | + PROCESS_TERMINATE, FALSE, + child_process.dwProcessId); + cl_assert(process); + cl_assert(GetProcessHandleCount(GetCurrentProcess(), &handle_limit)); + /* Only the process being terminated needs a temporary handle. */ + handle_limit++; + peak_handle_count = 0; + process_open_count = 0; + ret = terminate_process_tree(process, 143); + process = NULL; + cl_assert_equal_i(ret, 0); + cl_assert_equal_i(process_open_count, ARRAY_SIZE(descendants)); + cl_assert_equal_i(peak_handle_count, handle_limit); + cl_assert(GetProcessHandleCount(GetCurrentProcess(), &count)); + cl_assert_equal_i(count + 2, handle_limit); + for (i = 0; i <= ARRAY_SIZE(descendants); i++) { + HANDLE child = i < ARRAY_SIZE(descendants) ? + descendants[i].hProcess : child_process.hProcess; + + cl_assert_equal_i(WaitForSingleObject(child, 10000), + WAIT_OBJECT_0); + cl_assert(GetExitCodeProcess(child, &exit_code)); + cl_assert_equal_i(exit_code, 143); + } +} +#endif