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

Subsystems: the rest

STALE3715d

7 messages, 4 authors, 2016-06-15 · open the first message on its own page

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

From: Thomas Rast <hidden>
Date: 2016-06-15 22:56:37

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


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

From: Eric Wong <hidden>
Date: 2016-06-15 22:56:37

Thomas Rast [off-list ref] 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.

quoted hunk

--- 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 2/2] t9700: do not close STDERR

From: Thomas Rast <hidden>
Date: 2016-06-15 22:56:41

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

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

From: Thomas Rast <hidden>
Date: 2016-06-15 22:56:41

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

quoted hunk

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 2/2] t9700: do not close STDERR

From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:56:41

Thomas Rast wrote:

quoted hunk

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

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

From: Eric Wong <hidden>
Date: 2016-06-15 22:56:41

Thomas Rast [off-list ref] 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 <redacted>

Acked-by: Eric Wong <redacted>
Thanks.

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

From: Petr Baudis <hidden>
Date: 2016-06-15 22:56:42

  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.

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help