Re: Zsh 4.3.12: subshell in midnight commander: precmd: 15: bad file descriptor

[Peter Stephenson <Peter.Stephenson@xxxxxxx>]

On Mon, 18 Jul 2011 09:26:18 -0700
Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> } > *  2978                         else if (fn->fd1 > max_zsh_fd)
> } >    2979                             bad = 3;
> } 
> } Hmm... that's saying we're closing an fd with {foo}>&- syntax that
> } we don't know about in the first place.  Do we just try to close it and
> } report an error closing an unknown fd if that fails?
> 
> Hrm.  Right now it says
> 
> zsh: file descriptor 11 out of range, not closed
> 
> Strictly speaking there should be some way to close an arbitrary file
> descriptor.  If the shell "knows about" all open descriptors that's
> not an issue here ...

The following closes fd's it doesn't know about, but warns if the close
failed --- even if it does know about it, which is a change but seems
reasonable as the close was an explicit user request, particularly since
the failure to close doesn't cause the command to fail.  I haven't
turned this into a hard error, since that would be an incompatibility,
but arguably it should be.

The debug test in zclose() looked really screwy (it would lead to a
memory error as written), and doesn't make sense any more, so I've just
removed it rather than decoding it and fixing it.

Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.199
diff -p -u -r1.199 exec.c
--- Src/exec.c	19 Jul 2011 09:26:57 -0000	1.199
+++ Src/exec.c	19 Jul 2011 09:45:19 -0000
@@ -2975,17 +2975,16 @@ execcmd(Estate state, int input, int out
 			fn->fd1 = (int)getintvalue(v);
 			if (errflag)
 			    bad = 1;
-			else if (fn->fd1 > max_zsh_fd)
-			    bad = 3;
-			else if (fn->fd1 >= 10 &&
+			else if (fn->fd1 <= max_zsh_fd) {
+			    if (fn->fd1 >= 10 &&
 				 fdtable[fn->fd1] == FDT_INTERNAL)
-			    bad = 4;
+				bad = 3;
+			}
 		    }
 		    if (bad) {
 			const char *bad_msg[] = {
 			    "parameter %s does not contain a file descriptor",
 			    "can't close file descriptor from readonly parameter %s",
-			    "file descriptor %d out of range, not closed",
 			    "file descriptor %d used by shell, not closed"
 			};
 			if (bad > 2)
@@ -2995,11 +2994,18 @@ execcmd(Estate state, int input, int out
 			execerr();
 		    }
 		}
+		/*
+		 * Note we may attempt to close an fd beyond max_zsh_fd:
+		 * OK as long as we never look in fdtable for it.
+ 		 */
 		if (!forked && fn->fd1 < 10 && save[fn->fd1] == -2)
 		    save[fn->fd1] = movefd(fn->fd1);
 		if (fn->fd1 < 10)
 		    closemn(mfds, fn->fd1);
-		zclose(fn->fd1);
+		if (zclose(fn->fd1) < 0) {
+		    zwarn("failed to close file descriptor %d: %e",
+			  fn->fd1, errno);
+		}
 		break;
 	    case REDIR_MERGEIN:
 	    case REDIR_MERGEOUT:
Index: Src/utils.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/utils.c,v
retrieving revision 1.259
diff -p -u -r1.259 utils.c
--- Src/utils.c	19 Jun 2011 16:26:11 -0000	1.259
+++ Src/utils.c	19 Jul 2011 09:45:19 -0000
@@ -1802,22 +1802,20 @@ zclose(int fd)
 {
     if (fd >= 0) {
 	/*
-	 * We sometimes zclose() an fd twice where the second
-	 * time is a catch-all in case there was a failure using
-	 * the fd.  This is harmless but we need to trap it
-	 * for the error check here.
+	 * Careful: we allow closing of arbitrary fd's, beyond
+	 * max_zsh_fd.  In that case we don't try anything clever.
 	 */
-	DPUTS2(fd > max_zsh_fd && fdtable[fd] != FDT_UNUSED,
-	       "BUG: fd is %d, max_zsh_fd is %d", fd, max_zsh_fd);
-	if (fdtable[fd] == FDT_FLOCK)
-	    fdtable_flocks--;
-	fdtable[fd] = FDT_UNUSED;
-	while (max_zsh_fd > 0 && fdtable[max_zsh_fd] == FDT_UNUSED)
-	    max_zsh_fd--;
-	if (fd == coprocin)
-	    coprocin = -1;
-	if (fd == coprocout)
-	    coprocout = -1;
+	if (fd <= max_zsh_fd) {
+	    if (fdtable[fd] == FDT_FLOCK)
+		fdtable_flocks--;
+	    fdtable[fd] = FDT_UNUSED;
+	    while (max_zsh_fd > 0 && fdtable[max_zsh_fd] == FDT_UNUSED)
+		max_zsh_fd--;
+	    if (fd == coprocin)
+		coprocin = -1;
+	    if (fd == coprocout)
+		coprocout = -1;
+	}
 	return close(fd);
     }
     return -1;
Index: Test/A04redirect.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A04redirect.ztst,v
retrieving revision 1.18
diff -p -u -r1.18 A04redirect.ztst
--- Test/A04redirect.ztst	6 Mar 2011 21:37:38 -0000	1.18
+++ Test/A04redirect.ztst	19 Jul 2011 09:45:19 -0000
@@ -155,9 +155,12 @@
   (exec 3<&-
   read foo <&-)
 1:'<&-' redirection
+?(eval):1: failed to close file descriptor 3: bad file descriptor
+?(eval):2: failed to close file descriptor 0: bad file descriptor
 
   print foo >&-
 0:'>&-' redirection
+?(eval):1: failed to close file descriptor 1: bad file descriptor
 
   fn() { local foo; read foo; print $foo; }
   coproc fn

-- 
Peter Stephenson <pws@xxxxxxx>            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
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog