NetBSD Problem Report #51123

From kre@munnari.OZ.AU  Sun May  8 04:11:57 2016
Return-Path: <kre@munnari.OZ.AU>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 1FBA57A473
	for <gnats-bugs@www.NetBSD.org>; Sun,  8 May 2016 04:11:57 +0000 (UTC)
Message-Id: <201605080411.u484BMcM006729@andromeda.noi.kre.to>
Date: Sun, 8 May 2016 11:11:22 +0700 (ICT)
From: kre@munnari.OZ.AU
To: gnats-bugs@www.NetBSD.org
Subject: /bin/sh fails to close file (N>&-) in some cases (+FIX)
X-Send-Pr-Version: 3.95

>Number:         51123
>Category:       bin
>Synopsis:       /bin/sh fails to close file (N>&-) in some cases (+FIX)
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun May 08 04:15:00 +0000 2016
>Closed-Date:    Sun May 08 20:17:38 +0000 2016
>Last-Modified:  Mon May 09 22:35:01 +0000 2016
>Originator:     Robert Elz
>Release:        NetBSD 7.99.26   (all NetBSD 7 -> now versions)
>Organization:
>Environment:
System: NetBSD andromeda.noi.kre.to 7.99.26 NetBSD 7.99.26 (VBOX64-1.1-20160128) #43: Thu Jan 28 16:09:08 ICT 2016 kre@onyx.coe.psu.ac.th:/usr/obj/current/kernels/amd64/VBOX64 amd64
Architecture: x86_64
Machine: amd64
>Description:
	In some cases, when explicitly closing a file on the command
	line (using >&- or <&-) sh fails to close the file (or do
	anything at all to it.)

	This only happens when the redirect is applied in one of the
	following cases ...
		a function call
		a compound statement (not including (...) subshells)
		a builtin command (other than exec)
	That is, something executed in the current shell (other than exec).

	This is a side effect of a change made in June 2013, and so
	applies to NetBSD 7, and current (but not to NetBSd 6 or 6.1)

	Note: at first glance, it appears as if this may be the cause
	of the bug reported in PR bin/48875 - however, that was reported
	against NetBSD 6.1, which does not have this bug in it.

	It was while testing a fix for that PR that this problem was
	uncovered (the fix itself worked without fixing this bug, for
	other reasons, writing test cases to validate that the fix
	broke nothing exposed this problem...)

>How-To-Repeat:
	One easy way, if you are running NetBSD 7 or current is ...

		for f in 1 2 3
		do
		echo hello $f
		done >&-

	There should be no output, as stdout should be closed, but
	there won't be none...

	Running that test as
		 ( for f in 1 2 3
		 do
		 echo hello $f
		 done ) >&-
	does indeed produce no output (the sub-shell exemption above)

	An even simpler test is

		echo hello >&-

	(or printf if you prefer).   That's the builtin case.

>Fix:
	Apply the following patch (against current).   This will be
	committed once approved, and a pull-up for NetBSD 7 requested.

Index: redir.c
===================================================================
RCS file: /cvsroot/src/bin/sh/redir.c,v
retrieving revision 1.44
diff -u -u -r1.44 redir.c
--- redir.c	8 May 2016 03:51:15 -0000	1.44
+++ redir.c	8 May 2016 03:54:07 -0000
@@ -219,9 +219,15 @@
 				(void)fcntl(i, F_SETFD, FD_CLOEXEC);
 			fd_rename(sv, fd, i);
 			INTON;
-		} else {
-			close(fd);
 		}
+
+		/*
+		 * openredirect only closes the target fd if it successfuly
+		 * opens a new one, which it does not do for the >&- (<&-)
+		 * case .. so we need to do it here in all cases, not
+		 * only when we did not do the fd_rename() above.
+		 */
+		close(fd);
 		if (fd == 0)
 			fd0_redirected++;
 		openredirect(n, memory, flags);

>Release-Note:

>Audit-Trail:
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/51123: /bin/sh fails to close file (N>&-) in some cases (+FIX)
Date: Sun, 08 May 2016 14:17:44 +0700

     Date:        Sun,  8 May 2016 04:15:00 +0000 (UTC)
     From:        kre@munnari.OZ.AU
     Message-ID:  <20160508041500.2E11C7A473@mollari.NetBSD.org>

   | >Fix:
   | 	Apply the following patch (against current).

 I have a different patch.  This one works just as well for solving the
 problem in the PR, but also allows

 	ls >/dev/stdout

 to work.   That's been broken forever - except, as it happens in the cases
 where the bug from this PR exists (so echo hello >/dev/stdout works, as
 does for ... do ... done >/dev/stdout etc, since the bug was installed in 
 2014.)

 The problem is/was that the fd we are redirecting was closed, then the
 file was opened, but "close(1); open("/dev/stdout",...)" is pretty much
 guaranteed to fail.   The bug from the PR (which "forgot" to do the close
 in some cases) allowed it to work.

 The same applies to </dev/stdin and 2>/dev/stderr of course.   (I don't
 want to even imagine why anyone wants to do any of this, but apparently
 some people do...)

 The patch appended handles the PR problem in a different way, and as a side
 effect, never closes the fd before we open the replacement.   This can 
 sometimes save a system call, as we used to do "close(), open(), dup2()"
 and now we just do "open(), dup2()" in the normal case.   But only sometimes
 save, as if open happens to return the fd we want to use, which would be
 common when that fd was 0, 1, or 2, and has just been closed, the dup2()
 was not needed, so now we're swapping a close() for a dup2().

 This patch applies to current in CVS (simply ignoring the previous patch
 which was never committed.)

 kre

 Index: redir.c
 ===================================================================
 RCS file: /cvsroot/src/bin/sh/redir.c,v
 retrieving revision 1.44
 diff -u -u -r1.44 redir.c
 --- redir.c	8 May 2016 03:51:15 -0000	1.44
 +++ redir.c	8 May 2016 07:07:13 -0000
 @@ -219,8 +219,6 @@
  				(void)fcntl(i, F_SETFD, FD_CLOEXEC);
  			fd_rename(sv, fd, i);
  			INTON;
 -		} else {
 -			close(fd);
  		}
  		if (fd == 0)
  			fd0_redirected++;
 @@ -306,7 +304,8 @@
  			else
  				copyfd(redir->ndup.dupfd, fd, 1,
  				    (flags & REDIR_PUSH) != 0);
 -		}
 +		} else
 +			close(fd);
  		INTON;
  		return;
  	case NHERE:


