Re: Mangement of fdtable[]

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

On Thu, 15 Oct 2015 17:22:52 -0700
Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> I was puzzling over Yen Chi Hsuan's bug report in 36866 so was looking
> through tcp.c and noticed that it opens file descriptors with socket()
> without marking them used in fdtable[].  The only time they're handled
> "properly" is with "ztcp -l" which makes a movefd() call.

Is this the fix for tcp.c (not socket.c)?  I couldn't see any obvious
missing places since there appears to be strictly one fd per session so
there is only a single case for closing, and the only non-standard point
at which an fd is created is with listen(), giving two cases for opening.

I added a different fd type because although the sockets can be used
directly by users (as with FDT_EXTERNAL), they shouldn't be closed by
the normal explicit shell syntax because that leaves other stuff
dangling.  I didn't think FDT_INTERNAL was the right thing, on balance,
as they're too public for the shell taking over management to be the
right thing to do.  For simiilar reasons, I don't think CLOEXEC
behaviour is wanted --- the user gets to decide when and where the fd is
usable.

pws

diff --git a/Src/Modules/tcp.c b/Src/Modules/tcp.c index
bc1765d..8e8ed92 100644 --- a/Src/Modules/tcp.c +++ b/Src/Modules/tcp.c
@@ -236,6 +236,8 @@ tcp_socket(int domain, int type, int protocol, int
ztflags) if (!sess) return NULL;
 
     sess->fd = socket(domain, type, protocol);
+    /* We'll check failure and tidy up in caller */
+    addmodulefd(sess->fd);
     return sess;
 }
 
@@ -298,7 +300,7 @@ tcp_close(Tcp_session sess)
     {  
 	if (sess->fd != -1)
 	{
-	    err = close(sess->fd);
+	    err = zclose(sess->fd);
 	    if (err)
 		zwarn("connection close failed: %e", errno);
 	}
@@ -546,6 +548,9 @@ bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 	    return 1;
 	}
 
+	/* redup expects fd is already registered */
+	addmodulefd(rfd);
+
 	if (targetfd) {
 	    sess->fd = redup(rfd, targetfd);
 	    if (sess->fd < 0) {
diff --git a/Src/utils.c b/Src/utils.c
index 61cbe84..b4de6c1 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1892,6 +1892,25 @@ redup(int x, int y)
 }
 
 /*
+ * Add an fd opened by external means within a module.
+ * Although the fd can be used within the shell for normal I/O, it can
+ * only be closed by the module (which should included zclose() as part
+ * of the sequence).
+ * Safe if fd is -1 to indicate failure.
+ */
+/**/
+mod_export void
+addmodulefd(int fd)
+{
+    if (fd >= 0) {
+	check_fd_table(fd);
+	fdtable[fd] = FDT_MODULE;
+    }
+}
+
+/**/
+
+/*
  * Indicate that an fd has a file lock; if cloexec is 1 it will be closed
  * on exec.
  * The fd should already be known to fdtable (e.g. by movefd).
diff --git a/Src/zsh.h b/Src/zsh.h
index 15fa5e4..f819249 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -406,25 +406,32 @@ enum {
  */
 #define FDT_EXTERNAL		2
 /*
+ * 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
+/*
  * Entry used by output from the XTRACE option.
  */
-#define FDT_XTRACE		3
+#define FDT_XTRACE		4
 /*
  * Entry used for file locking.
  */
-#define FDT_FLOCK		4
+#define FDT_FLOCK		5
 /*
  * As above, but the fd is not marked for closing on exec,
  * so the shell can still exec the last process.
  */
-#define FDT_FLOCK_EXEC		5
+#define FDT_FLOCK_EXEC		6
 #ifdef PATH_DEV_FD
 /*
  * Entry used by a process substition.
  * This marker is not tested internally as we associated the file
  * descriptor with a job for closing.
  */
-#define FDT_PROC_SUBST		6
+#define FDT_PROC_SUBST		7
 #endif
 
 /* Flags for input stack */