threads / rfc / 37850

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

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

## tl;dr

3 messages between Oct 30, 2014 and Oct 31, 2014.

replies: 2people: 2as markdown or json

Eric Wong· Oct 30, 2014, 22:08 UTC · lore

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
Kyle J. McKay· Oct 31, 2014, 00:13 UTC · re: Eric Wong · lore

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

On Oct 30, 2014, at 15:08, Eric Wong wrote:
Show 18 quoted lines
> 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.
With this patch added, do you then see the warning:
   carp "Temp file '", $name,
	"' was closed. Opening replacement.";
 From _temp_cache?  It would seem odd if you didn't unless there was  
only one symlink involved.

I suspect that symlinks were rare in the repositories I was testing against. I wonder if having a test svn repo that adds several new symlinks for several revisions in a row might trigger this problem more robustly.

The _repository->temp_acquire('svn_hash') call is assigned to a "my" variable and then has Git::temp_release called on it later in the same function and the only calls made on it in between are Git::temp_path, so I don't see how the libsvn library could be responsible for closing it since it looks to me like it never sees it. But I'm looking at v2.1.2 of Fetcher.pm, so if some other calls have been inserted there since then.

Show 64 quoted lines
> 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

The changes themselves look okay, but eeewwww. I don't see how libsvn can see the svn_hash fd to close it unless it's randomly closing fds.

--Kyle
Eric Wong· Oct 31, 2014, 01:26 UTC · re: Kyle J. McKay · lore

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

"Kyle J. McKay" <mackyle@gmail.com> wrote:
Show 27 quoted lines
> On Oct 30, 2014, at 15:08, Eric Wong wrote:
> >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.
> 
> With this patch added, do you then see the warning:
> 
>   carp "Temp file '", $name,
> 	"' was closed. Opening replacement.";
> 
> From _temp_cache?  It would seem odd if you didn't unless there was
> only one symlink involved.

Nope. $$temp_fd was defined so the message is not hit. Is suppose we can also make the following change:

--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1282,11 +1282,9 @@ sub _temp_cache {
 				$name . "' already in use");
 		}
 	} else {
-		if (defined $$temp_fd) {
-			# then we're here because of a closed handle.
-			carp "Temp file '", $name,
-				"' was closed. Opening replacement.";
-		}
+		# then we're here because of a closed handle.
+		carp "Temp file '", $name,
+			"' was closed. Opening replacement.";
 		my $fname;
 
 		my $tmpdir;

> I suspect that symlinks were rare in the repositories I was testing
> against.  I wonder if having a test svn repo that adds several new
> symlinks for several revisions in a row might trigger this problem
> more robustly.
> 
> The _repository->temp_acquire('svn_hash') call is assigned to a "my"
> variable and then has Git::temp_release called on it later in the
> same function and the only calls made on it in between are
> Git::temp_path, so I don't see how the libsvn library could be
> responsible for closing it since it looks to me like it never sees
> it.  But I'm looking at v2.1.2 of Fetcher.pm, so if some other calls
> have been inserted there since then.

I stared at it for a long while and could not figure out what was going
on, either.  The other temp_acquire users are far less straightforward
and did not get mysteriously closed.

> The changes themselves look okay, but eeewwww.  I don't see how
> libsvn can see the svn_hash fd to close it unless it's randomly
> closing fds.

Right.  Even worse, I can't even get it to trigger anymore...
Thanks for your response!

← back to recent threads