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

[Bruno Haible]

Since 2020-12-25, the 4 pipe-filter-* tests were hanging on native Windows.
This patch, together with the previous one, fixes the issue.

The change that triggered the malfunction was switching the spawn-pipe
implementation from a code specific to native Windows to a generic code
that uses posix_spawn, in nearly exactly the same way as we use on Unix
platforms.

The root cause was that the old, specific-to-Windows code followed the
algorithm indicated in
<https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/pipe>:
   "To use the _pipe function to communicate between a parent
    process and a child process, each process must have only one
    descriptor open on the pipe. The descriptors must be opposites:
    if the parent has a read descriptor open, then the child must have
    a write descriptor open. The easiest way to do this is to bitwise
    "or" (|) the _O_NOINHERIT flag with textmode. Then, use _dup or
    _dup2 to create an inheritable copy of the pipe descriptor that
    you want to pass to the child. Close the original descriptor, and
    then spawn the child process. On returning from the spawn call,
    close the duplicate descriptor in the parent process."
whereas the new, generic code did it differently: It called _pipe
without the _O_NOINHERIT flag, thus producing file descriptors
which Windows knew were inheritable. Half of these file descriptors
were then closed. But the remaining two file descriptors were inheritable,
and apparently this is sufficient for breaking the "end of input" /
"end of output" recognition. In other words, Windows behaves as if
some other child processes _exist_ which use these HANDLEs, even
though no such child processes actually exist at the given moment.

The previous patch fixed this; but now the HANDLEs are no longer
forwarded to the child process. The fix is to proceed in three
different steps, as described in
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn.html>:
  1. The set of open file descriptors for the child process shall initially
     be the same set as is open for the calling process.
  3. The file actions specified by the spawn file actions object shall be
     performed in the order in which they were added.
  4. Any file descriptor that has its FD_CLOEXEC flag set (see fcntl) shall
     be closed.
I had coded things in such a way that steps 1 and 4 were done together. But
this does not work, because as part of step 3 we have
    dup2 (oldfd, newfd)
instructions, where oldfd has the FD_CLOEXEC flag set and newfd (after the dup2)
call has the FD_CLOEXEC flag cleared, per
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/dup2.html>.

Fixing this, and everything works.

There is another difference between the old, specific-to-Windows code and the
generic code: The former creates a process with process_creation_flags = 0,
whereas the generic code creates a process with process_creation_flags =
DETACHED_PROCESS. But so far this has not caused problems. If someone has a
problem with it, they can use -DSPAWN_PIPE_IMPL_AVOID_POSIX_SPAWN=1.


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.

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,
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/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