* fd used for saving redirected fds leaked to child processes @ 2017-08-13 16:12 Stephane Chazelas 2017-08-13 18:49 ` Bart Schaefer 2017-08-13 18:49 ` Peter Stephenson 0 siblings, 2 replies; 12+ messages in thread From: Stephane Chazelas @ 2017-08-13 16:12 UTC (permalink / raw) To: Zsh hackers list In: mkfifo fifo zsh -c '{ echo GO > fifo & echo $!; } > pid; echo done' | cat "cat" hangs until some process open the fifo in read mode, even though that "echo GO > fifo" command was run in background and "done" is output straight away. What we see is the child zsh process opening the "fifo" having a fd open on the pipe to cat: zsh 28400 chazelas 12w FIFO 0,10 0t0 125690 pipe 28399,cat,0r I think that fd was the one that was dupped from stdout by its parent to /save/ stdout before doing the > pid redirection (so it can be used to restore stdout after the command group returns). That fd is not needed/used in the child so should be closed there. (here, the work around and more obvious way to do it is with zsh -c 'echo GO > fifo & echo $! > pid; echo done' | cat that was just an example to illustrate the problem). bash and dash seem to also have the problem. pdksh, ksh93 and yash are OK. Looking at strace outputs, they seem to be closing those fds in the children. The Bourne shell and rc are OK, but only because they don't use a dupped fd to "save" stdout when redirecting compound commands. They run the compound command in a subshell. Note that those fds have the CLOEXEC flag, so the circumstances in which it becomes a problem are few. -- Stephane ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-13 16:12 fd used for saving redirected fds leaked to child processes Stephane Chazelas @ 2017-08-13 18:49 ` Bart Schaefer 2017-08-13 18:49 ` Peter Stephenson 1 sibling, 0 replies; 12+ messages in thread From: Bart Schaefer @ 2017-08-13 18:49 UTC (permalink / raw) To: Zsh hackers list On Sun, Aug 13, 2017 at 9:12 AM, Stephane Chazelas <stephane.chazelas@gmail.com> wrote: > In: > > mkfifo fifo > zsh -c '{ echo GO > fifo & echo $!; } > pid; echo done' | cat > > "cat" hangs until some process open the fifo in read mode, even > though that "echo GO > fifo" command was run in background and > "done" is output straight away. Hmm. Here in a bit more detail is what I believe is going on: There is a parent shell (probably also zsh in this example) managing the two processes "zsh -c ..." and "cat". Following zsh's usual practice, the "zsh -c" process has been forked (in case "cat" is a builtin) and then "cat" has also been forked (because it's not a builtin). Thus the stdin of "cat" is connected to the stdout of "zsh -c" as you would expect. Looking inside that "zsh -c", the "{ ... }" construct is supposed to be executed in the current shell. So *that* zsh saves its stdout as fd 12, redirects the "{ ... }" output to the "pid" file, and begins handling what's inside the braces. At this point fd 12 cannot be closed, because the current shell still needs it in order to restore stdout. Now zsh does "echo GO > file &". It is after zsh forks to background this process that fd 12 might become irrelevant. However, (the subtle bit), zsh attempts to process all the redirections first, before doing the cleanup of the file descriptors that the child process doesn't need. So it blocks on opening the fifo with fd 12 still open, even though it would eventually close fd 12 before "echo" is executed. You can demonstrate this because even using /bin/echo in the example above, which would invoke the close-on-exec flag, still blocks in exactly the same way. The question is how it's possible to address this without "accidentally" closing descriptors that might legitimately be managed with further redirections e.g. X>&Y. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-13 16:12 fd used for saving redirected fds leaked to child processes Stephane Chazelas 2017-08-13 18:49 ` Bart Schaefer @ 2017-08-13 18:49 ` Peter Stephenson 2017-08-13 21:45 ` Bart Schaefer 1 sibling, 1 reply; 12+ messages in thread From: Peter Stephenson @ 2017-08-13 18:49 UTC (permalink / raw) To: Zsh hackers list On Sun, 13 Aug 2017 17:12:07 +0100 Stephane Chazelas <stephane.chazelas@gmail.com> wrote: > In: > > mkfifo fifo > zsh -c '{ echo GO > fifo & echo $!; } > pid; echo done' | cat > > "cat" hangs until some process open the fifo in read mode, even > though that "echo GO > fifo" command was run in background and > "done" is output straight away. > > What we see is the child zsh process opening the "fifo" having > a fd open on the pipe to cat: > > zsh 28400 chazelas 12w FIFO 0,10 0t0 125690 pipe 28399,cat,0r > > I think that fd was the one that was dupped from stdout by its > parent to /save/ stdout before doing the > pid redirection (so > it can be used to restore stdout after the command group > returns). That explanation makes sense, in which case I think this fix should be good enough. pws diff --git a/Src/exec.c b/Src/exec.c index f339dd6..61ae5e8 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -947,6 +947,14 @@ hashcmd(char *arg0, char **pp) int forklevel; +/* FDs saved for possible restoring, not needed in a subshell + * where we will never need to restore them. Hence if we enter + * a subshell these will simply be closed unconditionally. + * + * A value >= 10 indicates a valid saved fd. + */ +static int saved_fds[10]; + /* Arguments to entersubsh() */ enum { /* Subshell is to be run asynchronously (else synchronously) */ @@ -972,7 +980,7 @@ enum { static void entersubsh(int flags) { - int sig, monitor, job_control_ok; + int i, sig, monitor, job_control_ok; if (!(flags & ESUB_KEEPTRAP)) for (sig = 0; sig < SIGCOUNT; sig++) @@ -1083,6 +1091,12 @@ entersubsh(int flags) opts[MONITOR] = 0; opts[USEZLE] = 0; zleactive = 0; + for (i = 0; i != 10; i++) { + if (saved_fds[i] >= 10) { + close(saved_fds[i]); + saved_fds[i] = 0; + } + } if (flags & ESUB_PGRP) clearjobtab(monitor); get_usage(); @@ -2318,6 +2332,7 @@ addfd(int forked, int *save, struct multio **mfds, int fd1, int fd2, int rflag, return; } save[fd1] = fdN; + saved_fds[fd1] = fdN; } } } @@ -4249,9 +4264,12 @@ fixfds(int *save) int old_errno = errno; int i; - for (i = 0; i != 10; i++) + for (i = 0; i != 10; i++) { if (save[i] != -2) redup(save[i], i); + if (save[i] >= 10) + saved_fds[i] = 0; + } errno = old_errno; } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-13 18:49 ` Peter Stephenson @ 2017-08-13 21:45 ` Bart Schaefer 2017-08-14 9:09 ` Peter Stephenson 0 siblings, 1 reply; 12+ messages in thread From: Bart Schaefer @ 2017-08-13 21:45 UTC (permalink / raw) To: Zsh hackers list On Sun, Aug 13, 2017 at 11:49 AM, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote: > > +/* FDs saved for possible restoring, not needed in a subshell > + * where we will never need to restore them. Hence if we enter > + * a subshell these will simply be closed unconditionally. > + * > + * A value >= 10 indicates a valid saved fd. > + */ No objection to defensive programming, but would it not be a bug for an fd < 10 to ever be assigned to a slot in saved_fds[] in the first place? It feels as though there's probably a reason that (int *save) is passed around as a parameter to addfd() et al. rather than being a global to begin with. Are you sure there aren't circumstances where the same fd might get saved more than once at different levels of the recursive execsomething() hierarchy, causing saved_fds[] to contain incomplete information? I'm envisioning something like { { cmd1 ; cmd2 } > file2; cmd3 } > file1 & ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-13 21:45 ` Bart Schaefer @ 2017-08-14 9:09 ` Peter Stephenson 2017-08-14 14:49 ` Bart Schaefer 0 siblings, 1 reply; 12+ messages in thread From: Peter Stephenson @ 2017-08-14 9:09 UTC (permalink / raw) To: Zsh hackers list On Sun, 13 Aug 2017 14:45:12 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Sun, Aug 13, 2017 at 11:49 AM, Peter Stephenson > <p.w.stephenson@ntlworld.com> wrote: > > > > +/* FDs saved for possible restoring, not needed in a subshell > > + * where we will never need to restore them. Hence if we enter > > + * a subshell these will simply be closed unconditionally. > > + * > > + * A value >= 10 indicates a valid saved fd. > > + */ > > No objection to defensive programming, but would it not be a bug for > an fd < 10 to ever be assigned to a slot in saved_fds[] in the first > place? Yes, it would --- I just need to exclude the case of 0 as that's the default (wasn't worth statically initialising to -1's), but checked the range instead. > It feels as though there's probably a reason that (int *save) is > passed around as a parameter to addfd() et al. rather than being a > global to begin with. Are you sure there aren't circumstances where > the same fd might get saved more than once at different levels of the > recursive execsomething() hierarchy, causing saved_fds[] to contain > incomplete information? I'm envisioning something like > > { { cmd1 ; cmd2 } > file2; cmd3 } > file1 & Yes, exactly that did occur to me later. We need to expose the entire hierarchy for this particular case, but that looks like a stack or a linked list, which seems a little heavyweight for this case. It needs a bit more thought. A linked list rooted at each fd 0 to 9 is the best I've come up with so far. pws ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-14 9:09 ` Peter Stephenson @ 2017-08-14 14:49 ` Bart Schaefer 2017-08-14 15:24 ` Peter Stephenson ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Bart Schaefer @ 2017-08-14 14:49 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh hackers list On Mon, Aug 14, 2017 at 2:09 AM, Peter Stephenson <p.stephenson@samsung.com> wrote: > > We need to expose the entire hierarchy for this particular case, but > that looks like a stack or a linked list, which seems a little > heavyweight for this case. Another flag in the fdtable array? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-14 14:49 ` Bart Schaefer @ 2017-08-14 15:24 ` Peter Stephenson 2017-08-14 15:31 ` Bart Schaefer 2017-08-14 19:19 ` Peter Stephenson 2017-08-15 18:42 ` Peter Stephenson 2 siblings, 1 reply; 12+ messages in thread From: Peter Stephenson @ 2017-08-14 15:24 UTC (permalink / raw) To: Zsh hackers list On Mon, 14 Aug 2017 07:49:52 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Mon, Aug 14, 2017 at 2:09 AM, Peter Stephenson > <p.stephenson@samsung.com> wrote: > > > > We need to expose the entire hierarchy for this particular case, but > > that looks like a stack or a linked list, which seems a little > > heavyweight for this case. > > Another flag in the fdtable array? That probably works --- by default we've got to search 64 fd's for the flag (the usual size for the fdtable if we don't need to enlarge it) unless we add a (bug-prone) max saved index, but arguably if that seems a lot it's the 64 itself that's the target for optimisation as it's already widely used. pws ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-14 15:24 ` Peter Stephenson @ 2017-08-14 15:31 ` Bart Schaefer 0 siblings, 0 replies; 12+ messages in thread From: Bart Schaefer @ 2017-08-14 15:31 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh hackers list On Mon, Aug 14, 2017 at 8:24 AM, Peter Stephenson <p.stephenson@samsung.com> wrote: > On Mon, 14 Aug 2017 07:49:52 -0700 > Bart Schaefer <schaefer@brasslantern.com> wrote: >> >> Another flag in the fdtable array? > > That probably works --- by default we've got to search 64 fd's Perhaps something that it would work to put in a global is a value for max_saved_fd so we only have to search from 11 - max no matter the size of the actual fdtable array. The max could be reset when the fds are closed. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-14 14:49 ` Bart Schaefer 2017-08-14 15:24 ` Peter Stephenson @ 2017-08-14 19:19 ` Peter Stephenson 2017-09-29 15:06 ` Daniel Shahaf 2017-08-15 18:42 ` Peter Stephenson 2 siblings, 1 reply; 12+ messages in thread From: Peter Stephenson @ 2017-08-14 19:19 UTC (permalink / raw) To: Zsh hackers list On Mon, 14 Aug 2017 07:49:52 -0700 Bart Schaefer <schaefer@brasslantern.com> wrote: > On Mon, Aug 14, 2017 at 2:09 AM, Peter Stephenson > <p.stephenson@samsung.com> wrote: > > > > We need to expose the entire hierarchy for this particular case, but > > that looks like a stack or a linked list, which seems a little > > heavyweight for this case. > > Another flag in the fdtable array? This does that --- this looks easy. It didn't seem convenient to optimise, since once one set of saved fds is restored, exposing any in an enclosing scope, there's no way of working out which the new highest number is without an exhaustive search. pws diff --git a/Src/exec.c b/Src/exec.c index f339dd6..9996dff 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -972,7 +972,7 @@ enum { static void entersubsh(int flags) { - int sig, monitor, job_control_ok; + int i, sig, monitor, job_control_ok; if (!(flags & ESUB_KEEPTRAP)) for (sig = 0; sig < SIGCOUNT; sig++) @@ -1083,6 +1083,14 @@ entersubsh(int flags) opts[MONITOR] = 0; opts[USEZLE] = 0; zleactive = 0; + /* + * If we've saved fd's for later restoring, we're never going + * to restore them now, so just close them. + */ + for (i = 10; i <= max_zsh_fd; i++) { + if (fdtable[i] & FDT_SAVED_MASK) + zclose(i); + } if (flags & ESUB_PGRP) clearjobtab(monitor); get_usage(); @@ -2318,6 +2326,9 @@ addfd(int forked, int *save, struct multio **mfds, int fd1, int fd2, int rflag, return; } save[fd1] = fdN; + DPUTS(fdtable[fdN] != FDT_INTERNAL, + "Saved file descriptor not marked as internal"); + fdtable[fdN] |= FDT_SAVED_MASK; } } } @@ -3575,7 +3586,8 @@ execcmd_exec(Estate state, Execcmd_params eparams, } if (!bad && fn->fd1 <= max_zsh_fd) { if (fn->fd1 >= 10 && - fdtable[fn->fd1] == FDT_INTERNAL) + (fdtable[fn->fd1] & FDT_TYPE_MASK) == + FDT_INTERNAL) bad = 3; } } @@ -4270,7 +4282,7 @@ closem(int how) for (i = 10; i <= max_zsh_fd; i++) if (fdtable[i] != FDT_UNUSED && - (how == FDT_UNUSED || fdtable[i] == how)) { + (how == FDT_UNUSED || (fdtable[i] & FDT_TYPE_MASK) == how)) { if (i == SHTTY) SHTTY = -1; zclose(i); diff --git a/Src/zsh.h b/Src/zsh.h index ccd11db..f42ada7 100644 --- a/Src/zsh.h +++ b/Src/zsh.h @@ -456,6 +456,18 @@ enum { #define FDT_PROC_SUBST 7 #endif +/* + * Mask to get the basic FDT type. + */ +#define FDT_TYPE_MASK 15 + +/* + * Bit flag that fd is saved for later restoration. + * Currently this is only use with FDT_INTERNAL. We use this fact so as + * not to have to mask checks against other types. + */ +#define FDT_SAVED_MASK 16 + /* Flags for input stack */ #define INP_FREE (1<<0) /* current buffer can be free'd */ #define INP_ALIAS (1<<1) /* expanding alias or history */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-14 19:19 ` Peter Stephenson @ 2017-09-29 15:06 ` Daniel Shahaf 2017-09-29 15:18 ` Peter Stephenson 0 siblings, 1 reply; 12+ messages in thread From: Daniel Shahaf @ 2017-09-29 15:06 UTC (permalink / raw) To: Peter Stephenson; +Cc: Zsh hackers list Peter Stephenson wrote on Mon, 14 Aug 2017 20:19 +0100: > On Mon, 14 Aug 2017 07:49:52 -0700 > Bart Schaefer <schaefer@brasslantern.com> wrote: > > On Mon, Aug 14, 2017 at 2:09 AM, Peter Stephenson > > <p.stephenson@samsung.com> wrote: > > > > > > We need to expose the entire hierarchy for this particular case, but > > > that looks like a stack or a linked list, which seems a little > > > heavyweight for this case. > > > > Another flag in the fdtable array? > > This does that --- this looks easy. It didn't seem convenient to > optimise, since once one set of saved fds is restored, exposing any in > an enclosing scope, there's no way of working out which the new highest > number is without an exhaustive search. This patch introduces a new warning: % Src/zsh -fc ': 3>&1' 1: Src/exec.c:2330: Saved file descriptor not marked as internal It's commit ddb86759012992fa6471bd738f906a4ed434b69f (the log message doesn't include the X-Seq). Found this via z-sy-h's "make quiet-test". Cheers, Daniel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-09-29 15:06 ` Daniel Shahaf @ 2017-09-29 15:18 ` Peter Stephenson 0 siblings, 0 replies; 12+ messages in thread From: Peter Stephenson @ 2017-09-29 15:18 UTC (permalink / raw) To: Daniel Shahaf, Zsh hackers list On Fri, 29 Sep 2017 15:06:24 +0000 Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > % Src/zsh -fc ': 3>&1' > 1: Src/exec.c:2330: Saved file descriptor not marked as internal That happens if the file descriptor was already closed, in which case the new code is inapplicable, so this would access off the start of the array. pws diff --git a/Src/exec.c b/Src/exec.c index 0d2dc4e..f2e187a 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -2325,16 +2325,19 @@ addfd(int forked, int *save, struct multio **mfds, int fd1, int fd2, int rflag, * fd1 may already be closed here, so * ignore bad file descriptor error */ - if (fdN < 0 && errno != EBADF) { - zerr("cannot duplicate fd %d: %e", fd1, errno); - mfds[fd1] = NULL; - closemnodes(mfds); - return; + if (fdN < 0) { + if (errno != EBADF) { + zerr("cannot duplicate fd %d: %e", fd1, errno); + mfds[fd1] = NULL; + closemnodes(mfds); + return; + } + } else { + DPUTS(fdtable[fdN] != FDT_INTERNAL, + "Saved file descriptor not marked as internal"); + fdtable[fdN] |= FDT_SAVED_MASK; } save[fd1] = fdN; - DPUTS(fdtable[fdN] != FDT_INTERNAL, - "Saved file descriptor not marked as internal"); - fdtable[fdN] |= FDT_SAVED_MASK; } } } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: fd used for saving redirected fds leaked to child processes 2017-08-14 14:49 ` Bart Schaefer 2017-08-14 15:24 ` Peter Stephenson 2017-08-14 19:19 ` Peter Stephenson @ 2017-08-15 18:42 ` Peter Stephenson 2 siblings, 0 replies; 12+ messages in thread From: Peter Stephenson @ 2017-08-15 18:42 UTC (permalink / raw) To: Zsh hackers list I'm not sure what happened to this. pws diff --git a/Src/exec.c b/Src/exec.c index f339dd6..9996dff 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -972,7 +972,7 @@ enum { static void entersubsh(int flags) { - int sig, monitor, job_control_ok; + int i, sig, monitor, job_control_ok; if (!(flags & ESUB_KEEPTRAP)) for (sig = 0; sig < SIGCOUNT; sig++) @@ -1083,6 +1083,14 @@ entersubsh(int flags) opts[MONITOR] = 0; opts[USEZLE] = 0; zleactive = 0; + /* + * If we've saved fd's for later restoring, we're never going + * to restore them now, so just close them. + */ + for (i = 10; i <= max_zsh_fd; i++) { + if (fdtable[i] & FDT_SAVED_MASK) + zclose(i); + } if (flags & ESUB_PGRP) clearjobtab(monitor); get_usage(); @@ -2318,6 +2326,9 @@ addfd(int forked, int *save, struct multio **mfds, int fd1, int fd2, int rflag, return; } save[fd1] = fdN; + DPUTS(fdtable[fdN] != FDT_INTERNAL, + "Saved file descriptor not marked as internal"); + fdtable[fdN] |= FDT_SAVED_MASK; } } } @@ -3575,7 +3586,8 @@ execcmd_exec(Estate state, Execcmd_params eparams, } if (!bad && fn->fd1 <= max_zsh_fd) { if (fn->fd1 >= 10 && - fdtable[fn->fd1] == FDT_INTERNAL) + (fdtable[fn->fd1] & FDT_TYPE_MASK) == + FDT_INTERNAL) bad = 3; } } @@ -4270,7 +4282,7 @@ closem(int how) for (i = 10; i <= max_zsh_fd; i++) if (fdtable[i] != FDT_UNUSED && - (how == FDT_UNUSED || fdtable[i] == how)) { + (how == FDT_UNUSED || (fdtable[i] & FDT_TYPE_MASK) == how)) { if (i == SHTTY) SHTTY = -1; zclose(i); diff --git a/Src/zsh.h b/Src/zsh.h index ccd11db..91e8d7f 100644 --- a/Src/zsh.h +++ b/Src/zsh.h @@ -456,6 +456,18 @@ enum { #define FDT_PROC_SUBST 7 #endif +/* + * Mask to get the basic FDT type. + */ +#define FDT_TYPE_MASK 15 + +/* + * Bit flag that fd is saved for later restoration. + * Currently this is only use with FDT_INTERNAL. We use this fact so as + * not to have to mask checks against other types. + */ +#define FDT_SAVED_MASK 16 + /* Flags for input stack */ #define INP_FREE (1<<0) /* current buffer can be free'd */ #define INP_ALIAS (1<<1) /* expanding alias or history */ ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-09-29 15:18 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-13 16:12 fd used for saving redirected fds leaked to child processes Stephane Chazelas 2017-08-13 18:49 ` Bart Schaefer 2017-08-13 18:49 ` Peter Stephenson 2017-08-13 21:45 ` Bart Schaefer 2017-08-14 9:09 ` Peter Stephenson 2017-08-14 14:49 ` Bart Schaefer 2017-08-14 15:24 ` Peter Stephenson 2017-08-14 15:31 ` Bart Schaefer 2017-08-14 19:19 ` Peter Stephenson 2017-09-29 15:06 ` Daniel Shahaf 2017-09-29 15:18 ` Peter Stephenson 2017-08-15 18:42 ` Peter Stephenson
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).