threads / patch / 33373

patchperl: redirect stderr to /dev/null instead of closing

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

## tl;dr

11 messages between Apr 3, 2013 and Apr 6, 2013. Diffs are folded; open one to read it.

replies: 10people: 5as markdown or json

Thomas Rast· Apr 3, 2013, 22:26 UTC · lore
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(+)
Show changes to perl/Git.pm +3 −0
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
Eric Wong· Apr 4, 2013, 01:16 UTC · re: Thomas Rast · lore

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

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.

Show 12 quoted lines
> --- 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:
Show changes to perl/Git.pm +0 −3
--- 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: $!";
Thomas Rast· Apr 4, 2013, 20:41 UTC · re: Eric Wong · lore

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

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(-)
Show changes to perl/Git.pm +3 −3
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
Eric Wong· Apr 4, 2013, 21:14 UTC · re: Thomas Rast · lore

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

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.
Petr Baudis· Apr 5, 2013, 14:48 UTC · re: Thomas Rast · lore

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

  Hi!
On Thu, Apr 04, 2013 at 10:41:41PM +0200, Thomas Rast wrote:
Show 19 quoted lines
> 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
Junio C Hamano· Apr 5, 2013, 18:57 UTC · re: Petr Baudis · lore

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

Petr Baudis <pasky@ucw.cz> writes:
Show 14 quoted lines
>> >  		} 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...
Petr Baudis· Apr 5, 2013, 23:34 UTC · re: Junio C Hamano · lore

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

On Fri, Apr 05, 2013 at 11:57:19AM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> 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
Thomas Rast· Apr 6, 2013, 08:07 UTC · re: Petr Baudis · lore

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

Petr Baudis <pasky@ucw.cz> writes:
Show 5 quoted lines
> 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
Petr Baudis· Apr 6, 2013, 10:34 UTC · re: Thomas Rast · lore

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

On Sat, Apr 06, 2013 at 10:07:40AM +0200, Thomas Rast wrote:
Show 7 quoted lines
> 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
Thomas Rast· Apr 4, 2013, 20:41 UTC · re: Eric Wong · lore

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

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(-)
Show changes to t/t9700/test.pl +2 −1
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
Jonathan Nieder· Apr 4, 2013, 21:11 UTC · re: Thomas Rast · lore

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

Thomas Rast wrote:
Show 13 quoted lines
> --- 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>

← back to recent threads