spawn-pipe: Fix pipe-filter-* test hangs (regression 2020-12-24).

* lib/windows-spawn.h (struct inheritable_handles): Widen the per-fd
flags from 8 bits to 16 bits.
(KEEP_OPEN_IN_CHILD): New macro.
(init_inheritable_handles): Change description of what it does when
duplicate == true.
* lib/windows-spawn.c (init_inheritable_handles): If duplicate == true,
add all fds to the array, regardless whether they are scheduled to be
preserved in the child process.
(compose_handles_block): Update.
(spawnpvech): Update.
* lib/spawni.c (grow_inheritable_handles): Update.
(shrink_inheritable_handles): Also close the handles not marked with
KEEP_OPEN_IN_CHILD.
(do_open, do_dup2): Mark the new fd with KEEP_OPEN_IN_CHILD.

4 files changed, 113 insertions, 52 deletions

diff --git a/ChangeLog b/ChangeLog
index c5d5788636..7c237a1e66 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,23 @@
 2022-09-09  Bruno Haible  <bruno@clisp.org>
 
+	spawn-pipe: Fix pipe-filter-* test hangs (regression 2020-12-24).
+	* lib/windows-spawn.h (struct inheritable_handles): Widen the per-fd
+	flags from 8 bits to 16 bits.
+	(KEEP_OPEN_IN_CHILD): New macro.
+	(init_inheritable_handles): Change description of what it does when
+	duplicate == true.
+	* lib/windows-spawn.c (init_inheritable_handles): If duplicate == true,
+	add all fds to the array, regardless whether they are scheduled to be
+	preserved in the child process.
+	(compose_handles_block): Update.
+	(spawnpvech): Update.
+	* lib/spawni.c (grow_inheritable_handles): Update.
+	(shrink_inheritable_handles): Also close the handles not marked with
+	KEEP_OPEN_IN_CHILD.
+	(do_open, do_dup2): Mark the new fd with KEEP_OPEN_IN_CHILD.
+
+2022-09-09  Bruno Haible  <bruno@clisp.org>
+
 	spawn-pipe: Fix possible hangs in programs that spawn several children.
 	* lib/spawn-pipe.c (create_pipe) [Unix]: Create the ifd[] and ofd[] file
 	descriptors with the close-on-exec flag set.
diff --git a/lib/spawni.c b/lib/spawni.c
index 9bca2002b6..8125ce19ee 100644
--- a/lib/spawni.c
+++ b/lib/spawni.c
@@ -129,9 +129,9 @@ grow_inheritable_handles (struct inheritable_handles *inh_handles, int newfd)
           errno = ENOMEM;
           return -1;
         }
-      unsigned char *new_flags_array =
-        (unsigned char *)
-        realloc (inh_handles->flags, new_allocated * sizeof (unsigned char));
+      unsigned short *new_flags_array =
+        (unsigned short *)
+        realloc (inh_handles->flags, new_allocated * sizeof (unsigned short));
       if (new_flags_array == NULL)
         {
           free (new_handles_array);
@@ -151,15 +151,33 @@ grow_inheritable_handles (struct inheritable_handles *inh_handles, int newfd)
   return 0;
 }
 
-/* Reduces inh_handles->count to the minimum needed.  */
+/* Closes the handles in inh_handles that are not meant to be preserved in the
+   child process, and reduces inh_handles->count to the minimum needed.  */
 static void
 shrink_inheritable_handles (struct inheritable_handles *inh_handles)
 {
   HANDLE *handles = inh_handles->handles;
+  unsigned short *flags = inh_handles->flags;
+  size_t handles_count = inh_handles->count;
+  unsigned int fd;
+
+  for (fd = 0; fd < handles_count; fd++)
+    {
+      HANDLE handle = handles[fd];
+
+      if (handle != INVALID_HANDLE_VALUE
+          && (flags[fd] & KEEP_OPEN_IN_CHILD) == 0)
+        {
+          CloseHandle (handle);
+          handles[fd] = INVALID_HANDLE_VALUE;
+        }
+    }
+
+  while (handles_count > 3
+         && handles[handles_count - 1] == INVALID_HANDLE_VALUE)
+    handles_count--;
 
-  while (inh_handles->count > 3
-         && handles[inh_handles->count - 1] == INVALID_HANDLE_VALUE)
-    inh_handles->count--;
+  inh_handles->count = handles_count;
 }
 
 /* Closes all handles in inh_handles.  */
@@ -411,7 +429,8 @@ do_open (struct inheritable_handles *inh_handles, int newfd,
       errno = EBADF; /* arbitrary */
       return -1;
     }
-  inh_handles->flags[newfd] = ((flags & O_APPEND) != 0 ? 32 : 0);
+  inh_handles->flags[newfd] =
+    ((flags & O_APPEND) != 0 ? 32 : 0) | KEEP_OPEN_IN_CHILD;
   return 0;
 }
 
