fd used for saving redirected fds leaked to child processes

fd used for saving redirected fds leaked to child processes

[Stephane Chazelas]

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

Re: fd used for saving redirected fds leaked to child processes

[Bart Schaefer]

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.

Re: fd used for saving redirected fds leaked to child processes

[Peter Stephenson]

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;
 }
 

Re: fd used for saving redirected fds leaked to child processes

[Bart Schaefer]

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 &

Re: fd used for saving redirected fds leaked to child processes

[Peter Stephenson]

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

Re: fd used for saving redirected fds leaked to child processes

[Bart Schaefer]

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?

Re: fd used for saving redirected fds leaked to child processes

[Peter Stephenson]

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

Re: fd used for saving redirected fds leaked to child processes

[Bart Schaefer]

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.

Re: fd used for saving redirected fds leaked to child processes

[Peter Stephenson]

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

Re: fd used for saving redirected fds leaked to child processes

[Daniel Shahaf]

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

Re: fd used for saving redirected fds leaked to child processes

[Peter Stephenson]

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;
 		}
 	    }
 	}

Re: fd used for saving redirected fds leaked to child processes

[Peter Stephenson]

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