Re: MinGW open-process, take N

[Eli Zaretskii]

> From: Andy Wingo <address@hidden>
> Cc: address@hidden,  address@hidden,  address@hidden
> Date: Thu, 14 Jul 2016 23:31:31 +0200
> 
> > I see your point, and I think I can fix this, together with the pid_t
> > width issue in 64-bit build, if I introduce an array private to
> > posix-w32.c that will hold the PIDs of all processes known to Guile
> > for which waitpid did not yet return an exit code, and their
> > corresponding process handles.  Then we can return an int to Scheme,
> > and the w32 functions will get hold of the handle by looking up the
> > PID in that array.
> >
> > How's that sound?  If you agree, I can work on this tomorrow.
> 
> That actually sounds really nice!  That way the PID-using functions will
> all see proper PIDS and we also get the waitpid() behavior.
> 
> I would be happy to apply such a patch.  Thank you :-)

Here's the first cut.  (I will rework it into git-format-patch form,
or commit and push myself, whatever is more convenient for you, as
soon as it is okayed for upstream.)

There are two more issues which I need help/guidance to resolve:

 . Access to the procs[] array should be synchronized between
   threads.  (Currently, MinGW builds of Guile don't work at all
   unless built with --disable-threads, but AFAIR Mark wanted to have
   the code thread-safe anyway.)  I guess this entails taking some
   mutex before accessing the array, but I never wrote any such code
   in Guile, so I'd appreciate to be pointed to some example, or to
   have some boilerplate for that.

 . Once a subprocess is launched, its record sits in the procs[] array
   until deleted by waitpid, if it finds that the process has exited,
   or by kill.  If neither waitpid nor kill are called, the process's
   record will not be deleted, even though the process might be long
   dead.  This means that we leak handles, and the system gets process
   objects accumulated that it cannot recycle.  (This problem was
   already present in the previous version of the code, it is not new
   with the modified version.)  Can we be sure that a well-behaving
   Guile program will always call one of these 2 functions?  If not,
   how to prevent that in a well-behaving Guile program?  I guess at
   least close-port should try killing the process (if it doesn't
   already)?  Any other ideas?

Thanks.

Here's the patch:

2016-07-16  Eli Zaretskii  <address@hidden>

        * libguile/posix-w32.c (uname): Update to modern processors (ia64
        and x86_64) and OS versions (Vista to Windows 10).  Delete
        trailing whitespace.
        (proc_record): New structure tag.
        <procs, proc_size>: New static variables.
        (find_proc, proc_handle, record_proc, delete_proc): New utility
        functions.
        (start_child): Return value is now pid_t, as it is on Posix
        platforms.  Record the new process and returns its PID, instead of
        returning a handle.
        (waitpid, kill, getpriority, setpriority, sched_getaffinity)
        (sched_setaffinity): Look up the PID in the recorded subprocesses
        before trying to open a process that is not our subprocess.  Make
        sure any open handle is closed before returning, unless it's our
        subprocess.

--- libguile/posix-w32.c~2      2016-07-16 10:22:08.273250000 +0300
+++ libguile/posix-w32.c        2016-07-16 13:18:51.163875000 +0300
@@ -173,6 +173,80 @@ uname (struct utsname *uts)
   return 0;
 }
 
+/* Utility functions for maintaining the list of subprocesses launched
+   by Guile.  */
+
+struct proc_record {
+  DWORD pid;
+  HANDLE handle;
+};
+
+static struct proc_record *procs;
+static ptrdiff_t proc_size;
+
+/* Find the process slot that corresponds to PID.  Return the index of
+   the slot, or -1 if not found.  */
+static ptrdiff_t
+find_proc (pid_t pid)
+{
+  ptrdiff_t found = -1, i;
+
+  for (i = 0; i < proc_size; i++)
+    {
+      if (procs[i].pid == pid && procs[i].handle != INVALID_HANDLE_VALUE)
+       found = i;
+    }
+
+  return found;
+}
+
+/* Return the process handle corresponding to its PID.  If not found,
+   return invalid handle value.  */
+static HANDLE
+proc_handle (pid_t pid)
+{
+  ptrdiff_t idx = find_proc (pid);
+
+  if (idx < 0)
+    return INVALID_HANDLE_VALUE;
+  return procs[idx].handle;
+}
+
+/* Store a process record in the procs[] array.  */
+static void
+record_proc (pid_t proc_pid, HANDLE proc_handle)
+{
+  ptrdiff_t i;
+
+  /* Find a vacant slot.  */
+  for (i = 0; i < proc_size; i++)
+    {
+      if (procs[i].handle == INVALID_HANDLE_VALUE)
+       break;
+    }
+
+  /* If no vacant slot, enlarge the array.  */
+  if (i == proc_size)
+    {
+      proc_size++;
+      procs = scm_realloc (procs, proc_size * sizeof(procs[0]));
+    }
+
+  /* Store the process data.  */
+  procs[i].pid = proc_pid;
+  procs[i].handle = proc_handle;
+}
+
+/* Delete a process record for process PID.  */
+static void
+delete_proc (pid_t pid)
+{
+  ptrdiff_t idx = find_proc (pid);
+
+  if (0 <= idx && idx < proc_size)
+    procs[idx].handle = INVALID_HANDLE_VALUE;
+}
+
 /* Run a child process with redirected standard handles, without
    redirecting standard handles of the parent.  This is required in
    multithreaded programs, where redirecting a standard handle affects
@@ -527,7 +601,7 @@ prepare_cmdline (const char *cmd, const 
 
    Return the PID of the child process, or -1 if couldn't start a
    process.  */
