From 9ddf972ce1d1fbdb52ac58ed182f1859fe309469 Mon Sep 17 00:00:00 2001 From: Bruno Haible Date: Fri, 9 Sep 2022 16:49:10 +0200 Subject: 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. --- ChangeLog | 18 ++++++++++++ lib/spawni.c | 42 ++++++++++++++++++++------- lib/windows-spawn.c | 83 +++++++++++++++++++++++++++++++---------------------- lib/windows-spawn.h | 22 +++++++++----- 4 files changed, 113 insertions(+), 52 deletions(-) diff --git a/ChangeLog b/ChangeLog index c5d5788636..7c237a1e66 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,21 @@ +2022-09-09 Bruno Haible + + 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 spawn-pipe: Fix possible hangs in programs that spawn several children. 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 - */ - 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 + */ + 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, */ 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 */ 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 - */ + . + 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, -- cgit v1.2.1