Any comment on file descriptor behavior in functions?

Any comment on file descriptor behavior in functions?

[Bart Schaefer]

Specifically zsh-workers/20991:

> It could be argued that descriptors opened by process substitution
> should remain open across external command execution within functions;
> they do, in e.g. bash.

(But don't in zsh, in case that isn't clear.)

Re: Any comment on file descriptor behavior in functions?

[Peter Stephenson]

> Specifically zsh-workers/20991:
> 
> > It could be argued that descriptors opened by process substitution
> > should remain open across external command execution within functions;
> > they do, in e.g. bash.
> 
> (But don't in zsh, in case that isn't clear.)

Yes, I suppose they should be kept open, since the difference between
the command and function examples in that is rather nasty.  I presume
this can be done with an extra flag.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

**********************************************************************

Re: Any comment on file descriptor behavior in functions?

[Peter Stephenson]

Bart Schaefer wrote:
> Specifically zsh-workers/20991:
> 
> > It could be argued that descriptors opened by process substitution
> > should remain open across external command execution within functions;
> > they do, in e.g. bash.
> 
> (But don't in zsh, in case that isn't clear.)

Is this it?  Does it have other effects?

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.84
diff -u -r1.84 exec.c
--- Src/exec.c	6 Feb 2005 20:36:42 -0000	1.84
+++ Src/exec.c	22 Mar 2005 13:55:17 -0000
@@ -496,7 +496,13 @@
     }
 
     argv = makecline(args);
-    closem(3);
+    /*
+     * At this point we used to:
+     *   closem(3);
+     * which had the effect of closing file descriptors opened
+     * for process substitution.  However, experience suggests
+     * it's better to leave these open for other processes to use.
+     */
     child_unblock();
     if ((int) strlen(arg0) >= PATH_MAX) {
 	zerr("command too long: %s", arg0, 0);

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

**********************************************************************

Re: Any comment on file descriptor behavior in functions?

[Bart Schaefer]

On Mar 22,  2:04pm, Peter Stephenson wrote:
} Subject: Re: Any comment on file descriptor behavior in functions?
}
} > > It could be argued that descriptors opened by process substitution
} > > should remain open across external command execution within functions
} 
} Is this it?  Does it have other effects?

That's not it, or rather it's too much of it.  That also leaves open the
xtrace output descriptor, etc.  The fix needs to be more specific.

Re: Any comment on file descriptor behavior in functions?

[Peter Stephenson]

Bart Schaefer wrote:
> On Mar 22,  2:04pm, Peter Stephenson wrote:
> } Subject: Re: Any comment on file descriptor behavior in functions?
> }
> } > > It could be argued that descriptors opened by process substitution
> } > > should remain open across external command execution within functions
> } 
> } Is this it?  Does it have other effects?
> 
> That's not it, or rather it's too much of it.  That also leaves open the
> xtrace output descriptor, etc.  The fix needs to be more specific.

(I can't see any evidence for ", etc.", but I presume you just didn't
perform an exhaustive search.)

Maybe making the problem less obscure makes the solution more likely to
work.  The following doesn't look obviously broken to me, although I'm
not completely certain what was going on with the relative values
assigned to the xtrace output and the process substitutions.

As far as I can see, when entering a function the values for process
subsitutions would be incremented so that both types would be closed for
an external program, but on a nested function they'd both be incremented
further so they wouldn't.  That does seem to be what was happening:

% fn() { fn2 $1; }
% fn2() { cat $1; }
% fn <(echo 1)
1
% fn2 <(echo 1)
cat: /proc/self/fd/12: No such file or directory

I can't believe that was intended (though it gives a nasty workaround
for older shells).

Both should now work like the "fn" example, and the xtrace behaviour
should have become disengaged from the process substitution behaviour.
In other words, I'm assuming that anything that made an xtrace entry in
fdtable other than 3 wasn't doing so usefully.  The "#ifdef
PATH_DEV_FD"s suggest as much.

I will be less communicative until Thursday next week.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.84
diff -u -r1.84 exec.c
--- Src/exec.c	6 Feb 2005 20:36:42 -0000	1.84
+++ Src/exec.c	22 Mar 2005 18:15:25 -0000
@@ -77,7 +77,7 @@
  * by zclose.                                                      */
 
 /**/
-char *fdtable;
+unsigned char *fdtable;
 
 /* The allocated size of fdtable */
 
@@ -496,7 +496,11 @@
     }
 
     argv = makecline(args);
