[RFC] Git.pm: stat the FD to ensure the file is really open

From: Eric Wong <normalperson@yhbt.net>
Date: Oct 30, 2014, 22:08 UTC
Message-ID: <20141030220836.GA7873@dcvr.yhbt.net>

For a (currently) unknown reason, Git::SVN::Fetcher::close_file sometimes triggers "Bad file descriptor" errors on syswrite when writing symlink contents on the "svn_hash" tempfile.

The current IO::Handle::opened call in Git.pm is not a sufficient check, as the underlying file descriptor is closed without the PerlIO layer knowing about it. This is likely a bug inside libsvn (1.6.17), as none of the Git.pm or Git::SVN modules close IOs without the knowledge of the PerlIO layer.

Cc: Kyle J. McKay <mackyle@gmail.com>
Signed-off-by: Eric Wong <normalperson@yhbt.net>
---
 Kyle/Junio: thoughts?  I'm running out of time to track this down
 so it might be necessary for 2.2-rc0.  What's even stranger is
 I cannot always reproduce the problem even without this patch,
 so it may be only triggered by network latency...
 Thanks.

 perl/Git.pm | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index b5905ee..f1f210b 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -99,7 +99,7 @@ increase notwithstanding).
 =cut
 
 
-use Carp qw(carp croak); # but croak is bad - throw instead
+use Carp qw(carp croak cluck); # but croak is bad - throw instead
 use Error qw(:try);
 use Cwd qw(abs_path cwd);
 use IPC::Open2 qw(open2);
@@ -1206,6 +1206,16 @@ sub temp_acquire {
 	$temp_fd;
 }
 
+sub _temp_really_open {
+	my ($fh) = @_;
+
+	if (defined($fh) && $fh->opened) {
+		return 1 if stat($fh);
+		cluck("$fh closed independently of PerlIO\n");
+	}
+	return 0;
+}
+
 =item temp_is_locked ( NAME )
 
 Returns true if the internal lock created by a previous C<temp_acquire()>
@@ -1232,7 +1242,7 @@ sub temp_is_locked {
 	my ($self, $name) = _maybe_self(@_);
 	my $temp_fd = \$TEMP_FILEMAP{$name};
 
-	defined $$temp_fd && $$temp_fd->opened && $TEMP_FILES{$$temp_fd}{locked};
+	_temp_really_open($$temp_fd) && $TEMP_FILES{$$temp_fd}{locked};
 }
 
 =item temp_release ( NAME )
@@ -1264,7 +1274,7 @@ sub temp_release {
 		carp "Attempt to release temp file '",
 			$temp_fd, "' that has not been locked";
 	}
-	temp_reset($temp_fd) if $trunc and $temp_fd->opened;
+	temp_reset($temp_fd) if $trunc && _temp_really_open($temp_fd);
 
 	$TEMP_FILES{$temp_fd}{locked} = 0;
 	undef;
@@ -1276,7 +1286,7 @@ sub _temp_cache {
 	_verify_require();
 
 	my $temp_fd = \$TEMP_FILEMAP{$name};
-	if (defined $$temp_fd and $$temp_fd->opened) {
+	if (_temp_really_open($$temp_fd)) {
 		if ($TEMP_FILES{$$temp_fd}{locked}) {
 			throw Error::Simple("Temp file with moniker '" .
 				$name . "' already in use");

-- 
EW