PATCH: Actually prevent FDT_MODULE fds from being closed by user

[Mikael Magnusson <mikachu@xxxxxxxxx>]

The stated goal of FDT_MODULE is:
  /*
   * Entry visible to other processes but controlled by a module.
   * The difference from FDT_EXTERNAL is that closing this using
   * standard fd syntax will fail as there is some tidying up that
   * needs to be done by the module's own mechanism.
   */
  #define FDT_MODULE		3

but this wasn't actually enforced. By accident, some fds ended up as
FDT_INTERNAL because movefd clobbers the FDT_MODULE value and those were
protected from closing. I think this straightens out all the bits
properly, at least the ones I found.

(Note that zsocket is using FDT_EXTERNAL purposefully because the user
*is* expected to close those fds with the {myfd}>&- syntax, it has no
specific interface to close opened sockets since it has no internal
state for them.)

Both zsocket and ztcp forgot to movefd the some of their fds which meant
it was potentially in the 0-9 range, which is also not protected from
being closed by the user, but:

Since the -d flag allows the user to put module fds at low fds, I made
the check forbid closing FDT_MODULE fds in that range too, I think
that's correct?

zftp did some very questionable things, relying on FDT_UNUSED fds to act
approximately like it expected, instead use FDT_MODULE there too.

Also make redup ensure the fdtable is big enough for the assignments
after, just for consistency, since the old addmodulefd call before
didn't necessarily grow it to the moved-to fd number.

Remove some misleading comments like
/* move the fd since no one will want to read from it */
the reason we move the fd is because we have promised the user we will
not open fds in the 0-9 range.

Also added maximum macro, and converted existing users of MIN/MAX to
them, since we have not, as far as I can tell, made any effort to check
that those macros are in fact defined by anyone.
---
 Src/Modules/socket.c | 41 +++++++++++++++++++++++------------------
 Src/Modules/tcp.c    | 25 +++++++++++++++++++------
 Src/Modules/zftp.c   |  7 ++++---
 Src/exec.c           | 14 +++++++++-----
 Src/params.c         |  8 ++++----
 Src/utils.c          |  8 +++++---
 Src/zsh.h            |  1 +
 7 files changed, 65 insertions(+), 39 deletions(-)

diff --git a/Src/Modules/socket.c b/Src/Modules/socket.c
index 3ecab42c9d..95235453e4 100644
--- a/Src/Modules/socket.c
+++ b/Src/Modules/socket.c
@@ -119,17 +119,18 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	    return 1;
 	}
 
-	addmodulefd(sfd, FDT_EXTERNAL);
-
 	if (targetfd) {
 	    sfd = redup(sfd, targetfd);
-	}
-	else {
-	    /* move the fd since no one will want to read from it */
+	    if (sfd == -1) {
+		zerrnam(nam, "cannot duplicate socket fd to %d: %e", targetfd, errno);
+		return 1;
+	    }
+	} else {
+	    targetfd = sfd; /* for error message */
 	    sfd = movefd(sfd);
 	}
 	if (sfd == -1) {
-	    zerrnam(nam, "cannot duplicate fd %d: %e", sfd, errno);
+	    zerrnam(nam, "could not move socket fd %d: %e", targetfd, errno);
 	    return 1;
 	}
 
@@ -209,20 +210,20 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	    return 1;
 	}
 
