[PATCH v2 1/2] perl: redirect stderr to /dev/null instead of closing

From: Thomas Rast <trast@inf.ethz.ch>
Date: Apr 4, 2013, 20:41 UTC
Message-ID: <801ebb2a75d7cddfeee70eb86e8854c78d22eb3e.1365107899.git.trast@inf.ethz.ch>
In-Reply-To: <20130404011653.GA28492@dcvr.yhbt.net>

On my system, t9100.1 triggers the following warning:

  ==352== Syscall param write(buf) points to uninitialised byte(s)
  ==352==    at 0x57119C0: __write_nocancel (in /lib64/libc-2.17.so)
  ==352==    by 0x56AC1D2: _IO_file_write@@GLIBC_2.2.5 (in /lib64/libc-2.17.so)
  ==352==    by 0x56AC0B1: new_do_write (in /lib64/libc-2.17.so)
  ==352==    by 0x56AD3B4: _IO_do_write@@GLIBC_2.2.5 (in /lib64/libc-2.17.so)
  ==352==    by 0x56AD6FE: _IO_file_overflow@@GLIBC_2.2.5 (in /lib64/libc-2.17.so)
  ==352==    by 0x56AE3D8: _IO_default_xsputn (in /lib64/libc-2.17.so)
  ==352==    by 0x56ACAA2: _IO_file_xsputn@@GLIBC_2.2.5 (in /lib64/libc-2.17.so)
  ==352==    by 0x5682133: buffered_vfprintf (in /lib64/libc-2.17.so)
  ==352==    by 0x567CE9D: vfprintf (in /lib64/libc-2.17.so)
  ==352==    by 0x5687096: fprintf (in /lib64/libc-2.17.so)
  ==352==    by 0x4E7AC5: vreportf (usage.c:15)
  ==352==    by 0x4E7B14: die_builtin (usage.c:38)

The actual complaint appears to be a bug in the underlying implementation. What's interesting here is that it is apparently _triggered_ by closing stderr, which results in (from strace)

  write(2, "fatal: Needed a single revision\n", 32) = -1 EBADF (Bad file descriptor)
  write(2, "\0", 1) = -1 EBADF (Bad file descriptor)

Closing stderr is a bad idea anyway: there is a very real chance that we print fatal error messages to some other file that just happens to be opened on the now-free FD 2. So let's not do that.

As pointed out by Eric Wong (thanks), the initial close needs to go: die() would again write nowhere if we close STDERR beforehand.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---

Show 14 quoted lines

> Perhaps we should also do the following:
>
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1489,9 +1489,6 @@ sub _command_common_pipe {
>  		if (not defined $pid) {
>  			throw Error::Simple("open failed: $!");
>  		} elsif ($pid == 0) {
> -			if (defined $opts{STDERR}) {
> -				close STDERR;
> -			}
>  			if ($opts{STDERR}) {
>  				open (STDERR, '>&', $opts{STDERR})
>					or die "dup failed: $!";

Indeed.  Thanks for pointing that out.

 perl/Git.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 96cac39..2cec8cf 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1489,12 +1489,12 @@ sub _command_common_pipe {
 		if (not defined $pid) {
 			throw Error::Simple("open failed: $!");
 		} elsif ($pid == 0) {
-			if (defined $opts{STDERR}) {
-				close STDERR;
-			}
 			if ($opts{STDERR}) {
 				open (STDERR, '>&', $opts{STDERR})
 					or die "dup failed: $!";
+			} elsif (defined $opts{STDERR}) {
+				open (STDERR, '>', '/dev/null')
+					or die "opening /dev/null failed: $!";
 			}
 			_cmd_exec($self, $cmd, @args);
 		}

-- 
1.8.2.607.g19d29d3