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

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Tue, 2019-07-02 at 13:20 +0100, Stephane Chazelas wrote:
> 2019-07-02 09:59:36 +0100, Peter Stephenson:
> Note that ksh93 marks the fds above 2 that are open with
> exec (exec 7< file or exec {fd}< file) with close-on-exec
> (independently of whether the exec is done in background or
> not)

This will do that for file descriptors managed with varid or builtins from
modules, though it doesn't change the behaviour for file descriptors
managed directly by number.  I'm wondering if that's too traditional to
be worth any pain of changing --- {fd} is a much more manageable
interface.

pws

diff --git a/Doc/Zsh/mod_socket.yo b/Doc/Zsh/mod_socket.yo
index 78d9254e8..5b4355158 100644
--- a/Doc/Zsh/mod_socket.yo
+++ b/Doc/Zsh/mod_socket.yo
@@ -57,8 +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 file descriptor remains open in subshells
-and forked external executables.
+the inbound connection.  The file descriptor remains open in subshells,
+but is marked as "close on execute" on systems where this is possible.
 
 If tt(-d) is specified, its argument
 will be taken as the target file descriptor for the
diff --git a/Doc/Zsh/mod_tcp.yo b/Doc/Zsh/mod_tcp.yo
index bf17ec82a..441504a4c 100644
--- a/Doc/Zsh/mod_tcp.yo
+++ b/Doc/Zsh/mod_tcp.yo
@@ -25,6 +25,8 @@ item(File descriptor)(
 The file descriptor in use for the connection.  For normal inbound (tt(I))
 and outbound (tt(O)) connections this may be read and written by the usual
 shell mechanisms.  However, it should only be close with `tt(ztcp -c)'.
+All file descriptors opened by the tt(zsh/net/tcp) module
+are marked as "close on execute" on systems where this is possible.
 )
 item(Connection type)(
 A letter indicating how the session was created:
diff --git a/Doc/Zsh/redirect.yo b/Doc/Zsh/redirect.yo
index 13496d8d3..9b6b78efa 100644
--- a/Doc/Zsh/redirect.yo
+++ b/Doc/Zsh/redirect.yo
@@ -182,8 +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 file descriptor remains open in subshells
-and forked external executables.
+the syntax tt(>&$myfd).  The file descriptor remains open in subshells,
+but is marked as "close on execute" on systems where this is possible.
 
 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/socket.c b/Src/Modules/socket.c
index c65b7dfce..6083ee85b 100644
--- a/Src/Modules/socket.c
+++ b/Src/Modules/socket.c
@@ -129,8 +129,8 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	    return 1;
 	}
 
-	/* allow to be closed explicitly */
-	fdtable[sfd] = FDT_EXTERNAL;
+	/* allow to be closed explicitly, close on exec */
+	markfd(sfd, FDT_EXTERNAL, true);
 
 	setiparam_no_convert("REPLY", (zlong)sfd);
 
@@ -214,7 +214,7 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 		zclose(rfd);
 		return 1;
 	    }
-	    fdtable[sfd] = FDT_EXTERNAL;
+	    markfd(sfd, FDT_EXTERNAL, true);
 	}
 	else {
 	    sfd = rfd;
@@ -258,7 +258,7 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 		    return 1;
 		}
 		sfd = targetfd;
-		fdtable[sfd] = FDT_EXTERNAL;
+		markfd(sfd, FDT_EXTERNAL, true);
 	    }
 
 	    setiparam_no_convert("REPLY", (zlong)sfd);
diff --git a/Src/Modules/system.c b/Src/Modules/system.c
index 50de59cf9..d3b47429a 100644
--- a/Src/Modules/system.c
+++ b/Src/Modules/system.c
@@ -315,9 +315,7 @@ bin_sysopen(char *nam, char **args, Options ops, UNUSED(int func))
     char *opt, *ptr, *nextopt, *fdvar;
     int o, fd, moved_fd, explicit = -1;
     mode_t perms = 0666;
-#if defined(FD_CLOEXEC) && !defined(O_CLOEXEC)
-    int fdflags = 0;
-#endif
+    bool close_on_exec = 0;
 
     if (!OPT_ISSET(ops, 'u')) {
 	zwarnnam(nam, "file descriptor not specified");
@@ -349,7 +347,7 @@ bin_sysopen(char *nam, char **args, Options ops, UNUSED(int func))
 	    }
 #if defined(FD_CLOEXEC) && !defined(O_CLOEXEC)
 	    if (!openopts[o].oflag)
-		fdflags = FD_CLOEXEC;
+		close_on_exec = 1;
 #endif
 	    flags |= openopts[o].oflag;
 	    opt = nextopt;
@@ -382,8 +380,7 @@ bin_sysopen(char *nam, char **args, Options ops, UNUSED(int func))
 	return 1;
     }
 
-#ifdef FD_CLOEXEC
-#ifdef O_CLOEXEC
+#if defined(FD_CLOEXEC) && defined(O_CLOEXEC)
     /*
      * the O_CLOEXEC is a flag attached to the *file descriptor*, not the
      * *open file description* so it doesn't survive a dup(). If that flag was
@@ -391,12 +388,9 @@ bin_sysopen(char *nam, char **args, Options ops, UNUSED(int func))
      * even if the original one was open with O_CLOEXEC
      */
     if ((flags & O_CLOEXEC) && fd != moved_fd)
-#else
-    if (fdflags)
-#endif /* O_CLOEXEC */
-	fcntl(moved_fd, F_SETFD, FD_CLOEXEC);
-#endif /* FD_CLOEXEC */
-    fdtable[moved_fd] = FDT_EXTERNAL;
+	close_on_exec = 1;
+#endif /* defined(FD_CLOEXEC) && defined(O_CLOEXEC) */
+    markfd(moved_fd, FDT_EXTERNAL, close_on_exec);
     if (explicit == -1) {
 	setiparam(fdvar, moved_fd);
 	/* if setting the variable failed, close moved_fd to avoid leak */
diff --git a/Src/exec.c b/Src/exec.c
index 2acb2c0bc..3ab538908 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2300,7 +2300,8 @@ addfd(int forked, int *save, struct multio **mfds, int fd1, int fd2, int rflag,
 	    zerr("cannot moved fd %d: %e", fd2, errno);
 	    return;
 	} else {
-	    fdtable[fd1] = FDT_EXTERNAL;
+	    /* Mark for close-on-exec, consistent with ksh93 */
+	    markfd(fd1, FDT_EXTERNAL, true);
 	    setiparam(varid, (zlong)fd1);
 	    /*
 	     * If setting the parameter failed, close the fd else
diff --git a/Src/utils.c b/Src/utils.c
index 46cf7bcf6..f4440fc70 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2078,8 +2078,30 @@ redup(int x, int y)
     return ret;
 }
 
+
 /*
- * Add an fd opened ithin a module.
+ * Mark an fd as a particular type fdt: see addmodulefd() for details.
+ *
+ * Handle any additional business, e.g. marking fd's for close on exec
+ * which is normal behaviour for externally visible fds unless the
+ * caller wishes for special arrangements.
+ */
+
+/**/
+mod_export void
+markfd(int fd, int fdt, bool close_on_exec)
+{
+    fdtable[fd] = fdt;
+#ifdef FD_CLOEXEC
+    if (close_on_exec)
+	fcntl(fd, F_SETFD, FD_CLOEXEC);
+#else
+    (void)close_on_exec;
+#endif
+}
+
+/*
+ * Add an fd opened within a module.
  *
  * fdt is the type of the fd; see the FDT_ definitions in zsh.h.
  * The most likely falures are:
@@ -2096,6 +2118,9 @@ redup(int x, int y)
  * exec's or performs an exec with a fork optimised out.
  *
  * Safe if fd is -1 to indicate failure.
+ *
+ * It is assumed file descriptors from modules should be set to
+ * close-on-exec.
  */
 /**/
 mod_export void
@@ -2103,7 +2128,7 @@ addmodulefd(int fd, int fdt)
 {
     if (fd >= 0) {
 	check_fd_table(fd);
-	fdtable[fd] = fdt;
+	markfd(fd, fdt, true);
     }
 }