27284: better use of movefd()

7 files changed, 66 insertions, 18 deletions

diff --git a/ChangeLog b/ChangeLog
index 9e9e6d89a..944a5ab86 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2009-09-22  Peter Stephenson  <pws@csr.com>
+
+	* 27284: Src/exec.c, Src/parse.c, Src/utils.c,
+	Src/Modules/socket.c, Src/Modules/tcp.c, Src/Modules/zpty.c:
+	improve use of movefd() and restore closing of original fd
+	on failure pending further work.
+
 2009-09-21  Peter Stephenson  <p.w.stephenson@ntlworld.com>
 
 	* 27283: Src/exec.c, Src/utils.c: failure to dup fd accessed
@@ -12202,5 +12209,5 @@
 
 *****************************************************
 * This is used by the shell to define $ZSH_PATCHLEVEL
-* $Revision: 1.4785 $
+* $Revision: 1.4786 $
 *****************************************************
diff --git a/Src/Modules/socket.c b/Src/Modules/socket.c
index 469568a11..3f47636bc 100644
--- a/Src/Modules/socket.c
+++ b/Src/Modules/socket.c
@@ -120,13 +120,19 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	if (targetfd) {
-	    redup(sfd, targetfd);
-	    sfd = targetfd;
+	    if (redup(sfd, targetfd) == -1)
+		sfd = -1;
+	    else
+		sfd = targetfd;
 	}
 	else {
 	    /* move the fd since no one will want to read from it */
 	    sfd = movefd(sfd);
 	}
+	if (sfd == -1) {
+	    zerrnam(nam, "cannot duplicate fd %d: %e", sfd, errno);
+	    return 1;
+	}
 
 	setiparam("REPLY", sfd);
 
diff --git a/Src/Modules/tcp.c b/Src/Modules/tcp.c
index 2b9e9df18..2825cb978 100644
--- a/Src/Modules/tcp.c
+++ b/Src/Modules/tcp.c
@@ -446,14 +446,22 @@ bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	if (targetfd) {
-	    redup(sess->fd,targetfd);
-	    sess->fd = targetfd;
+	    if (redup(sess->fd,targetfd) == -1)
+		sess->fd = -1;
+	    else
+		sess->fd = targetfd;
 	}
 	else {
 	    /* move the fd since no one will want to read from it */
 	    sess->fd = movefd(sess->fd);
 	}
 
+	if (sess->fd == -1) {
+	    zwarnnam(nam, "cannot duplicate fd %d: %e", sess->fd, errno);
+	    tcp_close(sess);
+	    return 1;
+	}
+
 	setiparam("REPLY", sess->fd);
 
 	if (verbose)
diff --git a/Src/Modules/zpty.c b/Src/Modules/zpty.c
index 4899f8e2e..16bec2bc9 100644
--- a/Src/Modules/zpty.c
+++ b/Src/Modules/zpty.c
@@ -401,6 +401,12 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 	zexit(lastval, 0);
     }
     master = movefd(master);
+    if (master == -1) {
+	zerrnam(nam, "cannot duplicate fd %d: %e", master, errno);
+	scriptname = oscriptname;
+	ineval = oineval;
+	return 1;
+    }
 
     p = (Ptycmd) zalloc(sizeof(*p));
 
@@ -423,6 +429,7 @@ newptycmd(char *nam, char *pname, char **args, int echo, int nblock)
 
     scriptname = oscriptname;
     ineval = oineval;
+
     return 0;
 }
 
diff --git a/Src/exec.c b/Src/exec.c
index ca9cf0f4d..2263bd640 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1958,14 +1958,19 @@ addfd(int forked, int *save, struct multio **mfds, int fd1, int fd2, int rflag,
     if (varid) {
 	/* fd will be over 10, don't touch mfds */
 	fd1 = movefd(fd2);
-	fdtable[fd1] = FDT_EXTERNAL;
-	setiparam(varid, (zlong)fd1);
-	/*
-	 * If setting the parameter failed, close the fd else
-	 * it will leak.
-	 */
-	if (errflag)
-	    zclose(fd1);
+	if (fd1 == -1) {
+	    zerr("cannot moved fd %d: %e", fd2, errno);
+	    return;
+	} else {
+	    fdtable[fd1] = FDT_EXTERNAL;
+	    setiparam(varid, (zlong)fd1);
+	    /*
+	     * If setting the parameter failed, close the fd else
+	     * it will leak.
+	     */
+	    if (errflag)
+		zclose(fd1);
+	}
     } else if (!mfds[fd1] || unset(MULTIOS)) {
 	if(!mfds[fd1]) {		/* starting a new multio */
 	    mfds[fd1] = (struct multio *) zhalloc(sizeof(struct multio));
diff --git a/Src/parse.c b/Src/parse.c
index 105312f6e..7d736e4d3 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -3095,6 +3095,8 @@ load_dump_file(char *dump, struct stat *sbuf, int other, int len)
 	return;
 
     fd = movefd(fd);
+    if (fd == -1)
+	return;
 
     if ((addr = (Wordcode) mmap(NULL, mlen, PROT_READ, MAP_SHARED, fd, off)) ==
 	((Wordcode) -1)) {
diff --git a/Src/utils.c b/Src/utils.c
index a12914787..21a7b43f6 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1631,8 +1631,13 @@ movefd(int fd)
 #else
 	int fe = movefd(dup(fd));
 #endif
-	if (fe != -1)
-	    zclose(fd);
+	/*
+	 * To close or not to close if fe is -1?
+	 * If it is -1, we haven't moved the fd, so if we close
+	 * it we lose it; but we're probably not going to be able
+	 * to use it in situ anyway.  So probably better to avoid a leak.
+	 */
+	zclose(fd);
 	fd = fe;
     }
     if(fd != -1) {
@@ -1647,22 +1652,30 @@ movefd(int fd)
     return fd;
 }
 
-/* Move fd x to y.  If x == -1, fd y is closed. */
+/*
+ * Move fd x to y.  If x == -1, fd y is closed.
+ * Return 0 for success, -1 for failure.
+ */
 
 /**/
-mod_export void
+mod_export int
 redup(int x, int y)
 {
+    int ret = 0;
+
     if(x < 0)
 	zclose(y);
     else if (x != y) {
 	while (y >= fdtable_size)
 	    fdtable = zrealloc(fdtable, (fdtable_size *= 2)*sizeof(*fdtable));
-	dup2(x, y);
+	if (dup2(x, y) == -1)
+	    ret = -1;
 	if ((fdtable[y] = fdtable[x]) && y > max_zsh_fd)
 	    max_zsh_fd = y;
 	zclose(x);
     }
+
+    return ret;
 }
 
 /* Close the given fd, and clear it from fdtable. */