-    closem(3);
+    /*
+     * Note that we don't close fd's attached to process substitution
+     * here, which should be visible to external processes.
+     */
+    closem(FDT_XTRACE);
     child_unblock();
     if ((int) strlen(arg0) >= PATH_MAX) {
 	zerr("command too long: %s", arg0, 0);
@@ -1053,7 +1057,7 @@
 	mpipe(opipe);
 	coprocin = ipipe[0];
 	coprocout = opipe[1];
-	fdtable[coprocin] = fdtable[coprocout] = 0;
+	fdtable[coprocin] = fdtable[coprocout] = FDT_UNUSED;
     }
     /* This used to set list_pipe_pid=0 unconditionally, but in things
      * like `ls|if true; then sleep 20; cat; fi' where the sleep was
@@ -1576,7 +1580,7 @@
 	    mfds[fd1]->fds[mfds[fd1]->ct++] = movefd(fd2);
 	}
     }
-    if (subsh_close >= 0 && !fdtable[subsh_close])
+    if (subsh_close >= 0 && fdtable[subsh_close] == FDT_UNUSED)
 	subsh_close = -1;
 }
 
@@ -2132,7 +2136,7 @@
 	    read(synch[0], &dummy, 1);
 	    close(synch[0]);
 #ifdef PATH_DEV_FD
-	    closem(2);
+	    closem(FDT_PROC_SUBST);
 #endif
 	    if (how & Z_ASYNC) {
 		lastpid = (zlong) pid;
@@ -2196,7 +2200,7 @@
 	if (!(xtrerr = fdopen(movefd(dup(fileno(stderr))), "w")))
 	    xtrerr = stderr;
 	else
-	    fdtable[fileno(xtrerr)] = 3;
+	    fdtable[fileno(xtrerr)] = FDT_XTRACE;
     }
 
     /* Add pipeline input/output to mnodes */
@@ -2286,7 +2290,7 @@
 		if (fn->fd2 < 10)
 		    closemn(mfds, fn->fd2);
 		if (fn->fd2 > 9 &&
-		    (fdtable[fn->fd2] ||
+		    (fdtable[fn->fd2] != FDT_UNUSED ||
 		     fn->fd2 == coprocin ||
 		     fn->fd2 == coprocout)) {
 		    fil = -1;
@@ -2414,7 +2418,7 @@
 		int i;
 
 		for (i = 10; i <= max_zsh_fd; i++)
-		    if (fdtable[i] > 1)
+		    if (fdtable[i] >= FDT_PROC_SUBST)
 			fdtable[i]++;
 #endif
 		if (subsh_close >= 0)
@@ -2424,17 +2428,17 @@
 		execshfunc((Shfunc) hn, args);
 #ifdef PATH_DEV_FD
 		for (i = 10; i <= max_zsh_fd; i++)
-		    if (fdtable[i] > 1)
-			if (--(fdtable[i]) <= 2)
+		    if (fdtable[i] >= FDT_PROC_SUBST)
+			if (--(fdtable[i]) <= FDT_PROC_SUBST)
 			    zclose(i);
 #endif
 	    } else {
 		/* It's a builtin */
 		if (forked)
-		    closem(1);
+		    closem(FDT_INTERNAL);
 		lastval = execbuiltin(args, (Builtin) hn);
 #ifdef PATH_DEV_FD
-		closem(2);
+		closem(FDT_PROC_SUBST);
 #endif
 		fflush(stdout);
 		if (save[1] == -2) {
@@ -2478,7 +2482,7 @@
 		    if (errflag)
 			_exit(1);
 		}
-		closem(1);
+		closem(FDT_INTERNAL);
 		if (coprocin)
 		    zclose(coprocin);
 		if (coprocout)
@@ -2711,7 +2715,12 @@
     forklevel = locallevel;
 }
 
-/* close internal shell fds */
+/*
+ * Close internal shell fds.
+ *
+ * Close any that are marked as used if "how" is FDT_UNUSED, else
+ * close any with the value "how".
+ */
 
 /**/
 mod_export void
@@ -2720,7 +2729,8 @@
     int i;
 
     for (i = 10; i <= max_zsh_fd; i++)
-	if (fdtable[i] && (!how || fdtable[i] == how))
+	if (fdtable[i] != FDT_UNUSED &&
+	    (how == FDT_UNUSED || fdtable[i] == how))
 	    zclose(i);
 }
 
@@ -2865,7 +2875,7 @@
 
 	zclose(pipes[1]);
 	retval = readoutput(pipes[0], qt);
-	fdtable[pipes[0]] = 0;
+	fdtable[pipes[0]] = FDT_UNUSED;
 	waitforpid(pid);		/* unblocks */
 	lastval = cmdoutval;
 	return retval;
@@ -3069,7 +3079,7 @@
 	    addproc(pid, NULL, 1, &bgtime);
 	return pnam;
     }