-int
+pid_t
 start_child (const char *exec_file, char **argv,
             int reading, int c2p[2], int writing, int p2c[2],
              int infd, int outfd, int errfd)
@@ -647,7 +721,10 @@ start_child (const char *exec_file, char
        }
     }
   else
-    pid = (intptr_t)pi.hProcess;
+    {
+      record_proc (pi.dwProcessId, pi.hProcess);
+      pid = pi.dwProcessId;
+    }
 
   errno_save = errno;
 
@@ -683,13 +760,31 @@ start_child (const char *exec_file, char
 
 /* Emulation of waitpid which only supports WNOHANG, since _cwait doesn't.  */
 int
-waitpid (intptr_t pid, int *status, int options)
+waitpid (pid_t pid, int *status, int options)
 {
+  HANDLE ph;
+
+  /* Not supported on MS-Windows.  */
+  if (pid <= 0)
+    {
+      errno = ENOSYS;
+      return -1;
+    }
+
+  ph = proc_handle (pid);
+  /* Since scm_waitpid is documented to work only on child processes,
+     being unable to find a process in our records means failure.  */
+  if (ph == INVALID_HANDLE_VALUE)
+    {
+      errno = ECHILD;
+      return -1;
+    }
+
   if ((options & WNOHANG) != 0)
     {
       DWORD st;
 
-      if (!GetExitCodeProcess ((HANDLE)pid, &st))
+      if (!GetExitCodeProcess (ph, &st))
        {
          errno = ECHILD;
          return -1;
@@ -698,10 +793,14 @@ waitpid (intptr_t pid, int *status, int 
        return 0;
       if (status)
        *status = st;
-      return (int)pid;
+      CloseHandle (ph);
     }
+  else
+    _cwait (status, (intptr_t)ph, WAIT_CHILD);
+
+  delete_proc (pid);
 
-  return (int)_cwait (status, pid, WAIT_CHILD);
+  return pid;
 }
 
 
@@ -763,8 +862,23 @@ w32_status_to_termsig (DWORD status)
 int
 kill (int pid, int sig)
 {
-  HANDLE ph = OpenProcess (PROCESS_TERMINATE, 0, pid);
+  HANDLE ph;
+  int child_proc = 0;
 
+  if (pid == getpid ())
+    {
+      if (raise (sig) == 0)
+       errno = ENOSYS;
+      return -1;
+    }
+
+  ph = proc_handle (pid);
+  /* If not found among our subprocesses, look elsewhere in the
+     system.  */
+  if (ph == INVALID_HANDLE_VALUE)
+    ph = OpenProcess (PROCESS_TERMINATE, 0, pid);
+  else
+    child_proc = 1;
   if (!ph)
     {
       errno = EPERM;
@@ -772,10 +886,19 @@ kill (int pid, int sig)
     }
   if (!TerminateProcess (ph, w32_signal_to_status (sig)))
     {
-      errno = EINVAL;
+      /* If it's our subprocess, it could have already exited.  In
+        that case, waitpid will handily delete the process from our
+        records, and we should return a more meaningful ESRCH to the
+        caller.  */
+      if (child_proc && waitpid (pid, NULL, WNOHANG) == pid)
+       errno = ESRCH;
+      else
+       errno = EINVAL;
       return -1;
     }
   CloseHandle (ph);
+  if (child_proc)
+    delete_proc (pid);
 
   return 0;
 }
@@ -789,6 +912,7 @@ getpriority (int which, int who)
   HANDLE hp;
   int nice_value = -1;
   int error = 0;
+  int child_proc = 0;
 
   /* We don't support process groups and users.  */
   if (which != PRIO_PROCESS)
@@ -800,12 +924,25 @@ getpriority (int which, int who)
   if (who == 0)
     hp = GetCurrentProcess ();
   else
-    hp = OpenProcess (PROCESS_QUERY_INFORMATION, FALSE, who);
+    {
+      hp = proc_handle (who);
+      /* If not found among our subprocesses, look elsewhere in the
+        system.  */
+      if (hp == INVALID_HANDLE_VALUE)
+       hp = OpenProcess (PROCESS_QUERY_INFORMATION, FALSE, who);
+      else
+       child_proc = 1;
+    }
 
   if (hp)
     {
       DWORD pri_class = GetPriorityClass (hp);
 
+      /* The pseudo-handle returned by GetCurrentProcess doesn't need
+        to be closed.  */
+      if (who > 0 && !child_proc)
+       CloseHandle (hp);
+
       if (pri_class > 0)
        {
          switch (pri_class)
@@ -894,6 +1031,7 @@ setpriority (int which, int who, int nic
 {
   HANDLE hp;
   DWORD err;
+  int child_proc = 0, retval = -1;
 
   if (which != PRIO_PROCESS)
     {
@@ -904,7 +1042,15 @@ setpriority (int which, int who, int nic
   if (who == 0)
     hp = GetCurrentProcess ();
   else
-    hp = OpenProcess (PROCESS_SET_INFORMATION, FALSE, who);
+    {
+      hp = proc_handle (who);
+      /* If not found among our subprocesses, look elsewhere in the
+        system.  */
+      if (hp == INVALID_HANDLE_VALUE)
+       hp = OpenProcess (PROCESS_SET_INFORMATION, FALSE, who);
+      else
+       child_proc = 1;
+    }
 
   if (hp)
     {
@@ -926,7 +1072,7 @@ setpriority (int which, int who, int nic
        pri_class = REALTIME_PRIORITY_CLASS;
 
       if (SetPriorityClass (hp, pri_class))
-       return 0;
+       retval = 0;
     }
 
   err = GetLastError ();
@@ -940,8 +1086,12 @@ setpriority (int which, int who, int nic
       errno = EPERM;
       break;
     }
+  /* The pseudo-handle returned by GetCurrentProcess doesn't
+     need to be closed.  */
+  if (hp && who > 0 && !child_proc)
+    CloseHandle (hp);
 
-  return -1;
+  return retval;
 }
 
 /* Emulation of sched_getaffinity and sched_setaffinity.  */
@@ -950,6 +1100,7 @@ sched_getaffinity (int pid, size_t mask_
 {
   HANDLE hp;
   DWORD err;
+  int child_proc = 0;
 
   if (mask == NULL)
     {
@@ -960,14 +1111,24 @@ sched_getaffinity (int pid, size_t mask_
   if (pid == 0)
     hp = GetCurrentProcess ();
   else
-    hp = OpenProcess (PROCESS_QUERY_INFORMATION, FALSE, pid);
+    {
+      hp = proc_handle (pid);
+      /* If not found among our subprocesses, look elsewhere in the
+        system.  */
+      if (hp == INVALID_HANDLE_VALUE)
+       hp = OpenProcess (PROCESS_QUERY_INFORMATION, FALSE, pid);
+      else
+       child_proc = 1;
+    }
 
   if (hp)
     {
       DWORD_PTR ignored;
       BOOL result = GetProcessAffinityMask (hp, (DWORD_PTR *)mask, &ignored);
 
-      if (pid != 0)
+      /* The pseudo-handle returned by GetCurrentProcess doesn't
+        need to be closed.  */
+      if (pid > 0 && !child_proc)
        CloseHandle (hp);
       if (result)
        return 0;
@@ -994,6 +1155,7 @@ sched_setaffinity (int pid, size_t mask_
 {
   HANDLE hp;
   DWORD err;
+  int child_proc = 0;
 
   if (mask == NULL)
     {
@@ -1004,13 +1166,23 @@ sched_setaffinity (int pid, size_t mask_
   if (pid == 0)
     hp = GetCurrentProcess ();
   else
-    hp = OpenProcess (PROCESS_SET_INFORMATION, FALSE, pid);
+    {
+      hp = proc_handle (pid);
+      /* If not found among our subprocesses, look elsewhere in the
+        system.  */
+      if (hp == INVALID_HANDLE_VALUE)
+       hp = OpenProcess (PROCESS_SET_INFORMATION, FALSE, pid);
+      else
+       child_proc = 1;
+    }
 
   if (hp)
     {
       BOOL result = SetProcessAffinityMask (hp, *(DWORD_PTR *)mask);
 
-      if (pid != 0)
+      /* The pseudo-handle returned by GetCurrentProcess doesn't
+        need to be closed.  */
+      if (pid > 0 && !child_proc)
        CloseHandle (hp);
       if (result)
        return 0;