ztcp should not pick fd 0

ztcp should not pick fd 0

[Greg Klanderman]

I'm using ztcp to connect to a server; when the connection is lost and
needs to be re-opened from within the completion system (compctl),
calling 'ztcp <host> <port>' will actually choose fd 0 for the socket
file descriptor which is hosing other parts of my program (calling out
to a python process specifically).  I can work around this problem by
requesting the previous fd with the '-d' option (I suppose that's not
100% safe because after closing the original socket that fd might get
reallocated in the intervening time), but it would be really nice if
ztcp could be made to never choose fds 0 through 2.  This is also a
problem because if you try to close fd 0 with ztcp you'll get the
error '0 is an invalid argument to -c'.

The obvious patch below does not work because movefd() marks the fd as
FDT_INTERNAL, and that causes the fd to get closed when external
programs are exec'd.  This calls into question the other use of movefd
in tcp.c as well.

Suggestions welcome..

Greg


Index: Src/Modules/tcp.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/tcp.c,v
retrieving revision 1.51
diff -u -r1.51 tcp.c
--- Src/Modules/tcp.c	22 Sep 2009 16:04:16 -0000	1.51
+++ Src/Modules/tcp.c	11 Aug 2010 15:34:42 -0000
@@ -668,6 +668,16 @@
 		    return 1;
 		}
 	    }
+            else {
+                /* if fd is < 10, move it up to be >= 10 */
+                int oldfd = sess->fd;
+                sess->fd = movefd(sess->fd);
+		if (sess->fd < 0) {
+		    zerrnam(nam, "could not move socket fd %d: %e", oldfd, errno);
+		    return 1;
+		}
+            }
+
 
 	    setiparam("REPLY", sess->fd);
 

Re: ztcp should not pick fd 0

[Peter Stephenson]

On Wed, 11 Aug 2010 12:21:27 -0400
Greg Klanderman <gak@klanderman.net> wrote:
> I'm using ztcp to connect to a server; when the connection is lost and
> needs to be re-opened from within the completion system (compctl),
> calling 'ztcp <host> <port>' will actually choose fd 0 for the socket
> file descriptor which is hosing other parts of my program (calling out
> to a python process specifically).  I can work around this problem by
> requesting the previous fd with the '-d' option (I suppose that's not
> 100% safe because after closing the original socket that fd might get
> reallocated in the intervening time), but it would be really nice if
> ztcp could be made to never choose fds 0 through 2.  This is also a
> problem because if you try to close fd 0 with ztcp you'll get the
> error '0 is an invalid argument to -c'.

Moving it above 9 is probably OK.  I'm trying to work out whether there's
any use at present where that might cause problems, but it would have to be
somewhere that assumed it was less than 10, which would be an unsafe
assumption.  It's more consistent to have it over 9, other "magic" file
descriptors work like that.

> The obvious patch below does not work because movefd() marks the fd as
> FDT_INTERNAL, and that causes the fd to get closed when external
> programs are exec'd.  This calls into question the other use of movefd
> in tcp.c as well.

There's nothing very special about fdtable; adding "fdtable[sess->fd] =
FDT_EXTERNAL" to the patch on success should be OK, or invent another FDT_
type (fdtable is already exported to modules).  The other use in the file
may need this too --- it's quite likely the CLOEXEC behaviour of
zsh/net/tcp hasn't been thought through, but the default should obviously
be the system default of not closing fd's.

-- 
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: ztcp should not pick fd 0

[Greg Klanderman]

>>>>> On August 11, 2010 Peter Stephenson <Peter.Stephenson@csr.com> wrote:

> There's nothing very special about fdtable; adding "fdtable[sess->fd] =
> FDT_EXTERNAL" to the patch on success should be OK,

OK, that should be easy but since the fdtable stuff is completely
unabstracted, and since the redup case also needs that, and since
redup only sets max_zsh_fd when the old fdtable entry is nonzero, I
need to either make several more copies of the fdtable resizing code
or actually abstract it.  I should have a patch in a little bit...
but first need lunch.

thanks,
Greg

Re: ztcp should not pick fd 0

[Greg Klanderman]

Peter, how's this look?

greg


Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.236
diff -u -r1.236 utils.c
--- Src/utils.c	16 Dec 2009 18:38:44 -0000	1.236
+++ Src/utils.c	11 Aug 2010 19:46:00 -0000
@@ -1621,6 +1621,34 @@
     }
 }
 
+/* Add an entry for fd to the fdtable with the given value. */
+
+/**/
+mod_export void
+fdtable_add(int fd, int fdtval)
+{
+    if (fd != -1 && fdtval != FDT_UNUSED) {
+	if (fd > max_zsh_fd) {
+	    while (fd >= fdtable_size)
+		fdtable = zrealloc(fdtable,
+				   (fdtable_size *= 2)*sizeof(*fdtable));
+	    max_zsh_fd = fd;
+	}
+	fdtable[fd] = fdtval;
+    }
+}
+
+/* Clear the entry for fd in the fdtable. */
+
+/**/
+mod_export void
+fdtable_clear(int fd)
+{
+    fdtable[fd] = FDT_UNUSED;
+    while (max_zsh_fd > 0 && fdtable[max_zsh_fd] == FDT_UNUSED)
+        max_zsh_fd--;
+}
+
 /* Move a fd to a place >= 10 and mark the new fd in fdtable.  If the fd *
  * is already >= 10, it is not moved.  If it is invalid, -1 is returned. */
 
