mingw: guard process tree traversal against PID reuse

compat/mingw.c

@@ -48,6 +48,49 @@ void open_in_gdb(void)
 	sleep(1);
 }
 
+typedef NTSTATUS (NTAPI *nt_query_information_process_fn)(
+	HANDLE, PROCESSINFOCLASS, PVOID, ULONG, PULONG);
+
+int mingw_process_started_after(HANDLE process, const FILETIME *time,
+				FILETIME *creation_time)
+{
+	FILETIME exit_time, kernel_time, user_time, actual_creation_time;
+
+	if (!creation_time)
+		creation_time = &actual_creation_time;
+
+	return GetProcessTimes(process, creation_time, &exit_time,
+			       &kernel_time, &user_time) &&
+		(!time || CompareFileTime(creation_time, time) >= 0);
+}
+
+static int process_has_parent(HANDLE process, DWORD parent_pid)
+{
+	static struct proc_addr proc_addr_NtQueryInformationProcess =
+		{ "ntdll.dll", "NtQueryInformationProcess", NULL, 0 };
+	nt_query_information_process_fn NtQueryInformationProcess;
+	PROCESS_BASIC_INFORMATION process_info;
+
+	NtQueryInformationProcess =
+		(nt_query_information_process_fn)get_proc_addr(
+			&proc_addr_NtQueryInformationProcess);
+	return NtQueryInformationProcess &&
+		NT_SUCCESS(NtQueryInformationProcess(process, ProcessBasicInformation,
+						     &process_info,
+						     sizeof(process_info), NULL)) &&
+		(DWORD)(uintptr_t)process_info.InheritedFromUniqueProcessId ==
+		parent_pid;
+}
+
+int mingw_process_is_child_of(HANDLE process, DWORD parent_pid,
+				      const FILETIME *parent_creation_time,
+				      FILETIME *creation_time)
+{
+	return process_has_parent(process, parent_pid) &&
+		mingw_process_started_after(process, parent_creation_time,
+					  creation_time);
+}
+
 int err_win_to_posix(DWORD winerr)
 {
 	int error = ENOSYS;
@@ -2552,12 +2595,19 @@ int mingw_kill(pid_t pid, int sig)
 		if (h)
 			ret = exit_process(h, 128 + sig);
 		else {
-			h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
-			if (!h) {
-				errno = err_win_to_posix(GetLastError());
-				return -1;
+			h = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_TERMINATE,
+					FALSE, pid);
+			if (h)
+				ret = terminate_process_tree(h, 128 + sig);
+			else {
+				h = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
+				if (!h) {
+					errno = err_win_to_posix(GetLastError());
+					return -1;
+				}
+				ret = TerminateProcess(h, 128 + sig) ? 0 : -1;
+				CloseHandle(h);
 			}
-			ret = terminate_process_tree(h, 128 + sig);
 		}
 		if (ret)
 			errno = err_win_to_posix(GetLastError());

compat/mingw.h

@@ -284,6 +284,21 @@ void open_in_gdb(void);
  */
 int err_win_to_posix(DWORD winerr);
 
+/*
+ * Get a process's creation time and check that it started no earlier than the
+ * supplied time. If requested, also return the process's creation time.
+ */
+int mingw_process_started_after(HANDLE process, const FILETIME *time,
+				FILETIME *creation_time);
+
+/*
+ * Check whether a process still identifies the supplied process as its parent
+ * and was created no earlier than that parent.
+ */
+int mingw_process_is_child_of(HANDLE process, DWORD parent_pid,
+				      const FILETIME *parent_creation_time,
+				      FILETIME *creation_time);
+
 int mingw_platform_has_symlinks(void);
 #define platform_has_symlinks() mingw_platform_has_symlinks()
 

compat/win32/exit-process.h

@@ -21,16 +21,61 @@
  * no chance of cleaning up after themselves (closing file handles, removing
  * .lock files, terminating spawned processes (if any), etc).
  */
+struct process_info {
+	DWORD pid;
+	FILETIME creation_time;
+	HANDLE handle;
+};
+
+static int add_process(struct process_info *processes, int len, int parent,
+		       const PROCESSENTRY32 *entry)
+{
+	HANDLE process;
+	FILETIME creation_time;
+
+	if (processes[parent].pid != entry->th32ParentProcessID)
+		return 0;
+
+	process = OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_TERMINATE,
+			      FALSE, entry->th32ProcessID);
+	if (!process)
+		return 0;
+
+	if (!mingw_process_is_child_of(process, processes[parent].pid,
+					       &processes[parent].creation_time,
+					       &creation_time)) {
+		CloseHandle(process);
+		return 0;
+	}
+
+	processes[len].pid = entry->th32ProcessID;
+	processes[len].creation_time = creation_time;
+	processes[len].handle = process;
+	return 1;
+}
+
 static int terminate_process_tree(HANDLE main_process, int exit_status)
 {
-	HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+	HANDLE snapshot;
 	PROCESSENTRY32 entry;
-	DWORD pids[16384];
-	int max_len = sizeof(pids) / sizeof(*pids), i, len, ret = 0;
-	pid_t pid = GetProcessId(main_process);
+	int max_len = 16384, i, len, ret = 0;
+	struct process_info *processes = xcalloc(max_len, sizeof(*processes));
 
-	pids[0] = (DWORD)pid;
+	/*
+	 * A process's PID can be reused after it exits. Do not mistake an older
+	 * orphan whose former parent happens to have the same PID for a child.
+	 *
+	 * Keep a handle for every accepted child. This both gives us the handle for
+	 * termination and keeps the process object alive so its PID cannot be
+	 * reused while walking the rest of the tree.
+	 */
+	if (!mingw_process_started_after(main_process, NULL,
+					 &processes[0].creation_time))
+		goto terminate_main_process;
+	processes[0].pid = GetProcessId(main_process);
+	processes[0].handle = main_process;
 	len = 1;
+	snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
 
 	/*
 	 * Even if Process32First()/Process32Next() seem to traverse the
@@ -41,41 +86,48 @@ static int terminate_process_tree(HANDLE main_process, int exit_status)
 	 * Therefore, run through them at least twice and stop when no more
 	 * process IDs were added to the list.
 	 */
-	for (;;) {
-		int orig_len = len;
-
-		memset(&entry, 0, sizeof(entry));
-		entry.dwSize = sizeof(entry);
-
-		if (!Process32First(snapshot, &entry))
-			break;
-
-		do {
-			for (i = len - 1; i >= 0; i--) {
-				if (pids[i] == entry.th32ProcessID)
+	if (snapshot != INVALID_HANDLE_VALUE) {
+		for (;;) {
+			int orig_len = len;
+
+			memset(&entry, 0, sizeof(entry));
+			entry.dwSize = sizeof(entry);
+
+			if (!Process32First(snapshot, &entry))
+				break;
+
+			do {
+				for (i = len - 1; i >= 0; i--) {
+					if (processes[i].pid == entry.th32ProcessID)
+						break;
+					if (processes[i].pid != entry.th32ParentProcessID)
+						continue;
+					if (add_process(processes, len, i, &entry))
+						len++;
 					break;
-				if (pids[i] == entry.th32ParentProcessID)
-					pids[len++] = entry.th32ProcessID;
-			}
-		} while (len < max_len && Process32Next(snapshot, &entry));
+				}
+			} while (len < max_len && Process32Next(snapshot, &entry));
 
-		if (orig_len == len || len >= max_len)
-			break;
+			if (orig_len == len || len >= max_len)
+				break;
+		}
+		CloseHandle(snapshot);
 	}
 
-	for (i = len - 1; i > 0; i--) {
-		HANDLE process = OpenProcess(PROCESS_TERMINATE, FALSE, pids[i]);
-
-		if (process) {
-			if (!TerminateProcess(process, exit_status))
-				ret = -1;
-			CloseHandle(process);
-		}
+	for (i = len - 1; i >= 0; i--) {
+		if (!TerminateProcess(processes[i].handle, exit_status))
+			ret = -1;
+		CloseHandle(processes[i].handle);
 	}
+	free(processes);
+
+	return ret;
+
+terminate_main_process:
 	if (!TerminateProcess(main_process, exit_status))
 		ret = -1;
 	CloseHandle(main_process);
-
+	free(processes);
 	return ret;
 }
 

t/unit-tests/u-mingw.c

@@ -70,3 +70,76 @@ void test_mingw__errno_translation(void)
     cl_skip();
 #endif
 }
+
+void test_mingw__process_creation_time(void)
+{
+#if defined(GIT_WINDOWS_NATIVE)
+	FILETIME creation_time, observed_creation_time, now;
+
+	cl_assert(mingw_process_started_after(GetCurrentProcess(), NULL,
+					     &creation_time));
+	cl_assert(mingw_process_started_after(GetCurrentProcess(),
+					     &creation_time,
+					     &observed_creation_time));
+	cl_assert_equal_i(CompareFileTime(&creation_time,
+					     &observed_creation_time), 0);
+
+	GetSystemTimeAsFileTime(&now);
+	cl_assert(!mingw_process_started_after(GetCurrentProcess(),
+					     &now, NULL));
+#else
+	cl_skip();
+#endif
+}
+
+#if defined(GIT_WINDOWS_NATIVE)
+static PROCESS_INFORMATION child_process;
+static int child_process_running;
+
+static void cleanup_child_process(void *unused)
+{
+	(void)unused;
+	if (child_process_running) {
+		TerminateProcess(child_process.hProcess, 0);
+		WaitForSingleObject(child_process.hProcess, INFINITE);
+		CloseHandle(child_process.hThread);
+		CloseHandle(child_process.hProcess);
+		child_process_running = 0;
+	}
+}
+#endif
+
+void test_mingw__process_child_identity(void)
+{
+#if defined(GIT_WINDOWS_NATIVE)
+	STARTUPINFOA startup_info = { .cb = sizeof(startup_info) };
+	FILETIME parent_creation_time, time_after_child, child_creation_time;
+	char command[] = "ping.exe -n 5 127.0.0.1";
+
+	cl_assert(CreateProcessA(NULL, command, NULL, NULL, FALSE,
+				       CREATE_NO_WINDOW, NULL, NULL, &startup_info,
+				       &child_process));
+	child_process_running = 1;
+	cl_set_cleanup(cleanup_child_process, NULL);
+	Sleep(100);
+	GetSystemTimeAsFileTime(&time_after_child);
+
+	/* A child cannot predate the process that is claimed as its parent. */
+	cl_assert(!mingw_process_is_child_of(child_process.hProcess,
+					       GetCurrentProcessId(),
+					       &time_after_child, NULL));
+
+	cl_assert(mingw_process_started_after(GetCurrentProcess(), NULL,
+					     &parent_creation_time));
+	cl_assert(mingw_process_is_child_of(child_process.hProcess,
+					       GetCurrentProcessId(),
+					       &parent_creation_time,
+					       &child_creation_time));
+	cl_assert(CompareFileTime(&child_creation_time,
+					  &parent_creation_time) >= 0);
+	cl_assert(!mingw_process_is_child_of(child_process.hProcess, 0,
+					       &parent_creation_time, NULL));
+#else
+	cl_skip();
+#endif
+}