Fix in ash: do DuplicateHandle() after calling mingw_spawn_proc() before calling WaitForMultipleObjects()

[shunf4]

Consider the situation below:

1. evalpipe() is called so that two spawn_forkshell() is to be done
2. The first child process is created by mingw_spawn_proc() in spawn_forkshell(), and its process handle is returned
3. The first child process gets its timeslice and finishes running quickly
4. The second child process is created by mingw_spawn_proc(), in which there are several chances to call cull_exited_processes(), where the first child's handle is closed
5. Backing to ash, in waitpid_child(), WaitForMultipleObjects() is called with handles of the two child processes. But the first one's handle is now invalid
6. The return value(idx) is thus invalid, then waitpid_child() returns -1 and dowait() returns -1
7. Because of the lack of error handling of dowait(), it is called repeatedly inside a while loop in waitforjob().
8. Busybox gets into an endless loop.

Recently I am working on proxychains-windows. It hooks CreateProcessW(), changing the thread scheduling order a little bit, and that causes this problem.

We should DuplicateHandle() in spawn_forkshell(), and properly close the duplicated handle.

Please cherry-pick the earliest commit.

[dscho]

Great contribution, and a very detailed explanation, thank you so much!

I wonder whether you want to open this PR at https://github.com/rmyorston/busybox-w32, too? That's the actual BusyBox-w32 project...

[shunf4]

cull_exited_processes() and friends seems not to exist in rmyorston's fork; so this issue only exists in this fork.

[dscho]

I integrated those patches at long last (but it's still March 2020, right?), which required a bit of adjusting to the current code base. Range-diff to the versions I cherry-picked:

• 1: 6188b90 ! 49: c432a1a ash: fix TRACE printf format and do fflush() after every output

  @@ shell/ash.c: evalpipe(union node *n, int flags)
    	for (lp = n->npipe.cmdlist; lp; lp = lp->next)
    		pipelen++;
   @@ shell/ash.c: evalcommand(union node *cmd, int flags)
  - 	smallint cmd_is_exec;
  + 	errlinno = lineno = cmd->ncmd.linno;
    
    	/* First expand the arguments. */
   -	TRACE(("evalcommand(0x%lx, %d) called\n", (long)cmd, flags));
   +	TRACE(("evalcommand(0x%p, %d) called\n", cmd, flags));
  - 	setstackmark(&smark);
  - 	localvar_stop = pushlocalvars();
  - 	back_exitstatus = 0;
  + #if BASH_PROCESS_SUBST
  + 	redir_stop = redirlist;
  + #endif

• 2: a339bfe < -: --------- ash: fix WNOHANG checking (| -> &)

• 3: 761768b ! 50: 3933933 ash: duplicate proc handle after mingw_spawn_proc

  @@ Commit message
   
    ## shell/ash.c ##
   @@ shell/ash.c: waitpid_child(int *status, int wait_flags)
  - 		return pid;
  + 		*status = (int)win_status << 8;
  + 		if (win_status == 128 + SIGTERM || win_status == 128 + SIGKILL)
  + 			*status += win_status - 128;
  ++	    CloseHandle(proclist[idx]);
  + 		pid = pidlist[idx];
    	}
  - 	GetExitCodeProcess(proclist[idx], &win_status);
  -+	CloseHandle(proclist[idx]);
  - 	pid = pidlist[idx];
  - 	free(pidlist);
  - 	free(proclist);
  -@@ shell/ash.c: spawn_forkshell(struct job *jp, struct forkshell *fs, int mode)
  - 	char buf[16];
  - 	const char *argv[] = { "sh", "--forkshell", NULL, NULL };
  - 	intptr_t ret;
  -+	HANDLE hCurrentProcess;
  - 
  - 	new = forkshell_prepare(fs);
  +  done:
  +@@ shell/ash.c: spawn_forkshell(struct forkshell *fs, struct job *jp, union node *n, int mode)
    	sprintf(buf, "%p", new->hMapFile);
    	argv[2] = buf;
  - 	ret = mingw_spawn_proc(argv);
  + 	ret = spawnve(P_NOWAIT, bb_busybox_exec_path, (char *const *)argv, NULL);
   +	if (ret != -1) {
  -+		hCurrentProcess = GetCurrentProcess();
  ++	    HANDLE hCurrentProcess = GetCurrentProcess();
   +		if (!DuplicateHandle(hCurrentProcess, (HANDLE)ret, hCurrentProcess,
   +				(HANDLE*)&ret, 0, TRUE, DUPLICATE_SAME_ACCESS)) {
   +			ret = -1;

Where a339bfe was already applied in the form of 96c9c00:

• 1: a339bfe ! 1: 96c9c00 ash: fix WNOHANG checking (| -> &)

  @@
    ## Metadata ##
  -Author: shunf4 <[email protected]>
  +Author: Ron Yorston <[email protected]>
   
    ## Commit message ##
  -    ash: fix WNOHANG checking (| -> &)
  +    ash: allow waitpid_child to block
  +
  +    The wrong sort of boolean operator resulted in waitpid_child always
  +    being nonblocking.
   
    ## shell/ash.c ##
   @@ shell/ash.c: waitpid_child(int *status, int wait_flags)
  @@ shell/ash.c: waitpid_child(int *status, int wait_flags)
    
    	idx = WaitForMultipleObjects(pid_nr, proclist, FALSE,
   -				wait_flags|WNOHANG ? 1 : INFINITE);
  -+				(wait_flags&WNOHANG) ? 1 : INFINITE);
  ++				wait_flags&WNOHANG ? 1 : INFINITE);
    	if (idx >= pid_nr) {
    		free(pidlist);
    		free(proclist);