From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, 
	kre@munnari.OZ.AU
Cc: 
Subject: Re: bin/51123: /bin/sh fails to close file (N>&-) in some cases (+FIX)
Date: Sun, 8 May 2016 10:26:00 -0400

 On May 8,  7:20am, kre@munnari.OZ.AU (Robert Elz) wrote:
 -- Subject: Re: bin/51123: /bin/sh fails to close file (N>&-) in some cases (

 | From: Robert Elz <kre@munnari.OZ.AU>
 | To: gnats-bugs@NetBSD.org
 | Cc: 
 | Subject: Re: bin/51123: /bin/sh fails to close file (N>&-) in some cases (+FIX)
 | Date: Sun, 08 May 2016 14:17:44 +0700
 | 
 |      Date:        Sun,  8 May 2016 04:15:00 +0000 (UTC)
 |      From:        kre@munnari.OZ.AU
 |      Message-ID:  <20160508041500.2E11C7A473@mollari.NetBSD.org>
 |  
 |    | >Fix:
 |    | 	Apply the following patch (against current).
 |  
 |  I have a different patch.  This one works just as well for solving the
 |  problem in the PR, but also allows
 |  
 |  	ls >/dev/stdout
 |  
 |  to work.   That's been broken forever - except, as it happens in the cases
 |  where the bug from this PR exists (so echo hello >/dev/stdout works, as
 |  does for ... do ... done >/dev/stdout etc, since the bug was installed in 
 |  2014.)
 |  
 |  The problem is/was that the fd we are redirecting was closed, then the
 |  file was opened, but "close(1); open("/dev/stdout",...)" is pretty much
 |  guaranteed to fail.   The bug from the PR (which "forgot" to do the close
 |  in some cases) allowed it to work.
 |  
 |  The same applies to </dev/stdin and 2>/dev/stderr of course.   (I don't
 |  want to even imagine why anyone wants to do any of this, but apparently
 |  some people do...)
 |  
 |  The patch appended handles the PR problem in a different way, and as a side
 |  effect, never closes the fd before we open the replacement.   This can 
 |  sometimes save a system call, as we used to do "close(), open(), dup2()"
 |  and now we just do "open(), dup2()" in the normal case.   But only sometimes
 |  save, as if open happens to return the fd we want to use, which would be
 |  common when that fd was 0, 1, or 2, and has just been closed, the dup2()
 |  was not needed, so now we're swapping a close() for a dup2().
 |  
 |  This patch applies to current in CVS (simply ignoring the previous patch
 |  which was never committed.)
 |  
 |  kre

 Fine...

 christos

From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51123 CVS commit: src/bin/sh
Date: Sun, 8 May 2016 20:14:27 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Sun May  8 20:14:27 UTC 2016

 Modified Files:
 	src/bin/sh: redir.c

 Log Message:
 PR bin/51123 - make >&- work in all cases, and while doing that fix
 things so that >/dev/stdout </dev/stdin (etc) work as well (in all cases).

 ok christos@


 To generate a diff of this commit:
 cvs rdiff -u -r1.44 -r1.45 src/bin/sh/redir.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Sun, 08 May 2016 20:17:38 +0000
State-Changed-Why:
Problem fixed


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51123 CVS commit: src/tests/bin/sh
Date: Mon, 9 May 2016 22:34:37 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Mon May  9 22:34:37 UTC 2016

 Modified Files:
 	src/tests/bin/sh: t_redir.sh

 Log Message:
 PR bin/48875   PR bin/51123    This adds tests more that verify fide descriptor
 redirection works correctly (including that the bugs reported in those PRs
 are fixed.)  Note that the tests for 48875 are slow, so one of the new
 test cases ends up running > 25 seconds (just doing sleeps) - each individual
 test is just a few seconds, but there are several of them.

 OK christos@


 To generate a diff of this commit:
 cvs rdiff -u -r1.6 -r1.7 src/tests/bin/sh/t_redir.sh

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search