-    closem(0);
+    closem(FDT_UNUSED);
     fd = open(pnam, out ? O_WRONLY | O_NOCTTY : O_RDONLY | O_NOCTTY);
     if (fd == -1) {
 	zerr("can't open %s: %e", pnam, errno);
@@ -3094,7 +3104,7 @@
 	    zclose(pipes[!out]);
 	    return NULL;
 	}
-	fdtable[pipes[!out]] = 2;
+	fdtable[pipes[!out]] = FDT_PROC_SUBST;
 	if (!out)
 	{
 	    addproc(pid, NULL, 1, &bgtime);
@@ -3103,7 +3113,7 @@
     }
     entersubsh(Z_ASYNC, 1, 0, 0);
     redup(pipes[out], out);
-    closem(0);   /* this closes pipes[!out] as well */
+    closem(FDT_UNUSED);   /* this closes pipes[!out] as well */
 #endif /* PATH_DEV_FD */
 
     cmdpush(CS_CMDSUBST);
@@ -3147,7 +3157,7 @@
     }
     entersubsh(Z_ASYNC, 1, 0, 0);
     redup(pipes[out], out);
-    closem(0);	/* this closes pipes[!out] as well */
+    closem(FDT_UNUSED);	/* this closes pipes[!out] as well */
     cmdpush(CS_CMDSUBST);
     execode(prog, 0, 1);
     cmdpop();
Index: Src/init.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/init.c,v
retrieving revision 1.48
diff -u -r1.48 init.c
--- Src/init.c	7 Feb 2005 15:53:23 -0000	1.48
+++ Src/init.c	22 Mar 2005 18:15:25 -0000
@@ -1048,7 +1048,7 @@
 	freeeprog(prog);
     else {
 	fclose(bshin);
-	fdtable[SHIN] = 0;
+	fdtable[SHIN] = FDT_UNUSED;
 	SHIN = fd;		     /* the shell input fd                   */
 	bshin = obshin;		     /* file handle for buffered shell input */
     }
@@ -1252,7 +1252,7 @@
     } while (zsh_name);
 
     fdtable_size = zopenmax();
-    fdtable = zshcalloc(fdtable_size);
+    fdtable = zshcalloc(fdtable_size*sizeof(*fdtable));
 
     createoptiontable();
     emulate(zsh_name, 1);   /* initialises most options */
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.77
diff -u -r1.77 utils.c
--- Src/utils.c	11 Mar 2005 15:22:36 -0000	1.77
+++ Src/utils.c	22 Mar 2005 18:15:25 -0000
@@ -1078,10 +1078,11 @@
     if(fd != -1) {
 	if (fd > max_zsh_fd) {
 	    while (fd >= fdtable_size)
-		fdtable = zrealloc(fdtable, (fdtable_size *= 2));
+		fdtable = zrealloc(fdtable,
+				   (fdtable_size *= 2)*sizeof(*fdtable));
 	    max_zsh_fd = fd;
 	}
-	fdtable[fd] = 1;
+	fdtable[fd] = FDT_INTERNAL;
     }
     return fd;
 }
@@ -1096,7 +1097,7 @@
 	zclose(y);
     else if (x != y) {
 	while (y >= fdtable_size)
-	    fdtable = zrealloc(fdtable, (fdtable_size *= 2));
+	    fdtable = zrealloc(fdtable, (fdtable_size *= 2)*sizeof(*fdtable));
 	dup2(x, y);
 	if ((fdtable[y] = fdtable[x]) && y > max_zsh_fd)
 	    max_zsh_fd = y;
@@ -1111,8 +1112,8 @@
 zclose(int fd)
 {
     if (fd >= 0) {
-	fdtable[fd] = 0;
-	while (max_zsh_fd > 0 && !fdtable[max_zsh_fd])
+	fdtable[fd] = FDT_UNUSED;
+	while (max_zsh_fd > 0 && fdtable[max_zsh_fd] == FDT_UNUSED)
 	    max_zsh_fd--;
 	if (fd == coprocin)
 	    coprocin = -1;
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.71
diff -u -r1.71 zsh.h
--- Src/zsh.h	18 Mar 2005 22:40:23 -0000	1.71
+++ Src/zsh.h	22 Mar 2005 18:15:26 -0000
@@ -254,6 +254,32 @@
 #define IS_READFD(X)          (((X)>=REDIR_READWRITE && (X)<=REDIR_MERGEIN) || (X)==REDIR_INPIPE)
 #define IS_REDIROP(X)         ((X)>=OUTANG && (X)<=TRINANG)
 
+/*
+ * Values for the fdtable array.  They say under what circumstances
+ * the fd will be close.  The fdtable is an unsigned char, so these are
+ * #define's rather than an enum.
+ */
+/* Entry not used. */
+#define FDT_UNUSED		0
+/*
+ * Entry used internally by the shell, should not be visible to other
+ * processes.
+ */
+#define FDT_INTERNAL		1
+/*
+ * Entry used by output from the XTRACE option.
+ */
+#define FDT_XTRACE		2
+#ifdef PATH_DEV_FD
+/*
+ * Entry used by a process substition.
+ * The value will be incremented on entering a function and
+ * decremented on exit; we don't close entries greater than
+ * FDT_PROC_SUBST except when closing everything.
+ */
+#define FDT_PROC_SUBST		3
+#endif
+
 /* Flags for input stack */
 #define INP_FREE      (1<<0)	/* current buffer can be free'd            */
 #define INP_ALIAS     (1<<1)	/* expanding alias or history              */

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

**********************************************************************

Re: Any comment on file descriptor behavior in functions?

[Bart Schaefer]

On Mar 22,  5:11pm, Bart Schaefer wrote:
}
} That's not it, or rather it's too much of it.  That also leaves open the
} xtrace output descriptor, etc.  The fix needs to be more specific.