-	addmodulefd(rfd, FDT_EXTERNAL);
-
 	if (targetfd) {
 	    sfd = redup(rfd, targetfd);
-	    if (sfd < 0) {
+	    if (sfd == -1) {
 		zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
-		zclose(rfd);
 		return 1;
 	    }
-	    fdtable[sfd] = FDT_EXTERNAL;
+	} else {
+	    sfd = movefd(rfd);
 	}
-	else {
-	    sfd = rfd;
+	if (sfd == -1) {
+	    zerrnam(nam, "could not move socket fd %d: %e", rfd, errno);
+	    return 1;
 	}
+	fdtable[sfd] = FDT_EXTERNAL;
 
 	setiparam_no_convert("REPLY", (zlong)sfd);
 
@@ -268,17 +269,21 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 	else
 	{
-	    addmodulefd(sfd, FDT_EXTERNAL);
-
 	    if (targetfd) {
-		if (redup(sfd, targetfd) < 0) {
+		if (redup(sfd, targetfd) == -1) {
 		    zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
-		    zclose(sfd);
 		    return 1;
 		}
 		sfd = targetfd;
-		fdtable[sfd] = FDT_EXTERNAL;
+	    } else {
+		targetfd = sfd; /* for error message */
+		sfd = movefd(sfd);
 	    }
+	    if (sfd == -1) {
+		zerrnam(nam, "could not move socket fd %d: %e", targetfd, errno);
+		return 1;
+	    }
+	    fdtable[sfd] = FDT_EXTERNAL;
 
 	    setiparam_no_convert("REPLY", (zlong)sfd);
 
diff --git a/Src/Modules/tcp.c b/Src/Modules/tcp.c
index 4366f3c026..36c3dca373 100644
--- a/Src/Modules/tcp.c
+++ b/Src/Modules/tcp.c
@@ -489,18 +489,26 @@ bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 
 	if (targetfd) {
 	    sess->fd = redup(sess->fd, targetfd);
+	    if (sess->fd < 0) {
+		zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
+		tcp_close(sess);
+		return 1;
+	    }
 	}
 	else {
-	    /* move the fd since no one will want to read from it */
+	    targetfd = sess->fd; /* just stashing here for error message */
 	    sess->fd = movefd(sess->fd);
 	}
 
 	if (sess->fd == -1) {
-	    zwarnnam(nam, "cannot duplicate fd %d: %e", sess->fd, errno);
+	    zwarnnam(nam, "cannot duplicate fd %d: %e", targetfd, errno);
 	    tcp_close(sess);
 	    return 1;
 	}
 
+	/* disallow to be closed explicitly, after movefd set it to INTERNAL */
+	fdtable[sess->fd] = FDT_MODULE;
+
 	setiparam_no_convert("REPLY", (zlong)sess->fd);
 
 	if (verbose)
@@ -588,19 +596,24 @@ bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 	    return 1;
 	}
 
-	/* redup expects fd is already registered */
-	addmodulefd(rfd, FDT_MODULE);
-
 	if (targetfd) {
 	    sess->fd = redup(rfd, targetfd);
 	    if (sess->fd < 0) {
 		zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
+		tcp_close(sess);
 		return 1;
 	    }
 	}
 	else {
-	    sess->fd = rfd;
+	    sess->fd = movefd(rfd);
+	}
+	if (sess->fd < 0) {
+	    zerrnam(nam, "could not move socket fd %d: %e", rfd, errno);
+	    tcp_close(sess);
+	    return 1;
 	}
+	/* disallow to be closed explicitly */
+	fdtable[sess->fd] = FDT_MODULE;
 
 	setiparam_no_convert("REPLY", (zlong)sess->fd);
 
diff --git a/Src/Modules/zftp.c b/Src/Modules/zftp.c
index 738b22b43e..b631c57cf9 100644
--- a/Src/Modules/zftp.c
+++ b/Src/Modules/zftp.c
@@ -458,7 +458,7 @@ zfunpipe(void)
 }
 
 /*
- * Same as movefd(), but don't mark the fd in the zsh tables,
+ * Same as movefd(), but mark the fd as FDT_MODULE in the zsh tables,
  * because we only want it closed by zftp.  However, we still
  * need to shift the fd's out of the way of the user-visible 0-9.
  */
@@ -473,9 +473,10 @@ zfmovefd(int fd)
 #else
 	int fe = zfmovefd(dup(fd));
 #endif
-	close(fd);
+	zclose(fd);
 	fd = fe;
     }
+    addmodulefd(fd, FDT_MODULE);
     return fd;
 }
 
@@ -1040,7 +1041,7 @@ zfclosedata(void)
 {
     if (zfsess->dfd == -1)
 	return;
-    close(zfsess->dfd);
+    zclose(zfsess->dfd);
     zfsess->dfd = -1;
 }
 
diff --git a/Src/exec.c b/Src/exec.c
index 20caf40d1e..5f849db63b 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3885,10 +3885,12 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 				    bad = 1;
 			    }
 			    if (!bad && fn->fd1 <= max_zsh_fd) {
-				if (fn->fd1 >= 10 &&
-				    (fdtable[fn->fd1] & FDT_TYPE_MASK) ==
-				    FDT_INTERNAL)
-				    bad = 3;
+				int fdt = fdtable[fn->fd1] & FDT_TYPE_MASK;
+				if ((fn->fd1 >= 10 && fdt == FDT_INTERNAL) ||
+				    fdt == FDT_MODULE)
+				{
+				    bad = 3 + (fdt == FDT_MODULE);
+				}
 			    }
 			}
 		    }
@@ -3896,7 +3898,8 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			const char *bad_msg[] = {
 			    "parameter %s does not contain a file descriptor",
 			    "can't close file descriptor from readonly parameter %s",
-			    "file descriptor %d used by shell, not closed"
+			    "file descriptor %d used by shell, not closed",
+			    "file descriptor %d used by module, not closed",
 			};
 			if (bad > 2)
 			    zwarn(bad_msg[bad-1], fn->fd1);
@@ -3947,6 +3950,7 @@ execcmd_exec(Estate state, Execcmd_params eparams,
 			  */
 			 (fn->fd2 <= max_zsh_fd &&
 			  ((fdtable[fn->fd2] != FDT_UNUSED &&
+			    fdtable[fn->fd2] != FDT_MODULE &&
 			    fdtable[fn->fd2] != FDT_EXTERNAL) ||
 			   fn->fd2 == coprocin ||
 			   fn->fd2 == coprocout))) {
diff --git a/Src/params.c b/Src/params.c
index e111b28c7b..47cf4c8c75 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2943,7 +2943,7 @@ setarrvalue(Value v, char **val)
 	    v->end = oldlen;
 
 	/* Strings before slice + strings from val + strings after slice */
-	newlen = v->start + vallen + MAX(0, oldlen - v->end);
+	newlen = v->start + vallen + maximum(0, oldlen - v->end);
 
 	if (v->pm->gsu.a->setfn == arrsetfn && old == v->pm->u.arr) {
 	    v->pm->u.arr = NULL; /* Steal the old array */
@@ -2963,7 +2963,7 @@ setarrvalue(Value v, char **val)
 			sizeof(char *) * (oldlen - v->end));
 	} else {
 	    new = (char **) zalloc(sizeof(char *) * (newlen + 1));
-	    for (p = new, q = old, i = MIN(v->start, oldlen); i > 0; i--)
+	    for (p = new, q = old, i = minimum(v->start, oldlen); i > 0; i--)
 		*p++ = ztrdup(*q++);
 	    if (v->end < oldlen)
 		for (p = new + v->start + vallen, q = old + v->end; *q;)
@@ -6350,7 +6350,7 @@ resolve_nameref_rec(Param pm, const Param stop, int keep_lastref)
 	int ppar = zstrtol(refname, NULL, 10);
 	if (ppar >= argnparams_size) {
 	    size_t old_size = argnparams_size;
-	    size_t new_size = argnparams_size = MAX(2 * old_size, ppar);
+	    size_t new_size = argnparams_size = maximum(2 * old_size, ppar);
 	    argnparams = zrealloc(argnparams, new_size * sizeof(Param));
 	    memset(argnparams + old_size, 0,
 		   (new_size - old_size) * sizeof(Param));
@@ -6442,7 +6442,7 @@ setscope_base(Param pm, int base)
 	LinkList refs;
 	if (base >= scoperefs_num) {
 	    int old_num = scoperefs_num;
-	    int new_num = scoperefs_num = MAX(2 * base, 8);
+	    int new_num = scoperefs_num = maximum(2 * base, 8);
 	    scoperefs = zrealloc(scoperefs, new_num * sizeof(refs));
 	    memset(scoperefs + old_num, 0, (new_num - old_num) * sizeof(refs));
 	}
diff --git a/Src/utils.c b/Src/utils.c
index 1130415db3..372d2a8283 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2050,7 +2050,7 @@ redup(int x, int y)
 	if (dup2(x, y) == -1) {
 	    ret = -1;
 	} else {
-	    check_fd_table(y);
+	    check_fd_table(maximum(x, y));
 	    fdtable[y] = fdtable[x];
 	    if (fdtable[y] == FDT_FLOCK || fdtable[y] == FDT_FLOCK_EXEC)
 		fdtable[y] = FDT_INTERNAL;
@@ -2059,16 +2059,18 @@ redup(int x, int y)
 	 * Closing any fd to the locked file releases the lock.
 	 * This isn't expected to happen, it's here for completeness.
 	 */
+	check_fd_table(x);
 	if (fdtable[x] == FDT_FLOCK)
 	    fdtable_flocks--;
 	zclose(x);
-    }
+    } else
+	check_fd_table(x);
 
     return ret;
 }
 
 /*
- * Add an fd opened ithin a module.
+ * Add an fd opened within a module.
  *
  * fdt is the type of the fd; see the FDT_ definitions in zsh.h.
  * The most likely failures are:
diff --git a/Src/zsh.h b/Src/zsh.h
index 1f1ecc3e6e..8a26041961 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -29,6 +29,7 @@
 
 /* A few typical macros */
 #define minimum(a,b)  ((a) < (b) ? (a) : (b))
+#define maximum(a,b)  ((a) > (b) ? (a) : (b))
 
 /*
  * Our longest integer type:  will be a 64 bit either if long already is,
-- 
2.38.1