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 <br...@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
