patchfixup! trace2: collect Windows-specific process information

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

> "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.