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

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

[Thomas Rast]

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.

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


The commit message is intentionally overdramatic on the chance of
printing stuff to bad places.  The code is actually from way back in
2006 (!).

The t9100 problem bisects to e3bd4dd (git-svn: don't create master if
another head exists, 2012-06-24), but that's just changing some
verify_ref(), which asks to close stderr on the git-rev-parse process.


I can easily reproduce the underlying issue with a small test: running

  #include <stdio.h>

  int main ()
  {
      	  fprintf(stderr, "%s%s\n", "fatal: ", "needed a single revision");
      	  return 0;
  }

with

  valgrind --log-fd=3 ./die_test 3>&2 2>&-

results in pretty much the same warnings.  I fail to see a reason
other than a glibc bug why

  fprintf(stderr, "%s%s\n", ...);

should attempt to write "\0" -- all its inputs are C strings.  But
maybe I'm missing something?



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

diff --git a/perl/Git.pm b/perl/Git.pm
index 96cac39..3b79a36 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1495,6 +1495,9 @@ sub _command_common_pipe {
 			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.551.g91a1e48

Re: [PATCH] perl: redirect stderr to /dev/null instead of closing

[Eric Wong]

Thomas Rast <trast@inf.ethz.ch> wrote:
> 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.

100% agreed.  FD 0, 1, and 2 should not be closed, way too much
potential for triggering rare bugs and interop issues like these to be
worth it.

> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -1495,6 +1495,9 @@ sub _command_common_pipe {
>  			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);
>  		}

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: $!";

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

[Thomas Rast]

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>
---

> 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

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

[Eric Wong]

Thomas Rast <trast@inf.ethz.ch> wrote:
> 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>

Acked-by: Eric Wong <normalperson@yhbt.net>
Thanks.

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

[Petr Baudis]

  Hi!

On Thu, Apr 04, 2013 at 10:41:41PM +0200, Thomas Rast wrote:
> As pointed out by Eric Wong (thanks), the initial close needs to go:
> die() would again write nowhere if we close STDERR beforehand.
> 
> > 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.

  I'm sorry, I don't follow. Doesn't this just break the STDERR option
altogether as we will try to dup2() over an already open file
descriptor? We do need to close STDERR if we are going to reopen it,
I think.

  Kind regards,

				Petr "Pasky" Baudis

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

[Junio C Hamano]

Petr Baudis <pasky@ucw.cz> writes:

>> >  		} 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.
>
>   I'm sorry, I don't follow. Doesn't this just break the STDERR option
> altogether as we will try to dup2() over an already open file
> descriptor? We do need to close STDERR if we are going to reopen it,
> I think.

When $opts{STDERR} is 2, what the three lines the proposed patch
removes did is actively wrong, because you dup2 the fd you just
closed.

When $opts{STDERR} is 1, it seems to do the right thing with or
without the "close STDERR" in front.  Isn't this because the usual
"open($fd, <<<anything>>>) closes $fd as necessary" applies to this
case as well?

So, I am not sure what you are viewing as a problem.  Puzzled...

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

[Petr Baudis]

On Fri, Apr 05, 2013 at 11:57:19AM -0700, Junio C Hamano wrote:
> Petr Baudis <pasky@ucw.cz> writes:
> >> > -			if (defined $opts{STDERR}) {
> >> > -				close STDERR;
> >> > -			}
> >> >  			if ($opts{STDERR}) {
> >> >  				open (STDERR, '>&', $opts{STDERR})
> >
> >   I'm sorry, I don't follow. Doesn't this just break the STDERR option
> > altogether as we will try to dup2() over an already open file
> > descriptor? We do need to close STDERR if we are going to reopen it,
> > I think.
> 
> When $opts{STDERR} is 2, what the three lines the proposed patch
> removes did is actively wrong, because you dup2 the fd you just
> closed.

Indeed, though $opts{STDERR} == 2 is something weird to do, it is a case
to consider.

> When $opts{STDERR} is 1, it seems to do the right thing with or
> without the "close STDERR" in front.  Isn't this because the usual
> "open($fd, <<<anything>>>) closes $fd as necessary" applies to this
> case as well?

I never actually tried that and was always happy to go with perldoc
maxim

	To (re)open "STDOUT" or "STDERR" as an in-memory file, close it first:
	           close STDOUT;
	           open(STDOUT, ">", \$variable)
	               or die "Can't open STDOUT: $!";

but my assumption that this generalizes to other kinds of open was
apparently invalid; an example further down the page proves me wrong
completely, moreover.

  The thing is, I was confused about dup2() all along as my old UNIX
masters taught me that I must close() the original descriptor first
and since that's what's commonly done anyway, I never thought to
double-check. Now I did and I learned something new, thanks!

I guess Acked-by: Petr Baudis <pasky@ucw.cz> then. :-)

