Can't use bare repositories with Git.pm

Can't use bare repositories with Git.pm

[Rodrigo]

We're having an issue migrating from 2.31.1 to 2.46.0. The following
Perl code does not work in 2.46.0 as it did in 2.31.1 (tested linux
and darwin, did not check Windows):

    # test.pl
    use Git;
    my $repo = Git->repository( Directory => '/repo/bare.git' );
    my ($fh, $ctx) = $repo->command_output_pipe('rev-list', "2acf3456");
    print do { local $/; <$fh> };

Run:

   $ cd /home/rodrigo
   $ perl test.pl

Fails with the error:

    fatal: not a git repository: '/home/rodrigo'

If the repository it points to is a *bare repository* outside of the
current working directory, it only works if the directory is a child
directory to the bare (/repo/bare.git/info). The code above does work
for non-bare repos, and also works if we set `Repository =>
"/repo/bare.git"` instead of `Directory => ...`, but the Git.pm
documentation states `Directory => ...` should work for both bare and
non-bare alike, like it did in 2.31.1 (and other versions).

Bug hunting through the Git.pm code and skimming through the Git SCM
repo, there's a significant change (commit 20da61f25) that makes the
recent Git.pm rely on:

     git rev-parse --is-bare-repository --git-dir

for locating the correct (maybe a parent) repo directory. The change
was understandably done for security (and other many) reasons. It
started using --is-bare-repository to detect if it's a bare repository
we're dealing with, instead of relying on the old Git.pm redundant
code for bare repo detection, which was a sound decision. But some
crucial code was taken out.

Now if the directory path we're passing to `Directory => ...` is a
bare repo this new code will fail because git rev-parse --git-dir will
return a dot `.` (internally Git.pm will chdir() to the directory
before running git rev-parse, hence the result). The issue is that the
dot is now is being taken as is by Git.pm as the intended directory,
tricking it to think my cwd /home/rodrigo is the bare repository,
leading finally to the fatal error.

This could even become a security issue if the Perl program runs from
another working dir which happens to be a sensitive repo. Git.pm
commands would take action on the wrong repo. This hypothetical Perl
program, if run as, ie., a server program, could reveal / change
sensitive information from/on a server.

SOLUTION: I propose using "--absolute-git-dir" instead of "--git-dir":

    git rev-parse --is-bare-repository --absolute-git-dir

Which will replace the `.`  rev-parse response with a full path
resolved by git itself (and not Perl). This means the change to the
Perl code is minimal. I don't know if this will resolve all possible
cases, but it does fix our issue.

Here's a diff on my proposal -- sorry, noob in this neck of the woods,
didn't know if this is the correct way to contribute, but the change
is tiny and better if parsed by seasoned eyes anyway:

