[PATCH 1/1] fixup! trace2: collect Windows-specific process information

From: Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>
Date: Feb 11, 2019, 19:12 UTC
Message-ID: <6af9ad6fbbd9ebeb4077ad2167e4e60674b1df75.1549912358.git.gitgitgadget@gmail.com>
In-Reply-To: <pull.123.git.gitgitgadget@gmail.com>

From: Jeff Hostetler <jeffhost@microsoft.com>

Guard against infinite loop while computing the parent process hierarchy.

CreateToolhelp32Snapshot() is used to get a list of all processes on the system. Each process entry contains the process PID and PPID (alive at the time of the snapshot). We compute the set of ancestors of the current process by repeated searches on this list.

Testing revealed that the snapshot can contain PPID cycles. This causes an infinite loop during the set construction and causes the git.exe command to hang.

Testing found an instance where 3 processes were in a PPID cycle. The snapshot implied that each of these processes was its own great-grandparent. This should not be possible unless a PID was recycled and just happened to match up.

For full disclosure, the Windows "System Idle Process" has PID and PPID 0. If it were to launch a Git command, it could cause a similar infinite loop. Or more properly, if any ancestor of the current Git command has PPID 0, it will appear to be a descendant of the idle process and trigger the problem.

This commit fixes both cases by maintaining a list of the PIDs seen during the ancestor walk and stopping if a cycle is detected.

Additionally, code was added to truncate the search after a reasonable fixed depth limit.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/win32/trace2_win32_process_info.c | 32 ++++++++++++++++++------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/compat/win32/trace2_win32_process_info.c b/compat/win32/trace2_win32_process_info.c
index 253199f812..751b366470 100644
--- a/compat/win32/trace2_win32_process_info.c
+++ b/compat/win32/trace2_win32_process_info.c
@@ -3,6 +3,8 @@
 #include <Psapi.h>
 #include <tlHelp32.h>
 
+#define NR_PIDS_LIMIT 42
+
 /*
  * Find the process data for the given PID in the given snapshot
  * and update the PROCESSENTRY32 data.
@@ -21,13 +23,17 @@ static int find_pid(DWORD pid, HANDLE hSnapshot, PROCESSENTRY32 *pe32)
 }
 
 /*
- * Accumulate JSON array:
+ * Accumulate JSON array of our parent processes:
  *     [
  *         exe-name-parent,
  *         exe-name-grand-parent,
  *         ...
  *     ]
  *
+ * We artificially limit this to NR_PIDS_LIMIT to quickly guard against cycles
+ * in the parent PIDs without a lot of fuss and because we just want some
+ * context and don't need an absolute answer.
+ *
  * Note: we only report the filename of the process executable; the
  *       only way to get its full pathname is to use OpenProcess()
  *       and GetModuleFileNameEx() or QueryfullProcessImageName()
@@ -38,16 +44,28 @@ static void get_processes(struct json_writer *jw, HANDLE hSnapshot)
 {
 	PROCESSENTRY32 pe32;
 	DWORD pid;
+	DWORD pid_list[NR_PIDS_LIMIT];
+	int k, nr_pids = 0;
 
 	pid = GetCurrentProcessId();
+	while (find_pid(pid, hSnapshot, &pe32)) {
+		/* Only report parents. Omit self from the JSON output. */
+		if (nr_pids)
+			jw_array_string(jw, pe32.szExeFile);
 
-	/* We only want parent processes, so skip self. */
-	if (!find_pid(pid, hSnapshot, &pe32))
-		return;
-	pid = pe32.th32ParentProcessID;
+		/* Check for cycle in snapshot. (Yes, it happened.) */
+		for (k = 0; k < nr_pids; k++)
+			if (pid == pid_list[k]) {
+				jw_array_string(jw, "(cycle)");
+				return;
+			}
 
-	while (find_pid(pid, hSnapshot, &pe32)) {
-		jw_array_string(jw, pe32.szExeFile);
+		if (nr_pids == NR_PIDS_LIMIT) {
+			jw_array_string(jw, "(truncated)");
+			return;
+		}
+
+		pid_list[nr_pids++] = pid;
 
 		pid = pe32.th32ParentProcessID;
 	}

-- 
gitgitgadget