-- 
				Petr "Pasky" Baudis
	For every complex problem there is an answer that is clear,
	simple, and wrong.  -- H. L. Mencken

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

[Thomas Rast]

Petr Baudis <pasky@ucw.cz> writes:

> On Fri, Apr 05, 2013 at 11:57:19AM -0700, Junio C Hamano wrote:
>   The thing is, I was confused about dup2() all along as my old UNIX
> masters taught me that I must close() the original descriptor first
> and since that's what's commonly done anyway, I never thought to
> double-check. Now I did and I learned something new, thanks!

Indeed, that's the crucial point here.  dup2() is defined to close the
original FD first if needed.

It's much saner this way for the case of stderr, as there is no time
when we have no stderr available to report errors: the FD is replace
atomically from the POV of the program.

The manpage for dup2 does, however, say

   If newfd was open, any errors  that  would  have  been  reported  at
   close(2) time are lost.  A careful programmer will not use dup2() or
   dup3() without closing newfd first.

which is probably what you were referring to.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Petr Baudis]

On Sat, Apr 06, 2013 at 10:07:40AM +0200, Thomas Rast wrote:
> The manpage for dup2 does, however, say
> 
>    If newfd was open, any errors  that  would  have  been  reported  at
>    close(2) time are lost.  A careful programmer will not use dup2() or
>    dup3() without closing newfd first.
> 
> which is probably what you were referring to.

Yes, that's probably one reason why I had this stuck in my mind (though,
how often does anyone bother to detect errors on close()...? ;-).

Funnily enough, POSIX.2008 specifies that if closing newfd would fail,
dup2() reports EIO and newfd is not closed, eliminating this problem.

The manpage does not cover this; well, that's fair enough as Linux just
doesn't care and never does that if I didn't miss anything in the code.

-- 
			Petr "Pasky who might even send
				a patch, but the matter is
				oh so obscure" Baudis

[PATCH v2 2/2] t9700: do not close STDERR

[Thomas Rast]

Much like the previous patch, this triggered an unrelated bug.
Closing STDERR is not worth it anyway, as we risk writing die() and
such to random files that happen to be subsequently opened on FD 2.
Don't do it.

Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
---
 t/t9700/test.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 0d4e366..1140767 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -45,7 +45,8 @@ is($r->get_color("color.test.slot1", "red"), $ansi_green, "get_color");
 # Failure cases for config:
 # Save and restore STDERR; we will probably extract this into a
 # "dies_ok" method and possibly move the STDERR handling to Git.pm.
-open our $tmpstderr, ">&STDERR" or die "cannot save STDERR"; close STDERR;
+open our $tmpstderr, ">&STDERR" or die "cannot save STDERR";
+open STDERR, ">", "/dev/null" or die "cannot redirect STDERR to /dev/null";
 is($r->config("test.dupstring"), "value2", "config: multivar");
 eval { $r->config_bool("test.boolother") };
 ok($@, "config_bool: non-boolean values fail");
-- 
1.8.2.607.g19d29d3

Re: [PATCH v2 2/2] t9700: do not close STDERR

[Jonathan Nieder]

Thomas Rast wrote:

> --- a/t/t9700/test.pl
> +++ b/t/t9700/test.pl
> @@ -45,7 +45,8 @@ is($r->get_color("color.test.slot1", "red"), $ansi_green, "get_color");
>  # Failure cases for config:
>  # Save and restore STDERR; we will probably extract this into a
>  # "dies_ok" method and possibly move the STDERR handling to Git.pm.
> -open our $tmpstderr, ">&STDERR" or die "cannot save STDERR"; close STDERR;
> +open our $tmpstderr, ">&STDERR" or die "cannot save STDERR";
> +open STDERR, ">", "/dev/null" or die "cannot redirect STDERR to /dev/null";
>  is($r->config("test.dupstring"), "value2", "config: multivar");
>  eval { $r->config_bool("test.boolother") };
>  ok($@, "config_bool: non-boolean values fail");
>  open STDERR, ">&", $tmpstderr or die "cannot restore STDERR";

Yeah, this makes sense.

At first I was confused: why not just let stderr go out to the console,
where a person reading can see it?  But this test is meant to be run
using test_external_without_stderr, which redirects stderr to a file and
dies if it ends up getting any content.

perlfunc(1) documents the close-and-then-open trick for redirecting a
filehandle to an in-memory buffer.  Here a plain reopen works fine.

So for what it's worth
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>