54986: actually prevent FDT_MODULE fds from being closed by user

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.

3 files changed, 46 insertions, 27 deletions

diff --git a/Src/Modules/socket.c b/Src/Modules/socket.c
index 3ecab42c9..95235453e 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 4366f3c02..36c3dca37 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 738b22b43..b631c57cf 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;
 }