Re: latest from CVS segfaults when FD ulimit is set too low

[Peter Stephenson <pws@csr.com>]

> Index: Src/utils.c
> ===================================================================
> RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
> retrieving revision 1.229
> diff -u -r1.229 utils.c
> --- Src/utils.c	9 Jul 2009 20:20:53 -0000	1.229
> +++ Src/utils.c	21 Sep 2009 20:41:49 -0000
> @@ -1631,7 +1631,8 @@
>  #else
>  	int fe = movefd(dup(fd));
>  #endif
> -	zclose(fd);
> +	if (fe != -1)
> +	    zclose(fd);
>  	fd = fe;
>      }
>      if(fd != -1) {

Here are some more thoughts on movefd().

I don't think it's safe to leave the unmoved fd open, after all, in too
many places that will leak.  In some places we need the original fd, but in
those places if the move fails the code will fail catastrophically---for
example in zle and completion we attempt to move fd 0 temporarily and then
move it back; if that failed we shouldn't attempt the operation.  I haven't
done that much surgery, so I've restored the original zclose() here for now.

I've found a few places that could handle a failed fd move better.  I've
tried to make the error handling match what's in the function already (if
any: load_dump_file() appears not to have any), but it's tricky to get
exactly right.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.171
diff -u -r1.171 exec.c
--- Src/exec.c	21 Sep 2009 20:49:30 -0000	1.171
+++ Src/exec.c	22 Sep 2009 08:54:47 -0000
@@ -1958,14 +1958,19 @@
     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));
Index: Src/parse.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/parse.c,v
retrieving revision 1.81
diff -u -r1.81 parse.c
--- Src/parse.c	17 Jul 2009 20:32:34 -0000	1.81
+++ Src/parse.c	22 Sep 2009 08:54:47 -0000
@@ -3095,6 +3095,8 @@
 	return;
 
     fd = movefd(fd);
+    if (fd == -1)
+	return;
 
     if ((addr = (Wordcode) mmap(NULL, mlen, PROT_READ, MAP_SHARED, fd, off)) ==
 	((Wordcode) -1)) {
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.230
diff -u -r1.230 utils.c
--- Src/utils.c	21 Sep 2009 20:49:30 -0000	1.230
+++ Src/utils.c	22 Sep 2009 08:54:47 -0000
@@ -1631,8 +1631,13 @@
 #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 @@
     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. */
Index: Src/Modules/socket.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/socket.c,v
retrieving revision 1.11
diff -u -r1.11 socket.c
--- Src/Modules/socket.c	6 Jul 2007 21:52:40 -0000	1.11
+++ Src/Modules/socket.c	22 Sep 2009 08:54:47 -0000
@@ -120,13 +120,19 @@
 	}
 
 	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);
 
Index: Src/Modules/tcp.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/tcp.c,v
retrieving revision 1.49
diff -u -r1.49 tcp.c
--- Src/Modules/tcp.c	6 Nov 2008 01:35:12 -0000	1.49
+++ Src/Modules/tcp.c	22 Sep 2009 08:54:47 -0000
@@ -446,14 +446,22 @@
 	}
 
 	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)
Index: Src/Modules/zpty.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/zpty.c,v
retrieving revision 1.41
diff -u -r1.41 zpty.c
--- Src/Modules/zpty.c	13 Jan 2009 12:09:26 -0000	1.41
+++ Src/Modules/zpty.c	22 Sep 2009 08:54:47 -0000
@@ -401,6 +401,12 @@
 	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 @@
 
     scriptname = oscriptname;
     ineval = oineval;
+
     return 0;
 }
 
-- 
Peter Stephenson <pws@csr.com>            Software Engineer
Tel: +44 (0)1223 692070                   Cambridge Silicon Radio Limited
Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, UK


Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom