Mangement of fdtable[]

Mangement of fdtable[]

[Bart Schaefer]

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.

I think this means some fds may be closed in some cases they shouldn't,
or conversely left open in cases they shouldn't.  This may apply to fds
in other modules, e.g., the descriptor from gdbm_open() in db_gdbm.c.

This further led me to notice that when descriptors are manipulated in
utils.c, it carefully calls the static check_fd_table() function every
time to be sure the descriptor has a slot in the table before an FDT_*
value is poked for it.  Other parts of the code (mostly exec.c) simply
reference fdtable[N] without error checking.

I guess this is OK because fdtable[] is allocated zopenmax() slots in
zsh_main(), but it seems inconsistent if not actually wrong.

Comments?

-- 
Barton E. Schaefer

Re: Mangement of fdtable[]

[Chi Hsuan Yen]

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

Dear Bart,

Thanks for digging my problem. In fact my codes uses zsocket. I think
they're similar and they actually cause the same bug. I give a TCP example
because I think it's easier to reproduce on different platforms.

In Src/Modules/socket.c, fdtable is checked but not updated. Maybe it sould
be fixed, too?

Best Regards,

Yen Chi Hsuan

On 16 October 2015 at 08:22, Bart Schaefer <schaefer@brasslantern.com>
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.
>
> I think this means some fds may be closed in some cases they shouldn't,
> or conversely left open in cases they shouldn't.  This may apply to fds
> in other modules, e.g., the descriptor from gdbm_open() in db_gdbm.c.
>
> This further led me to notice that when descriptors are manipulated in
> utils.c, it carefully calls the static check_fd_table() function every
> time to be sure the descriptor has a slot in the table before an FDT_*
> value is poked for it.  Other parts of the code (mostly exec.c) simply
> reference fdtable[N] without error checking.
>
> I guess this is OK because fdtable[] is allocated zopenmax() slots in
> zsh_main(), but it seems inconsistent if not actually wrong.
>
> Comments?
>
> --
> Barton E. Schaefer
>

Re: Mangement of fdtable[]

[Peter Stephenson]

On Thu, 15 Oct 2015 17:22:52 -0700
Bart Schaefer <schaefer@brasslantern.com> 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.
> 
> I think this means some fds may be closed in some cases they shouldn't,
> or conversely left open in cases they shouldn't.  This may apply to fds
> in other modules, e.g., the descriptor from gdbm_open() in db_gdbm.c.

Highly likely; this is probably just wrong.

> This further led me to notice that when descriptors are manipulated in
> utils.c, it carefully calls the static check_fd_table() function every
> time to be sure the descriptor has a slot in the table before an FDT_*
> value is poked for it.  Other parts of the code (mostly exec.c) simply
> reference fdtable[N] without error checking.
> 
> I guess this is OK because fdtable[] is allocated zopenmax() slots in
> zsh_main(), but it seems inconsistent if not actually wrong.

A lot of the fd code in exec.c is actually involved with opening the fd
as a redirection in the first place, so it might actually be correct.

pws

Re: Mangement of fdtable[]

[Peter Stephenson]

On Thu, 15 Oct 2015 17:22:52 -0700
Bart Schaefer <schaefer@brasslantern.com> 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 */

Re: Mangement of fdtable[]

[Bart Schaefer]

On Oct 24,  7:16pm, Peter Stephenson wrote:
} Subject: Re: Mangement of fdtable[]
}
} Is this the fix for tcp.c (not socket.c)?

This looks very similar to what I was contemplating, except that I had
not yet thought as far as adding another FDT_* flag.  The only other
thing I was wondering about is allowing the module to control whether
the FD is closed when entering a subshell [i.e., when exec.c calls
closem(FDT_INTERNAL)], because there is not always an execv() in that
case so simply setting CLOEXEC in the module is not enough.

} I didn't think FDT_INTERNAL was the right thing, on balance,
} as they're too public for the shell taking over management