I think it's as easy as this -- but I may have missed something, too:

Index: Src/exec.c
===================================================================
RCS file: /extra/cvsroot/zsh/zsh-4.0/Src/exec.c,v
retrieving revision 1.24
diff -c -r1.24 exec.c
--- Src/exec.c	18 Feb 2005 17:05:15 -0000	1.24
+++ Src/exec.c	22 Mar 2005 18:27:34 -0000
@@ -2415,7 +2415,7 @@
 
 		for (i = 10; i <= max_zsh_fd; i++)
 		    if (fdtable[i] > 1)
-			fdtable[i]++;
+			fdtable[i] += 2;
 #endif
 		if (subsh_close >= 0)
 		    zclose(subsh_close);

Re: Any comment on file descriptor behavior in functions?

[Bart Schaefer]

Finally got a chance to apply this patch and try it.  Looks good.

On Mar 22,  6:31pm, Peter Stephenson wrote:
}
} Bart Schaefer wrote:
} > That's not it, or rather it's too much of it.  That also leaves open the
} > xtrace output descriptor, etc.  The fix needs to be more specific.
} 
} (I can't see any evidence for ", etc.", but I presume you just didn't
} perform an exhaustive search.)

Correct; I'm sorry.

} As far as I can see, when entering a function the values for process
} subsitutions would be incremented so that both types would be closed for
} an external program, but on a nested function they'd both be incremented
} further so they wouldn't.

Yes, I concur.  What I can't figure out is why they need to be incremented
at all?  Why not just assign them the do-not-close value to begin with?
Something having to do with them persisting for only one command?

Appended is a suggested test entry.  While writing this, I noticed that
the first process substitution test relies on the 'cut' and 'paste'
external commands.  I guess there's never been a problem with that, but
I wasn't aware they were both supported on all platforms where zsh has
been compiled.

Index: Test/D03procsubst.ztst
===================================================================
retrieving revision 1.3
diff -c -r1.3 D03procsubst.ztst
--- Test/D03procsubst.ztst	23 Sep 2003 15:27:17 -0000	1.3
+++ Test/D03procsubst.ztst	26 Mar 2005 17:02:51 -0000
@@ -12,6 +12,8 @@
     true
   fi
 
+  function copycat { cat "$@" }
+
 %test
   paste <(cut -f1 FILE1) <(cut -f3 FILE2)
 0:<(...) substitution
@@ -29,3 +31,8 @@
 >< First	Second	Third	Fourth
 >---
 >> Erste	Zweite	Dritte	Vierte
+
+  copycat <(print First) <(print Zweite)
+0:FDs remain open for external commands called from functions
+>First
+>Zweite

Re: Any comment on file descriptor behavior in functions?

[Peter Stephenson]

Bart Schaefer wrote:
> } As far as I can see, when entering a function the values for process
> } subsitutions would be incremented so that both types would be closed for
> } an external program, but on a nested function they'd both be incremented
> } further so they wouldn't.
> 
> Yes, I concur.  What I can't figure out is why they need to be incremented
> at all?  Why not just assign them the do-not-close value to begin with?
> Something having to do with them persisting for only one command?

I think it's to do with the way they're closed.  There's no way of
knowing whether they're attached to the command currently passing
through execcmd() or to some shell structure that's calling a command at
a lower level, so there's the risk of a file descriptor being closed too
early.

A more rational way to do this might be to have a list of file
descriptors to close in the job structure, like the existing "filelist"
of temporary files to be deleted.

I'll commit my patch.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

**********************************************************************