Re: <(...), >(...) and fds above 9

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Mon, 2019-07-01 at 17:52 +0100, Peter Stephenson wrote:
> {fd}-opened file descriptors are marked FDT_EXTERNAL.  We could default
> to leaving those open on fork and document this.  This would then
> apply to file descriptors created with zsocket, which I presume you
> could think of as being in the same category.
> 
> sysopen file descriptors are marked FDT_INTERNAL, which we'd certainly
> expect to close.  It looks like we make an exception if you use an
> explicit file descriptor number rather than ask for one to be assigned
> to a variable, I think because that's constrained to be 0 to 9.  So these
> could be changed to FDT_EXTERNAL, too, which would probably be more
> consistent.
> 
> As I said, this is just based on needs and documentation rather than any
> definite logic.
> 
> Probably =(...) should use closem() consisent with <(...), too.

This fixes this up --- I think for file descriptors known to and hence
managed by the user the behaviour of silently closing is probably
uncontroversially unhelpful.  Historically, it's really just there to
ensure the shell doesn't leak its own bilge, so shouldn't really be
visible to the user as an effect.  I've explicitly noted the
file descriptors aren't closed (except with close-on-exec, obviously).

One thing I haven't changed is module-owned file descriptors ---
FDT_MODULE.  So currently, for example, file descriptors owned by
zsh/net/tcp are closed when exec'ing or doing substitutions involving
forks.  The rationale for these file descriptors is a little different
--- they're visible to the user, but they're managed by the module
rather than closed by the user.  So not sure whether to change this.
If we don't it should at least be documented.

pws

diff --git a/Doc/Zsh/mod_socket.yo b/Doc/Zsh/mod_socket.yo
index 867f6081f..78d9254e8 100644
--- a/Doc/Zsh/mod_socket.yo
+++ b/Doc/Zsh/mod_socket.yo
@@ -43,7 +43,8 @@ startitem()
 item(tt(zsocket) tt(-l) [ tt(-v) ] [ tt(-d) var(fd) ] var(filename))(
 tt(zsocket -l) will open a socket listening on var(filename).
 The shell parameter tt(REPLY) will be set to the file descriptor
-associated with that listener.
+associated with that listener.  The file descriptor remains open in subshells
+and forked external executables.
 
 If tt(-d) is specified, its argument
 will be taken as the target file descriptor for
@@ -56,7 +57,8 @@ tt(zsocket -a) will accept an incoming connection
 to the socket associated with var(listenfd).
 The shell parameter tt(REPLY) will
 be set to the file descriptor associated with
-the inbound connection.
+the inbound connection.  The file descriptor remains open in subshells
+and forked external executables.
 
 If tt(-d) is specified, its argument
 will be taken as the target file descriptor for the
diff --git a/Doc/Zsh/mod_system.yo b/Doc/Zsh/mod_system.yo
index 3a85e760f..6292af071 100644
--- a/Doc/Zsh/mod_system.yo
+++ b/Doc/Zsh/mod_system.yo
@@ -45,7 +45,9 @@ specified as a comma-separated list. The following is a list of possible
 options. Note that, depending on the system, some may not be available.
 startitem()
 item(tt(cloexec))(
-mark file to be closed when other programs are executed
+mark file to be closed when other programs are executed (else
+the file descriptor remains open in subshells and forked external
+executables)
 )
 xitem(tt(create))
 item(tt(creat))(
diff --git a/Doc/Zsh/redirect.yo b/Doc/Zsh/redirect.yo
index 7e38cd0c3..13496d8d3 100644
--- a/Doc/Zsh/redirect.yo
+++ b/Doc/Zsh/redirect.yo
@@ -182,7 +182,8 @@ indent(... tt({myfd}>&1))
 This opens a new file descriptor that is a duplicate of file descriptor
 1 and sets the parameter tt(myfd) to the number of the file descriptor,
 which will be at least 10.  The new file descriptor can be written to using
-the syntax tt(>&$myfd).
+the syntax tt(>&$myfd).  The file descriptor remains open in subshells
+and forked external executables.
 
 The syntax tt({)var(varid)tt(}>&-), for example tt({myfd}>&-), may be used
 to close a file descriptor opened in this fashion.  Note that the
diff --git a/Src/Modules/system.c b/Src/Modules/system.c
index 7a4f4ee13..50de59cf9 100644
--- a/Src/Modules/system.c
+++ b/Src/Modules/system.c
@@ -316,7 +316,7 @@ bin_sysopen(char *nam, char **args, Options ops, UNUSED(int func))
     int o, fd, moved_fd, explicit = -1;
     mode_t perms = 0666;
 #if defined(FD_CLOEXEC) && !defined(O_CLOEXEC)
-    int fdflags;
+    int fdflags = 0;
 #endif
 
     if (!OPT_ISSET(ops, 'u')) {
@@ -396,8 +396,8 @@ bin_sysopen(char *nam, char **args, Options ops, UNUSED(int func))
 #endif /* O_CLOEXEC */
 	fcntl(moved_fd, F_SETFD, FD_CLOEXEC);
 #endif /* FD_CLOEXEC */
+    fdtable[moved_fd] = FDT_EXTERNAL;
     if (explicit == -1) {
-	fdtable[moved_fd] = FDT_EXTERNAL;
 	setiparam(fdvar, moved_fd);
 	/* if setting the variable failed, close moved_fd to avoid leak */
 	if (errflag)
diff --git a/Src/exec.c b/Src/exec.c
index 60ab0acf8..2acb2c0bc 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4407,8 +4407,10 @@ closem(int how, int all)
 	    /*
 	     * Process substitution needs to be visible to user;
 	     * fd's are explicitly cleaned up by filelist handling.
+	     * External FDs are managed directly by the user.
 	     */
-	    (all || fdtable[i] != FDT_PROC_SUBST) &&
+	    (all || (fdtable[i] != FDT_PROC_SUBST &&
+		     fdtable[i] != FDT_EXTERNAL)) &&
 	    (how == FDT_UNUSED || (fdtable[i] & FDT_TYPE_MASK) == how)) {
 	    if (i == SHTTY)
 		SHTTY = -1;
@@ -4823,6 +4825,7 @@ getoutputfile(char *cmd, char **eptr)
     }
 
     /* pid == 0 */
+    closem(FDT_UNUSED, 0);
     redup(fd, 1);
     entersubsh(ESUB_PGRP|ESUB_NOMONITOR, NULL);
     cmdpush(CS_CMDSUBST);