That's true for tcp.c, but perhaps not so for e.g. db_gdbm.c.

Re: Mangement of fdtable[]

[Peter Stephenson]

On Sat, 24 Oct 2015 11:43:52 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 24,  7:16pm, Peter Stephenson wrote:
> } Subject: Re: Mangement of fdtable[]
> }
> } Is this the fix for tcp.c (not socket.c)?
> 
> This looks very similar to what I was contemplating, except that I had
> not yet thought as far as adding another FDT_* flag.  The only other
> thing I was wondering about is allowing the module to control whether
> the FD is closed when entering a subshell [i.e., when exec.c calls
> closem(FDT_INTERNAL)], because there is not always an execv() in that
> case so simply setting CLOEXEC in the module is not enough.
> 
> } I didn't think FDT_INTERNAL was the right thing, on balance,
> } as they're too public for the shell taking over management
> 
> That's true for tcp.c, but perhaps not so for e.g. db_gdbm.c.

So that means addmodulefd() should have an additional argument saying if
the fd is to be treated as internal.

I don't want to add a different category saying it can be closed on
exec but not by standard shell syntax since I'll get that wrong.

diff --git a/Src/Modules/tcp.c b/Src/Modules/tcp.c
index bc1765d..274f01f 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, FALSE);
     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, FALSE);
+
 	if (targetfd) {
 	    sess->fd = redup(rfd, targetfd);
 	    if (sess->fd < 0) {
diff --git a/Src/utils.c b/Src/utils.c
index 61cbe84..4c69d75 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1892,6 +1892,32 @@ redup(int x, int y)
 }
 
 /*
+ * Add an fd opened ithin a module.
+ *
+ * If is_internal is FALSE, the fd can be used within the shell for
+ * normal I/O but it will not be closed automatically or by normal shell
+ * syntax; it can only be closed by the module (which should included
+ * zclose() as part of the sequence).
+ *
+ * If is_internal is TRUE the fd is treated like others created by the
+ * shell for internall use; it can be closed and will be closed by the
+ * shell if it exec's or performs an exec with a fork optimised out.
+ *.
+ * Safe if fd is -1 to indicate failure.
+ */
+/**/
+mod_export void
+addmodulefd(int fd, bool is_internal)
+{
+    if (fd >= 0) {
+	check_fd_table(fd);
+	fdtable[fd] = is_internal ? FDT_INTERNAL : 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 */

Re: Mangement of fdtable[]

[Bart Schaefer]

On Oct 24,  8:14pm, Peter Stephenson wrote:
} Subject: Re: Mangement of fdtable[]
}
} +mod_export void
} +addmodulefd(int fd, bool is_internal)

My compiler chokes on "bool".

Re: Mangement of fdtable[]

[Peter Stephenson]

On Sat, 24 Oct 2015 20:14:39 +0100
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> So that means addmodulefd() should have an additional argument saying if
> the fd is to be treated as internal.

That's not good enough.  For zsocket, it needs to be an FDT_EXTERNAL
because we want the user to determine whne it gets closed just the same
as with ztcp, but in this case it can only be closed with standard shell
syntax --- I've clarified this.

pws

diff --git a/Doc/Zsh/mod_socket.yo b/Doc/Zsh/mod_socket.yo
index 332c551..867f608 100644
--- a/Doc/Zsh/mod_socket.yo
+++ b/Doc/Zsh/mod_socket.yo
@@ -28,6 +28,11 @@ will be taken as the target file descriptor for the
 connection.
 
 In order to elicit more verbose output, use tt(-v).
+
+File descriptors can be closed with normal shell syntax when no longer
+needed, for example:
+
+example(exec {REPLY}>&-)
 )
 enditem()
 
diff --git a/Src/Modules/socket.c b/Src/Modules/socket.c
index 65b87d7..f683496 100644
--- a/Src/Modules/socket.c
+++ b/Src/Modules/socket.c
@@ -115,6 +115,8 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	    return 1;
 	}
 
