latest from CVS segfaults when FD ulimit is set too low

latest from CVS segfaults when FD ulimit is set too low

[Jim Meyering]

Hello,

I was surprised to see zsh segfault.
I confirmed that it happens also with the latest from CVS.
Here's a nearly minimal reproducer (same with ulimit -n 12):

  $ printf '_src_etc_profile_d() { f=; }\n_src_etc_profile_d\n' > k
  $ Src/zsh -f -c '( . ./k; ulimit -n 7 && exec 0</dev/null )'
  zsh:1: cannot duplicate fd 0: invalid argument
  zsh: segmentation fault  Src/zsh -f ...

Note that this is not contrived.
It is derived from real code in coreutils' test suite that
exercises GNU sort, attempting to verify that it performs
even under extreme FD pressure.  Because zsh treats the first
10 FDs specially, that makes that particular test fail, so now
we just skip it when zsh happens to be the shell used by the test.

Also, note that the above does not segfault every time.
For me, on Fedora 11 and x86_64, it segfaults approximately 8 in 10 times.

Rerunning under valgrind:

==20742== Memcheck, a memory error detector.
==20742== Copyright (C) 2002-2008, and GNU GPL'd, by Julian Seward et al.
==20742== Using LibVEX rev 1884, a library for dynamic binary translation.
==20742== Copyright (C) 2004-2008, and GNU GPL'd, by OpenWorks LLP.
==20742== Using valgrind-3.4.1, a dynamic binary instrumentation framework.
==20742== Copyright (C) 2000-2008, and GNU GPL'd, by Julian Seward et al.
==20742== For more details, rerun with: -v
==20742==
==20742== Warning: invalid file descriptor 12 in syscall fcntl(DUPFD)()
zsh:1: cannot duplicate fd 0: too many open files
==20742== Invalid read of size 4
==20742==    at 0x42B593: closemnodes (exec.c:1907)
==20742==    by 0x42B7A5: addfd (exec.c:1983)
==20742==    by 0x42E43C: execcmd (exec.c:2872)
==20742==    by 0x42A758: execpline2 (exec.c:1579)
==20742==    by 0x4298EA: execpline (exec.c:1364)
==20742==    by 0x428FA8: execlist (exec.c:1161)
==20742==    by 0x42F39A: execcmd (exec.c:3170)
==20742==    by 0x42A758: execpline2 (exec.c:1579)
==20742==    by 0x4298EA: execpline (exec.c:1364)
==20742==    by 0x428FA8: execlist (exec.c:1161)
==20742==    by 0x4289E3: execode (exec.c:988)
==20742==    by 0x42898A: execstring (exec.c:973)
==20742==  Address 0x4c17000 is not stack'd, malloc'd or (recently) free'd
==20742==
==20742== Process terminating with default action of signal 11 (SIGSEGV)
==20742==  Access not within mapped region at address 0x4C17000
==20742==    at 0x42B593: closemnodes (exec.c:1907)
==20742==    by 0x42B7A5: addfd (exec.c:1983)
==20742==    by 0x42E43C: execcmd (exec.c:2872)
==20742==    by 0x42A758: execpline2 (exec.c:1579)
==20742==    by 0x4298EA: execpline (exec.c:1364)
==20742==    by 0x428FA8: execlist (exec.c:1161)
==20742==    by 0x42F39A: execcmd (exec.c:3170)
==20742==    by 0x42A758: execpline2 (exec.c:1579)
==20742==    by 0x4298EA: execpline (exec.c:1364)
==20742==    by 0x428FA8: execlist (exec.c:1161)
==20742==    by 0x4289E3: execode (exec.c:988)
==20742==    by 0x42898A: execstring (exec.c:973)
==20742==  If you believe this happened as a result of a stack overflow in your
==20742==  program's main thread (unlikely but possible), you can try to increase
==20742==  the size of the main thread stack using the --main-stacksize= flag.
==20742==  The main thread stack size used in this run was 10485760.
==20742==
==20742== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 6 from 2)
==20742== malloc/free: in use at exit: 48,565 bytes in 925 blocks.
==20742== malloc/free: 1,128 allocs, 203 frees, 89,327 bytes allocated.
==20742== For counts of detected errors, rerun with: -v
==20742== searching for pointers to 925 not-freed blocks.
==20742== checked 276,856 bytes.
==20742==
==20742== LEAK SUMMARY:
==20742==    definitely lost: 0 bytes in 0 blocks.
==20742==      possibly lost: 0 bytes in 0 blocks.
==20742==    still reachable: 48,565 bytes in 925 blocks.
==20742==         suppressed: 0 bytes in 0 blocks.
==20742== Rerun with --leak-check=full to see details of leaked memory.

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

[Peter Stephenson]

On Fri, 18 Sep 2009 11:48:13 +0200
Jim Meyering <jim@meyering.net> wrote:
>   $ printf '_src_etc_profile_d() { f=; }\n_src_etc_profile_d\n' > k
>   $ Src/zsh -f -c '( . ./k; ulimit -n 7 && exec 0</dev/null )'
>   zsh:1: cannot duplicate fd 0: invalid argument
>   zsh: segmentation fault  Src/zsh -f ...

