[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 tr...@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 32 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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Wong]

Thomas Rast tr...@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: $!;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html