diff --git a/perl/Git.pm b/perl/Git.pm
index aebfe0c..63d0f92 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -187,7 +187,7 @@ sub repository {
                try {
                  # Note that "--is-bare-repository" must come first, as
                  # --git-dir output could contain newlines.
-                 $out = $search->command([qw(rev-parse
--is-bare-repository --git-dir)],
+                 $out = $search->command([qw(rev-parse
--is-bare-repository --absolute-git-dir)],
                                          STDERR => 0);
                } catch Git::Error::Command with {
                        throw Error::Simple("fatal: not a git
repository: $opts{Directory}");


thanks and best regards,
Rod
gihub.com/rodrigolive

[PATCH 0/2] fix bare repositories with Git.pm

[Jeff King]

On Thu, Sep 12, 2024 at 06:32:00PM +0200, Rodrigo wrote:

> We're having an issue migrating from 2.31.1 to 2.46.0. The following
> Perl code does not work in 2.46.0 as it did in 2.31.1 (tested linux
> and darwin, did not check Windows):
> 
>     # test.pl
>     use Git;
>     my $repo = Git->repository( Directory => '/repo/bare.git' );
>     my ($fh, $ctx) = $repo->command_output_pipe('rev-list', "2acf3456");
>     print do { local $/; <$fh> };
> 
> Run:
> 
>    $ cd /home/rodrigo
>    $ perl test.pl
> 
> Fails with the error:
> 
>     fatal: not a git repository: '/home/rodrigo'

Yikes, good catch. That's a pretty bad bug. I'm surprised we didn't
cover this in the tests, but it's specific to bare repositories.

> Bug hunting through the Git.pm code and skimming through the Git SCM
> repo, there's a significant change (commit 20da61f25) that makes the
> recent Git.pm rely on:
> 
>      git rev-parse --is-bare-repository --git-dir

Yep, I confirmed via bisection that that commit is the culprit. Your
analysis is mostly good, though...

> for locating the correct (maybe a parent) repo directory. The change
> was understandably done for security (and other many) reasons. It
> started using --is-bare-repository to detect if it's a bare repository
> we're dealing with, instead of relying on the old Git.pm redundant
> code for bare repo detection, which was a sound decision. But some
> crucial code was taken out.

...the problem is actually that not enough code was taken out. ;) I left
the code in the bare-only code to turn the relative path to absolute,
but it used the wrong path (the one returned by rev-parse, rather than
the original Directory option that was passed in). That bare-only path
should just go away entirely, and both should use the same (correct)
code to get the absolute path.

> SOLUTION: I propose using "--absolute-git-dir" instead of "--git-dir":
> 
>     git rev-parse --is-bare-repository --absolute-git-dir
> 
> Which will replace the `.`  rev-parse response with a full path
> resolved by git itself (and not Perl). This means the change to the
> Perl code is minimal. I don't know if this will resolve all possible
> cases, but it does fix our issue.

It does fix all cases, but it leaves some redundant code in place.
Here are two patches. The first does the minimal fix within the code
(what 20da61f25 should have done!) and corrects the problem. The second
switches to --absolute-git-dir and drops the now-redundant code.

Thank you for a very thorough bug report!

  [1/2]: Git.pm: fix bare repository search with Directory option
  [2/2]: Git.pm: use "rev-parse --absolute-git-dir" rather than perl code

 perl/Git.pm         | 14 ++++++--------
 t/t9700-perl-git.sh |  3 ++-
 t/t9700/test.pl     |  5 +++++
 3 files changed, 13 insertions(+), 9 deletions(-)

-Peff

[PATCH 1/2] Git.pm: fix bare repository search with Directory option

[Jeff King]

When opening a bare repository like:

  Git->repository(Directory => '/path/to/bare.git');

we will incorrectly point the repository object at the _current_
directory, not the one specified by the option.

The bug was introduced by 20da61f25f (Git.pm: trust rev-parse to find
bare repositories, 2022-10-22). Before then, we'd ask "rev-parse
--git-dir" if it was a Git repo, and if it returned anything, we'd
correctly convert that result to an absolute path using File::Spec and
Cwd::abs_path(). If it didn't, we'd guess it might be a bare repository
and find it ourselves, which was wrong (rev-parse should find even a
bare repo, and our search circumvented some of its rules).

That commit dropped most of the custom bare-repo search code in favor of
using "rev-parse --is-bare-repository" and trusting the "--git-dir" it
returned. But it mistakenly left some of the bare-repo code path in
place, which was now broken. That code calls Cwd::abs_path($dir); prior
to 20da61f25f $dir contained the "Directory" option the user passed in.
But afterwards, it contains the output of "rev-parse --git-dir". And
since our tentative rev-parse command is invoked after changing
directory, it will always be the relative path "."! So we'll end up with
the absolute path of the process's current directory, not the Directory
option the caller asked for.

So the non-bare case is correct, but the bare one is broken. Our tests
only check the non-bare one, so we didn't notice. We can fix this by
running the same absolute-path fixup code for both sides.

Helped-by: Rodrigo <rodrigolive@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 perl/Git.pm         | 10 ++++------
 t/t9700-perl-git.sh |  3 ++-
 t/t9700/test.pl     |  5 +++++
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index aebfe0c6e0..cf1ef0b32a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -197,11 +197,11 @@ sub repository {
 		my ($bare, $dir) = split /\n/, $out, 2;
 
 		require Cwd;
-		if ($bare ne 'true') {
-			require File::Spec;
-			File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;
-			$opts{Repository} = Cwd::abs_path($dir);
+		require File::Spec;
+		File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;
+		$opts{Repository} = Cwd::abs_path($dir);
 
+		if ($bare ne 'true') {
 			# If --git-dir went ok, this shouldn't die either.
 			my $prefix = $search->command_oneline('rev-parse', '--show-prefix');
 			$dir = Cwd::abs_path($opts{Directory}) . '/';
@@ -214,8 +214,6 @@ sub repository {
 			$opts{WorkingCopy} = $dir;
 			$opts{WorkingSubdir} = $prefix;
 
-		} else {
-			$opts{Repository} = Cwd::abs_path($dir);
 		}
 
 		delete $opts{Directory};
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index ccc8212d73..4431697122 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -45,7 +45,8 @@ test_expect_success 'set up test repository' '
 '
 
 test_expect_success 'set up bare repository' '
-	git init --bare bare.git
+	git init --bare bare.git &&
+	git -C bare.git --work-tree=. commit --allow-empty -m "bare commit"
 '
 
 test_expect_success 'use t9700/test.pl to test Git.pm' '
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index d8e85482ab..2e1d50d4d1 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -147,6 +147,11 @@ sub adjust_dirsep {
 unlink $tmpfile3;
 chdir($abs_repo_dir);
 
+# open alternate bare repo
+my $r4 = Git->repository(Directory => "$abs_repo_dir/bare.git");
+is($r4->command_oneline(qw(log --format=%s)), "bare commit",
+	"log of bare repo works");
+
 # unquoting paths
 is(Git::unquote_path('abc'), 'abc', 'unquote unquoted path');
 is(Git::unquote_path('"abc def"'), 'abc def', 'unquote simple quoted path');
-- 
2.46.0.918.gab30941bff

[PATCH 2/2] Git.pm: use "rev-parse --absolute-git-dir" rather than perl code

[Jeff King]

When we open a repository with the "Directory" option, we use "rev-parse
--git-dir" to get the path relative to that directory, and then use
Cwd::abs_path() to make it absolute (since our process working directory
may not be the same).

These days we can just ask for "--absolute-git-dir" instead, which saves
us a little code. That option was added in Git v2.13.0 via a2f5a87626
(rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think
we make any promises about running mismatched versions of git and
Git.pm, but even if somebody tries it, that's sufficiently old that it
should be OK.

Signed-off-by: Jeff King <peff@peff.net>
---
I retained the "require Cwd" here since we use it in the conditional
(but moved it closer to the point of use). It's not strictly necessary,
as earlier code will have required it as a side effect, but it's
probably best not to rely on that.

 perl/Git.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index cf1ef0b32a..667152c6c6 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -187,7 +187,7 @@ sub repository {
 		try {
 		  # Note that "--is-bare-repository" must come first, as
 		  # --git-dir output could contain newlines.
-		  $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)],
+		  $out = $search->command([qw(rev-parse --is-bare-repository --absolute-git-dir)],
 			                  STDERR => 0);
 		} catch Git::Error::Command with {
 			throw Error::Simple("fatal: not a git repository: $opts{Directory}");
@@ -196,12 +196,12 @@ sub repository {
 		chomp $out;
 		my ($bare, $dir) = split /\n/, $out, 2;
 
-		require Cwd;
-		require File::Spec;
-		File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;
-		$opts{Repository} = Cwd::abs_path($dir);
+		# We know this is an absolute path, because we used
+		# --absolute-git-dir above.
+		$opts{Repository} = $dir;
 
 		if ($bare ne 'true') {
+			require Cwd;
 			# If --git-dir went ok, this shouldn't die either.
 			my $prefix = $search->command_oneline('rev-parse', '--show-prefix');
 			$dir = Cwd::abs_path($opts{Directory}) . '/';
-- 
2.46.0.918.gab30941bff

Re: [PATCH 1/2] Git.pm: fix bare repository search with Directory option

[Patrick Steinhardt]

On Thu, Sep 12, 2024 at 06:36:04PM -0400, Jeff King wrote:
> diff --git a/perl/Git.pm b/perl/Git.pm
> index aebfe0c6e0..cf1ef0b32a 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -197,11 +197,11 @@ sub repository {
>  		my ($bare, $dir) = split /\n/, $out, 2;
>  
>  		require Cwd;
> -		if ($bare ne 'true') {
> -			require File::Spec;
> -			File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;
> -			$opts{Repository} = Cwd::abs_path($dir);
> +		require File::Spec;
> +		File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;
> +		$opts{Repository} = Cwd::abs_path($dir);
>  
> +		if ($bare ne 'true') {
>  			# If --git-dir went ok, this shouldn't die either.
>  			my $prefix = $search->command_oneline('rev-parse', '--show-prefix');
>  			$dir = Cwd::abs_path($opts{Directory}) . '/';
> @@ -214,8 +214,6 @@ sub repository {
>  			$opts{WorkingCopy} = $dir;
>  			$opts{WorkingSubdir} = $prefix;
>  
> -		} else {
> -			$opts{Repository} = Cwd::abs_path($dir);
>  		}
>  
>  		delete $opts{Directory};

Makes sense. We already knew that the $dir was relative, but only
remembered to handle this in case the repository was non-bare. Now both
cases use the same code to translate the relative path to an absolute
one.

> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
> index ccc8212d73..4431697122 100755
> --- a/t/t9700-perl-git.sh
> +++ b/t/t9700-perl-git.sh
> @@ -45,7 +45,8 @@ test_expect_success 'set up test repository' '
>  '
>  
>  test_expect_success 'set up bare repository' '
> -	git init --bare bare.git
> +	git init --bare bare.git &&
> +	git -C bare.git --work-tree=. commit --allow-empty -m "bare commit"
>  '
>  
>  test_expect_success 'use t9700/test.pl to test Git.pm' '

I didn't even know that this hack was possible. I guess the alternative
would be to use git-commit-tree(1) with the empty tree ID, but that'd
also require us to update branches manually via git-update-ref(1). So...
a bit gross, but hey, if it works...

Patrick

Re: [PATCH 2/2] Git.pm: use "rev-parse --absolute-git-dir" rather than perl code

[Patrick Steinhardt]

On Thu, Sep 12, 2024 at 06:37:25PM -0400, Jeff King wrote:
> When we open a repository with the "Directory" option, we use "rev-parse
> --git-dir" to get the path relative to that directory, and then use
> Cwd::abs_path() to make it absolute (since our process working directory
> may not be the same).
> 
> These days we can just ask for "--absolute-git-dir" instead, which saves
> us a little code. That option was added in Git v2.13.0 via a2f5a87626
> (rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think
> we make any promises about running mismatched versions of git and
> Git.pm, but even if somebody tries it, that's sufficiently old that it
> should be OK.

Agreed. We should eventually be able to rely on things that we have
implemented many years ago.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I retained the "require Cwd" here since we use it in the conditional
> (but moved it closer to the point of use). It's not strictly necessary,
> as earlier code will have required it as a side effect, but it's
> probably best not to rely on that.
> 
>  perl/Git.pm | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/perl/Git.pm b/perl/Git.pm
> index cf1ef0b32a..667152c6c6 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -187,7 +187,7 @@ sub repository {
>  		try {
>  		  # Note that "--is-bare-repository" must come first, as
>  		  # --git-dir output could contain newlines.
> -		  $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)],
> +		  $out = $search->command([qw(rev-parse --is-bare-repository --absolute-git-dir)],
>  			                  STDERR => 0);
>  		} catch Git::Error::Command with {
>  			throw Error::Simple("fatal: not a git repository: $opts{Directory}");
> @@ -196,12 +196,12 @@ sub repository {
>  		chomp $out;
>  		my ($bare, $dir) = split /\n/, $out, 2;

This line here made me think for a second what happens if the absolute
path contains newlines. But it should be fine, because we only split at
the first newline character we find. And as the first parameter that we
pass to git-rev-parse(1) is `--is-bare-repository`, we know that it will
output either `true` or `false` as the first line. Any subsequent
newlines should thus be handled alright.

Patrick

Re: [PATCH 0/2] fix bare repositories with Git.pm

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yikes, good catch. That's a pretty bad bug. I'm surprised we didn't
> cover this in the tests, but it's specific to bare repositories.
>
>> Bug hunting through the Git.pm code and skimming through the Git SCM
>> repo, there's a significant change (commit 20da61f25) that makes the
>> recent Git.pm rely on:
>> 
>>      git rev-parse --is-bare-repository --git-dir
>
> Yep, I confirmed via bisection that that commit is the culprit.

Yeah, it is surprising that nobody noticed it since Dec 2022.

> It does fix all cases, but it leaves some redundant code in place.
> Here are two patches. The first does the minimal fix within the code
> (what 20da61f25 should have done!) and corrects the problem. The second
> switches to --absolute-git-dir and drops the now-redundant code.
>
> Thank you for a very thorough bug report!

Yup, thanks, both.

Queued.