+	addmodulefd(sfd, FDT_EXTERNAL);
+
 	if (targetfd) {
 	    sfd = redup(sfd, targetfd);
 	}
@@ -127,6 +129,9 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	    return 1;
 	}
 
+	/* allow to be closed explicitly */
+	fdtable[sfd] = FDT_EXTERNAL;
+
 	setiparam("REPLY", sfd);
 
 	if (verbose)
@@ -200,12 +205,16 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	    return 1;
 	}
 
+	addmodulefd(rfd, FDT_EXTERNAL);
+
 	if (targetfd) {
 	    sfd = redup(rfd, targetfd);
 	    if (sfd < 0) {
 		zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
+		zclose(rfd);
 		return 1;
 	    }
+	    fdtable[sfd] = FDT_EXTERNAL;
 	}
 	else {
 	    sfd = rfd;
@@ -240,12 +249,16 @@ bin_zsocket(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 	else
 	{
+	    addmodulefd(sfd, FDT_EXTERNAL);
+
 	    if (targetfd) {
-		sfd = redup(sfd, targetfd);
-		if (sfd < 0) {
+		if (redup(sfd, targetfd) < 0) {
 		    zerrnam(nam, "could not duplicate socket fd to %d: %e", targetfd, errno);
+		    zclose(sfd);
 		    return 1;
 		}
+		sfd = targetfd;
+		fdtable[sfd] = FDT_EXTERNAL;
 	    }
 
 	    setiparam("REPLY", sfd);
diff --git a/Src/Modules/tcp.c b/Src/Modules/tcp.c
index 274f01f..7b0dcc7 100644
--- a/Src/Modules/tcp.c
+++ b/Src/Modules/tcp.c
@@ -237,7 +237,7 @@ tcp_socket(int domain, int type, int protocol, int ztflags)
 
     sess->fd = socket(domain, type, protocol);
     /* We'll check failure and tidy up in caller */
-    addmodulefd(sess->fd, FALSE);
+    addmodulefd(sess->fd, FDT_MODULE);
     return sess;
 }
 
@@ -549,7 +549,7 @@ bin_ztcp(char *nam, char **args, Options ops, UNUSED(int func))
 	}
 
 	/* redup expects fd is already registered */
-	addmodulefd(rfd, FALSE);
+	addmodulefd(rfd, FDT_MODULE);
 
 	if (targetfd) {
 	    sess->fd = redup(rfd, targetfd);
diff --git a/Src/utils.c b/Src/utils.c
index 4c69d75..37a02c5 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1894,24 +1894,29 @@ redup(int x, int y)
 /*
  * Add an fd opened ithin a module.
  *
- * If is_internal is FALSE, the fd can be used within the shell for
- * normal I/O but it will not be closed automatically or by normal shell
- * syntax; it can only be closed by the module (which should included
- * zclose() as part of the sequence).
+ * fdt is the type of the fd; see the FDT_ definitions in zsh.h.
+ * The most likely falures are:
+ *
+ * FDT_EXTERNAL: the fd can be used within the shell for normal I/O but
+ * it will not be closed automatically or by normal shell syntax.
+ *
+ * FDT_MODULE: as FDT_EXTERNAL, but it can only be closed by the module
+ * (which should included zclose() as part of the sequence, not by
+ * the standard shell syntax for closing file descriptors.
+ *
+ * FDT_INTERNAL: fd is treated like others created by the shell for
+ * internall use; it can be closed and will be closed by the shell if it
+ * exec's or performs an exec with a fork optimised out.
  *
- * If is_internal is TRUE the fd is treated like others created by the
- * shell for internall use; it can be closed and will be closed by the
- * shell if it exec's or performs an exec with a fork optimised out.
- *.
  * Safe if fd is -1 to indicate failure.
  */
 /**/
 mod_export void
-addmodulefd(int fd, bool is_internal)
+addmodulefd(int fd, int fdt)
 {
     if (fd >= 0) {
 	check_fd_table(fd);
-	fdtable[fd] = is_internal ? FDT_INTERNAL : FDT_MODULE;
+	fdtable[fd] = fdt;
     }
 }
 

Re: Mangement of fdtable[]

[Bart Schaefer]

On Oct 24,  8:43pm, Peter Stephenson wrote:
} Subject: Re: Mangement of fdtable[]
}
} That's not good enough.  For zsocket, it needs to be an FDT_EXTERNAL
} because we want the user to determine whne it gets closed just the same
} as with ztcp, but in this case it can only be closed with standard shell
} syntax --- I've clarified this.