@@ -443,7 +462,7 @@ do_dup2 (struct inheritable_handles *inh_handles, int oldfd, int newfd,
           errno = EIO;
           return -1;
         }
-      /* Duplicate the handle, so that it a forthcoming do_close action on oldfd
+      /* Duplicate the handle, so that a forthcoming do_close action on oldfd
          has no effect on newfd.  */
       if (!DuplicateHandle (curr_process, inh_handles->handles[oldfd],
                             curr_process, &inh_handles->handles[newfd],
@@ -452,7 +471,7 @@ do_dup2 (struct inheritable_handles *inh_handles, int oldfd, int newfd,
           errno = EBADF; /* arbitrary */
           return -1;
         }
-      inh_handles->flags[newfd] = 0;
+      inh_handles->flags[newfd] = KEEP_OPEN_IN_CHILD;
     }
   return 0;
 }
@@ -624,7 +643,8 @@ __spawni (pid_t *pid, const char *prog_filename,
         }
     }
 
-  /* Reduce inh_handles.count to the minimum needed.  */
+  /* Close the handles in inh_handles that are not meant to be preserved in the
+     child process, and reduce inh_handles.count to the minimum needed.  */
   shrink_inheritable_handles (&inh_handles);
 
   /* CreateProcess
diff --git a/lib/windows-spawn.c b/lib/windows-spawn.c
index a9212d485e..4c55be0c34 100644
--- a/lib/windows-spawn.c
+++ b/lib/windows-spawn.c
@@ -324,14 +324,21 @@ init_inheritable_handles (struct inheritable_handles *inh_handles,
         HANDLE handle = (HANDLE) _get_osfhandle (fd);
         if (handle != INVALID_HANDLE_VALUE)
           {
-            DWORD hflags;
-            /* GetHandleInformation
-               <https://docs.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-gethandleinformation>  */
-            if (GetHandleInformation (handle, &hflags))
+            if (duplicate)
+              /* We will add fd to the array, regardless of whether it is
+                 inheritable or not.  */
+              break;
+            else
               {
-                if ((hflags & HANDLE_FLAG_INHERIT) != 0)
-                  /* fd denotes an inheritable descriptor.  */
-                  break;
+                DWORD hflags;
+                /* GetHandleInformation
+                   <https://docs.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-gethandleinformation>  */
+                if (GetHandleInformation (handle, &hflags))
+                  {
+                    if ((hflags & HANDLE_FLAG_INHERIT) != 0)
+                      /* fd denotes an inheritable descriptor.  */
+                      break;
+                  }
               }
           }
       }
@@ -348,8 +355,8 @@ init_inheritable_handles (struct inheritable_handles *inh_handles,
       errno = ENOMEM;
       return -1;
     }
