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

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

[Jeff Hostetler via GitGitGadget]

Fix infinite loop observed on Windows computing process ancestry. This
should be applied to V6 of "jh/trace2" currently in next.

See: https://github.com/gitgitgadget/git/pull/108

Thanks Jeff

Cc: gitster@pobox.comCc: peff@peff.netCc: jrnieder@gmail.com

Jeff Hostetler (1):
  fixup! trace2: collect Windows-specific process information

 compat/win32/trace2_win32_process_info.c | 32 ++++++++++++++++++------
 1 file changed, 25 insertions(+), 7 deletions(-)


base-commit: 878e2cd30e1656909c5073043d32fe9d02204daa
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-123%2Fjeffhostetler%2Ftrace2-next-fixup-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-123/jeffhostetler/trace2-next-fixup-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/123
-- 
gitgitgadget

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

[Jeff Hostetler via GitGitGadget]

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

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

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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(-)

Thanks.  Will queue for now, but shortly after the final, I expect
the whole topic to be eligible to graduate to the 'master' track.
At that point, you may want to help rephrase the log message of the
original when this gets squashed in.  Alternatively, we could leave
these two separate patches.


>
> 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
> +

;-)  

Any backstory about the choice of this number we want to share with
our future selves?

>  /*
>   * 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;
>  	}

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

[Jeff Hostetler]

On 2/11/2019 6:19 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Guard against infinite loop while computing the parent process hierarchy.
...
>> ---
>>   compat/win32/trace2_win32_process_info.c | 32 ++++++++++++++++++------
>>   1 file changed, 25 insertions(+), 7 deletions(-)
> 
> Thanks.  Will queue for now, but shortly after the final, I expect
> the whole topic to be eligible to graduate to the 'master' track.
> At that point, you may want to help rephrase the log message of the
> original when this gets squashed in.  Alternatively, we could leave
> these two separate patches.

I'll squash this into the next version of my main patch series
and update the in-code comments to explain.

I thought I saw a note that you were moving my V6 into "next" and
didn't want this patch to "cross in the mail" if I quickly re-rolled
a V7.

Thanks,
Jeff