mingw: avoid terminating unrelated processes during cleanup

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

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;

compat/win32/exit-process.h

@@ -11,7 +11,87 @@
  * For simplicity, these functions are marked as file-local.
  */
 
-#include <tlhelp32.h>
+#include <winternl.h>
+#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,58 +103,70 @@
  */
 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;
-	pid_t pid = GetProcessId(main_process);
+	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;
 
-	pids[0] = (DWORD)pid;
-	len = 1;
+	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)
-					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);
 		}
 	}
 	if (!TerminateProcess(main_process, exit_status))
 		ret = -1;
 	CloseHandle(main_process);
+	free(processes);
 
 	return ret;
 }

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 = [

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