-  unsigned char *flags_array =
-    (unsigned char *) malloc (handles_allocated * sizeof (unsigned char));
+  unsigned short *flags_array =
+    (unsigned short *) malloc (handles_allocated * sizeof (unsigned short));
   if (flags_array == NULL)
     {
       free (handles_array);
@@ -374,29 +381,34 @@ init_inheritable_handles (struct inheritable_handles *inh_handles,
                <https://docs.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-gethandleinformation>  */
             if (GetHandleInformation (handle, &hflags))
               {
-                if ((hflags & HANDLE_FLAG_INHERIT) != 0)
+                if (duplicate)
                   {
-                    /* fd denotes an inheritable descriptor.  */
-                    if (duplicate)
+                    /* Add fd to the array, regardless of whether it is
+                       inheritable or not.  */
+                    if (!DuplicateHandle (curr_process, handle,
+                                          curr_process, &handles_array[fd],
+                                          0, TRUE, DUPLICATE_SAME_ACCESS))
                       {
-                        if (!DuplicateHandle (curr_process, handle,
-                                              curr_process, &handles_array[fd],
-                                              0, TRUE, DUPLICATE_SAME_ACCESS))
-                          {
-                            unsigned int i;
-                            for (i = 0; i < fd; i++)
-                              if (handles_array[i] != INVALID_HANDLE_VALUE)
-                                CloseHandle (handles_array[i]);
-                            free (flags_array);
-                            free (handles_array);
-                            errno = EBADF; /* arbitrary */
-                            return -1;
-                          }
+                        unsigned int i;
+                        for (i = 0; i < fd; i++)
+                          if (handles_array[i] != INVALID_HANDLE_VALUE)
+                            CloseHandle (handles_array[i]);
+                        free (flags_array);
+                        free (handles_array);
+                        errno = EBADF; /* arbitrary */
+                        return -1;
+                      }
+                    flags_array[fd] =
+                      ((hflags & HANDLE_FLAG_INHERIT) != 0 ? KEEP_OPEN_IN_CHILD : 0);
+                  }
+                else
+                  {
+                    if ((hflags & HANDLE_FLAG_INHERIT) != 0)
+                      {
+                        /* fd denotes an inheritable descriptor.  */
+                        handles_array[fd] = handle;
+                        flags_array[fd] = KEEP_OPEN_IN_CHILD;
                       }
-                    else
-                      handles_array[fd] = handle;
-
-                    flags_array[fd] = 0;
                   }
               }
           }
@@ -475,7 +487,7 @@ compose_handles_block (const struct inheritable_handles *inh_handles,
         if (handle != INVALID_HANDLE_VALUE
             /* The first three are possibly already passed above.
                But they need to passed here as well, if they have some flags.  */
-            && (fd >= 3 || inh_handles->flags[fd] != 0))
+            && (fd >= 3 || (unsigned char) inh_handles->flags[fd] != 0))
           {
             DWORD hflags;
             /* GetHandleInformation
@@ -490,7 +502,7 @@ compose_handles_block (const struct inheritable_handles *inh_handles,
                        flags[fd] = 1.  But on ReactOS or Wine, adding the bit
                        that indicates the handle type may be necessary.  So,
                        just do it everywhere.  */
-                    flags[fd] = 1 | inh_handles->flags[fd];
+                    flags[fd] = 1 | (unsigned char) inh_handles->flags[fd];
                     switch (GetFileType (handle))
                       {
                       case FILE_TYPE_CHAR:
@@ -619,9 +631,12 @@ spawnpvech (int mode,
       errno = saved_errno;
       return -1;
     }
-  inh_handles.handles[0] = stdin_handle;  inh_handles.flags[0] = 0;
-  inh_handles.handles[1] = stdout_handle; inh_handles.flags[1] = 0;
-  inh_handles.handles[2] = stderr_handle; inh_handles.flags[2] = 0;
+  inh_handles.handles[0] = stdin_handle;
+  inh_handles.flags[0] = KEEP_OPEN_IN_CHILD;
+  inh_handles.handles[1] = stdout_handle;
+  inh_handles.flags[1] = KEEP_OPEN_IN_CHILD;
+  inh_handles.handles[2] = stderr_handle;
+  inh_handles.flags[2] = KEEP_OPEN_IN_CHILD;
 
   /* CreateProcess
      <https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa>  */
diff --git a/lib/windows-spawn.h b/lib/windows-spawn.h
index 0be407bf93..4414e1b3c7 100644
--- a/lib/windows-spawn.h
+++ b/lib/windows-spawn.h
@@ -85,10 +85,12 @@ extern char * compose_envblock (const char * const *envp)
   _GL_ATTRIBUTE_MALLOC _GL_ATTRIBUTE_DEALLOC_FREE;
 
 
-/* This struct keeps track of which handles to pass to a subprocess, and with
-   which flags.  All of the handles here are inheritable.
+/* This struct keeps track of which handles to potentially pass to a subprocess,
+   and with which flags.  All of the handles here are inheritable.
    Regarding handle inheritance, see
-   <https://docs.microsoft.com/en-us/windows/win32/sysinfo/handle-inheritance>  */
+   <https://docs.microsoft.com/en-us/windows/win32/sysinfo/handle-inheritance>.
+   Whether a handle is actually scheduled for being preserved in the child
+   process is determined by the KEEP_OPEN_IN_CHILD bit in the flags.  */
 struct inheritable_handles
 {
   /* The number of occupied entries in the two arrays below.
@@ -103,13 +105,19 @@ struct inheritable_handles
      flags[fd] is only relevant if handles[fd] != INVALID_HANDLE_VALUE.
      It is a bit mask consisting of:
        - 32 for O_APPEND.
+       - KEEP_OPEN_IN_CHILD if handles[fd] is scheduled to be preserved in the
+         child process.
    */
-  unsigned char *flags;
+  unsigned short *flags;
+  #define KEEP_OPEN_IN_CHILD 0x100
 };
 
-/* Initializes a set of inheritable handles, filling in all inheritable handles
-   assigned to file descriptors.
-   If DUPLICATE is true, the handles stored in the set are duplicates.
+/* Initializes a set of inheritable handles, filling in all or part of the
+   file descriptors of the current process.
+   If DUPLICATE is true, the handles stored are those of all file descriptors,
+   and we use DuplicateHandle to make sure that they are all inheritable.
+   If DUPLICATE is false, the handles stored are only the inheritables ones;
+   this is a shortcut for spawnpvech().
    Returns 0 upon success.  In case of failure, -1 is returned, with errno set.
  */
 extern int init_inheritable_handles (struct inheritable_handles *inh_handles,