OK, so the next question is, if the descriptor is going to be closed by
some external library (again I refer to db_gdbm.c), how should the
module mark it as no longer used in fdtable?  Poke FDT_UNUSED directly?

Re: Mangement of fdtable[]

[Peter Stephenson]

On Sat, 24 Oct 2015 14:05:39 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 24,  8:43pm, Peter Stephenson wrote:
> } Subject: Re: Mangement of fdtable[]
> }
> } That's not good enough.  For zsocket, it needs to be an FDT_EXTERNAL
> } because we want the user to determine whne it gets closed just the same
> } as with ztcp, but in this case it can only be closed with standard shell
> } syntax --- I've clarified this.
> 
> OK, so the next question is, if the descriptor is going to be closed by
> some external library (again I refer to db_gdbm.c), how should the
> module mark it as no longer used in fdtable?  Poke FDT_UNUSED directly?

I guess so, it's just the rest of zclose() apart from the close itself.

I'm wondering if really we should be using bit fields rather than
categories --- so there's a bit for close when we call closem(), a bit
for this is managed by a module, a bit for external, etc.  That probably
scales a lot better than what we've currently got.

pws

Re: Mangement of fdtable[]

[Bart Schaefer]

On Oct 25,  6:44pm, Peter Stephenson wrote:
} Subject: Re: Mangement of fdtable[]
}
} On Sat, 24 Oct 2015 14:05:39 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > OK, so the next question is, if the descriptor is going to be closed by
} > some external library (again I refer to db_gdbm.c), how should the
} > module mark it as no longer used in fdtable?  Poke FDT_UNUSED directly?
} 
} I guess so, it's just the rest of zclose() apart from the close itself.

So this then.


diff --git a/Src/Modules/db_gdbm.c b/Src/Modules/db_gdbm.c
index 76d4751..0329632 100644
--- a/Src/Modules/db_gdbm.c
+++ b/Src/Modules/db_gdbm.c
@@ -106,7 +106,9 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
     }
 
     dbf = gdbm_open(resource_name, 0, read_write, 0666, 0);
-    if(!dbf) {
+    if(dbf)
+	addmodulefd(gdbm_fdesc(dbf), FDT_INTERNAL);
+    else {
 	zwarnnam(nam, "error opening database file %s", resource_name);
 	return 1;
     }
@@ -114,6 +116,7 @@ bin_ztie(char *nam, char **args, Options ops, UNUSED(int func))
     if (!(tied_param = createspecialhash(pmname, &getgdbmnode, &scangdbmkeys,
 					 pmflags))) {
         zwarnnam(nam, "cannot create the requested parameter %s", pmname);
+	fdtable[gdbm_fdesc(dbf)] = FDT_UNUSED;
 	gdbm_close(dbf);
 	return 1;
     }
@@ -319,8 +322,10 @@ gdbmuntie(Param pm)
     GDBM_FILE dbf = (GDBM_FILE)(pm->u.hash->tmpdata);
     HashTable ht = pm->u.hash;
 
-    if (dbf) /* paranoia */
+    if (dbf) { /* paranoia */
+	fdtable[gdbm_fdesc(dbf)] = FDT_UNUSED;
 	gdbm_close(dbf);
+    }
 
     ht->tmpdata = NULL;