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

[Junio C Hamano <gitster@pobox.com>]

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