@@ -1643,15 +1671,12 @@
 	zclose(fd);
 	fd = fe;
     }
-    if(fd != -1) {
-	if (fd > max_zsh_fd) {
-	    while (fd >= fdtable_size)
-		fdtable = zrealloc(fdtable,
-				   (fdtable_size *= 2)*sizeof(*fdtable));
-	    max_zsh_fd = fd;
-	}
-	fdtable[fd] = FDT_INTERNAL;
-    }
+
+    /* FIXME: setting the entry to FDT_INTERNAL is pretty bogus; it would be
+     * better to copy the value from the old fd (as redup below does), but
+     * changing that would require auditing all uses of this function. */
+    fdtable_add(fd, FDT_INTERNAL);
+
     return fd;
 }
 
@@ -1669,12 +1694,9 @@
     if(x < 0)
 	zclose(y);
     else if (x != y) {
-	while (y >= fdtable_size)
-	    fdtable = zrealloc(fdtable, (fdtable_size *= 2)*sizeof(*fdtable));
 	if (dup2(x, y) == -1)
 	    ret = -1;
-	if ((fdtable[y] = fdtable[x]) && y > max_zsh_fd)
-	    max_zsh_fd = y;
+        fdtable_add(y, fdtable[x]);
 	zclose(x);
     }
 
@@ -1688,9 +1710,7 @@
 zclose(int fd)
 {
     if (fd >= 0) {
-	fdtable[fd] = FDT_UNUSED;
-	while (max_zsh_fd > 0 && fdtable[max_zsh_fd] == FDT_UNUSED)
-	    max_zsh_fd--;
+        fdtable_clear(fd);
 	if (fd == coprocin)
 	    coprocin = -1;
 	if (fd == coprocout)
Index: Src/Modules/tcp.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/Modules/tcp.c,v
retrieving revision 1.51
diff -u -r1.51 tcp.c
--- Src/Modules/tcp.c	22 Sep 2009 16:04:16 -0000	1.51
+++ Src/Modules/tcp.c	11 Aug 2010 19:46:00 -0000
@@ -236,6 +236,7 @@
     if (!sess) return NULL;
 
     sess->fd = socket(domain, type, protocol);
+    fdtable_add(sess->fd, FDT_EXTERNAL);
     return sess;
 }
 
@@ -298,6 +299,7 @@
     {  
 	if (sess->fd != -1)
 	{
+            fdtable_clear(sess->fd);
 	    err = close(sess->fd);
 	    if (err)
 		zwarn("connection close failed: %e", errno);
@@ -337,6 +339,29 @@
 }
 
 static int
+maybe_move_fd(char *nam, Tcp_session sess, int targetfd)
+{
+    if (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;
+        }
+    }
+    else {
+        /* if necessary, move the fd to one >= 10 */
+        int oldfd = sess->fd;
+        sess->fd = movefd(sess->fd);
+        fdtable_add(sess->fd, FDT_EXTERNAL); /* FIXME: see comment in movefd() */
+        if (sess->fd < 0) {
+            zerrnam(nam, "could not move socket fd %d: %e", oldfd, errno);
+            return 1;
+        }
+    }
+    return 0;
+}
+
+static int
 bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 {
     int herrno, err=1, destport, force=0, verbose=0, test=0, targetfd=0;
@@ -445,19 +470,8 @@
 	    return 1;
 	}
 
-	if (targetfd) {
-	    sess->fd = redup(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;
-	}
+        if (maybe_move_fd(nam, sess, targetfd))
+            return 1;
 
 	setiparam("REPLY", sess->fd);
 
@@ -543,16 +557,11 @@
 	    return 1;
 	}
 
-	if (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;
-	}
+        sess->fd = rfd;
+        fdtable_add(rfd, FDT_EXTERNAL);
+
+        if (maybe_move_fd(nam, sess, targetfd))
+            return 1;
 
 	setiparam("REPLY", sess->fd);
 
@@ -659,22 +668,14 @@
 	    zsfree(desthost);
 	    return 1;
 	}
-	else
-	{
-	    if (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);
+        if (maybe_move_fd(nam, sess, targetfd))
+            return 1;
 
-	    if (verbose)
-		printf("%s:%d is now on fd %d\n",
-			desthost, destport, sess->fd);
-	}
+        setiparam("REPLY", sess->fd);
+
+        if (verbose)
+            printf("%s:%d is now on fd %d\n", desthost, destport, sess->fd);
 	
 	zsfree(desthost);
     }

Re: ztcp should not pick fd 0

[Greg Klanderman]

> Peter, how's this look?

Oops apparently I haven't updated from CVS in a while at work and I
see you already made some changes back here which conflict with mine:

revision 1.238
date: 2010/02/22 10:12:31;  author: pws;  state: Exp;  lines: +27 -11
27721: rationalise initialisation of file descriptors

I'll send an updated patch..

Greg