This fixes the segmentation fault:  it was intermittent because we were
accessing uninitialised memory, so this is a fully paid up bug
regardless of file descriptor limitations.

I haven't looked further: this sort of stuff is certainly not well
tested and there could well be more nasties.  If you can provide any
further code to exercise it, splendid.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.170
diff -u -r1.170 exec.c
--- Src/exec.c	12 Jul 2009 15:10:07 -0000	1.170
+++ Src/exec.c	21 Sep 2009 20:41:49 -0000
@@ -1980,6 +1980,7 @@
 		     */
 		    if (fdN < 0 && errno != EBADF) {
 			zerr("cannot duplicate fd %d: %e", fd1, errno);
+			mfds[fd1] = NULL;
 			closemnodes(mfds);
 			return;
 		    }
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) {

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

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

[Peter Stephenson]

> 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

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

[Bart Schaefer]

On Sep 22, 10:00am, Peter Stephenson wrote:
} Subject: Re: latest from CVS segfaults when FD ulimit is set too low
}
} 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.

Perhaps instead of an unconditional close, movefd() should do

	if (fe != -1 || fd > 2)
	    zclose(fd);

so we never actually lose stdin/out/err but other leaks are plugged.

I haven't studied the ramifications of this.

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

[Wayne Davison]

[-- Attachment #1: Type: text/plain, Size: 212 bytes --]

We can simplify the code a bit if we make redup() return the target fd
on success.  The attached patch also adds a little more error checking
for redup().  I'll check it in later on if nobody objects.

..wayne..

[-- Attachment #2: redup.patch --]
[-- Type: text/x-diff, Size: 2603 bytes --]

index 3f47636..ad7eb58 100644
--- a/Src/Modules/socket.c
+++ b/Src/Modules/socket.c
@@ -120,10 +120,7 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	if (targetfd) {
-	    if (redup(sfd, targetfd) == -1)
-		sfd = -1;
-	    else
-		sfd = targetfd;
+	    sfd = redup(sfd, targetfd);
 	}
 	else {
 	    /* move the fd since no one will want to read from it */
@@ -205,8 +202,11 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	if (targetfd) {
-	    redup(rfd, targetfd);
-	    sfd = targetfd;
+	    sfd = redup(rfd, targetfd);
+	    if (sfd < 0) {
+		zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
+		return 1;
+	    }
 	}
 	else {
 	    sfd = rfd;
@@ -242,8 +242,11 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	else
 	{
 	    if (targetfd) {
-		redup(sfd, targetfd);
-		sfd = targetfd;
+		sfd = redup(sfd, targetfd);
+		if (sfd < 0) {
+		    zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
+		    return 1;
+		}
 	    }
 
 	    setiparam("REPLY", sfd);
index 2825cb9..3f92050 100644
--- a/Src/Modules/tcp.c
+++ b/Src/Modules/tcp.c
@@ -446,10 +446,7 @@ bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	if (targetfd) {
-	    if (redup(sess->fd,targetfd) == -1)
-		sess->fd = -1;
-	    else
-		sess->fd = targetfd;
+	    sess->fd = redup(sess->fd, targetfd);
 	}
 	else {
 	    /* move the fd since no one will want to read from it */
@@ -547,8 +544,11 @@ bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	if (targetfd) {
-	    redup(rfd, targetfd);
-	    sess->fd = targetfd;
+	    sess->fd = redup(rfd, targetfd);
+	    if (sess->fd < 0) {
+		zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
+		return 1;
+	    }
 	}
 	else {
 	    sess->fd = rfd;
@@ -662,8 +662,11 @@ bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 	else
 	{
 	    if (targetfd) {
-		redup(sess->fd, targetfd);
-		sess->fd = targetfd;
+		sess->fd = redup(sess->fd, targetfd);
+		if (sess->fd < 0) {
+		    zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
+		    return 1;
+		}
 	    }
 
 	    setiparam("REPLY", sess->fd);
index 21a7b43..b807eea 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1654,14 +1654,14 @@ movefd(int fd)
 
 /*
  * Move fd x to y.  If x == -1, fd y is closed.
- * Return 0 for success, -1 for failure.
+ * Returns y for success, -1 for failure.
  */
 
 /**/
 mod_export int
 redup(int x, int y)
 {
-    int ret = 0;
+    int ret = y;
 
     if(x < 0)
 	zclose(y);

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

[Peter Stephenson]

Wayne Davison wrote:
> We can simplify the code a bit if we make redup() return the target fd
> on success.  The attached patch also adds a little more error checking
> for redup().  I'll check it in later on if nobody objects.

Looks OK.

-- 
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

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

[Peter Stephenson]

On Tue, 22 Sep 2009 06:40:07 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Perhaps instead of an unconditional close, movefd() should do
> 
> 	if (fe != -1 || fd > 2)
> 	    zclose(fd);
> 
> so we never actually lose stdin/out/err but other leaks are plugged.

I think that does make sense, in the context of calls that aren't fully
error-checked.  The worst that's likely to happen is that if a user
deliberately closed 0, 1, or 2 we'll get an unwanted fd there.  In
return 0, 1, or 2 won't be accidentally closed.

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/