| Message ID | 20221016212236.12453-2-michael@mcclimon.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 77a1310e6b3f39f405275faa8c5af02cf5453cb6 |
| Headers | show |
| Series | Git.pm: add semicolon after catch statement | expand |
On Sun, Oct 16, 2022 at 05:22:36PM -0400, Michael McClimon wrote: > When attempting to initialize a repository object in an unsafe > directory, a syntax error is reported (Can't use string as a HASH ref > while strict refs in use). Fix this runtime error by adding the required > semicolon after the catch statement. > > Without the semicolon, the result of the following line (i.e., the > result of Cwd::abs_path) is passed as the third argument to Error.pm's > catch function. That function expects that its third argument, > $clauses, is a hash reference, and trying to access a string as a hash > reference is a fatal error. Curiously this works as expected for me, both before and after your patch. I wonder if it depends on perl version. Mine is 5.34. I've never used Error.pm's try/catch before, so I don't know what's normal. Regular if/unless doesn't need it, but certainly an earlier catch uses a semicolon. So it seems like a reasonable fix. > diff --git a/perl/Git.pm b/perl/Git.pm > index 080cdc2a..cf15ead6 100644 > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -217,7 +217,7 @@ sub repository { > } catch Git::Error::Command with { > # Mimic git-rev-parse --git-dir error message: > throw Error::Simple("fatal: Not a git repository: $dir"); > - } > + }; I'd assume t9700 passes for you, since I don't think we cover this case. Maybe it's worth squashing this in: diff --git a/t/t9700/test.pl b/t/t9700/test.pl index e046f7db76..5bd3687f37 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -30,6 +30,12 @@ sub adjust_dirsep { # set up our $abs_repo_dir = cwd(); ok(our $r = Git->repository(Directory => "."), "open repository"); +{ + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; + my $failed = eval { Git->repository(Directory => ".") }; + ok(!$failed, "reject unsafe repository"); + like($@, qr/not a git repository/i, "unsafe error message"); +} # config is($r->config("test.string"), "value", "config scalar: string");
> Curiously this works as expected for me, both before and after your > patch. I wonder if it depends on perl version. Mine is 5.34. Hm, curious indeed! It reliably fails without my patch and passes with it on all the versions I had lying around (5.8, 5.18, 5.24, 5.26, 5.28, 5.30, 5.34, and 5.36). > I've never used Error.pm's try/catch before, so I don't know what's > normal. Regular if/unless doesn't need it, but certainly an earlier > catch uses a semicolon. So it seems like a reasonable fix. Ha, well Perl is...let's say special. try/catch is not a language construct (until 5.34, where it is experimental), and so it's always implemented by subroutines. One upshot of this is that try/catch needs a semicolon, because it's sugar for try(sub { ... }), and statements need semicolons separating them. Compare these two examples: -MO=Deparse,-p tells perl to deparse the program and print it with parentheses: $ perl -MError -MO=Deparse,-p -e 'try { die "bad" } catch Pkg with { warn "caught" }; print "after"' do { die('bad') }->try('Pkg'->catch(do { warn('caught') }->with)); print('after'); -e syntax OK $ perl -MError -MO=Deparse,-p -e 'try { die "bad" } catch Pkg with { warn "caught" } print "after"' do { die('bad') }->try('Pkg'->catch(do { warn('caught') }->with(print('after')))); -e syntax OK The first here is the good case case (with semicolon), and you can see that print('after') is its own statement. The bad case, with no semicolon, passes it as an argument to with(), which is what's causing the error here: something called by with() is trying to use it as a hash reference, and it's a string. > I'd assume t9700 passes for you, since I don't think we cover this case. > Maybe it's worth squashing this in: > > diff --git a/t/t9700/test.pl b/t/t9700/test.pl > index e046f7db76..5bd3687f37 100755 > --- a/t/t9700/test.pl > +++ b/t/t9700/test.pl > @@ -30,6 +30,12 @@ sub adjust_dirsep { > # set up > our $abs_repo_dir = cwd(); > ok(our $r = Git->repository(Directory => "."), "open repository"); > +{ > + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; > + my $failed = eval { Git->repository(Directory => ".") }; > + ok(!$failed, "reject unsafe repository"); > + like($@, qr/not a git repository/i, "unsafe error message"); > +} > > # config > is($r->config("test.string"), "value", "config scalar: string"); Curiously, t9700 passes for me with this suggestion both with and without my patch. You'd only see this bug in bare repositories, though, and the one set up in t9700 is not bare. I can see about trying to make it do so, but I'll need to do a bit more reading of how even the tests are set up and run first. Thanks!
[+cc people who worked on safe-directory stuff; please check out the included test and final comments] On Sun, Oct 16, 2022 at 10:17:53PM -0400, Michael McClimon wrote: > > Curiously this works as expected for me, both before and after your > > patch. I wonder if it depends on perl version. Mine is 5.34. > > Hm, curious indeed! It reliably fails without my patch and passes with it on > all the versions I had lying around (5.8, 5.18, 5.24, 5.26, 5.28, 5.30, 5.34, > and 5.36). Doh, sorry to mislead you; I hadn't noticed this was in the bare repository code path until you pointed it out. I get the same outcome as you and the OP once that is fixed (both in t9700 and in my manual testing). > Ha, well Perl is...let's say special. try/catch is not a language construct > (until 5.34, where it is experimental), and so it's always implemented by > subroutines. One upshot of this is that try/catch needs a semicolon, because > it's sugar for try(sub { ... }), and statements need semicolons separating > them. Right, I imagined it was something like that. Your fix is definitely the right thing, then. > Curiously, t9700 passes for me with this suggestion both with and without my > patch. You'd only see this bug in bare repositories, though, and the one set > up in t9700 is not bare. I can see about trying to make it do so, but I'll > need to do a bit more reading of how even the tests are set up and run first. Yeah, this test is particularly confusing because unlike most of our suite, it drives the test harness using a separate perl script. So you have setup in one file and tests in another. You'd want something like: diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh index 4aa5d90d32..53a838a8e8 100755 --- a/t/t9700-perl-git.sh +++ b/t/t9700-perl-git.sh @@ -45,6 +45,10 @@ test_expect_success \ git config --add test.pathmulti bar ' +test_expect_success 'set up bare repository' ' + git init --bare bare.git +' + test_expect_success 'use t9700/test.pl to test Git.pm' ' "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr && test_must_be_empty stderr diff --git a/t/t9700/test.pl b/t/t9700/test.pl index e046f7db76..917b09cdf9 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -30,6 +30,12 @@ sub adjust_dirsep { # set up our $abs_repo_dir = cwd(); ok(our $r = Git->repository(Directory => "."), "open repository"); +{ + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; + my $failed = eval { Git->repository(Directory => "$abs_repo_dir/bare.git") }; + ok(!$failed, "reject unsafe bare repository"); + like($@, qr/not a git repository/i, "unsafe error message"); +} # config is($r->config("test.string"), "value", "config scalar: string"); But curiously this still does not pass after your patch, because we seem to actually open the repository! I think this is because the C code allows an explicit GIT_DIR to override the safe-directory checks. But in this case that GIT_DIR is set by Git.pm searching for the directory, not because the user desires it. (I know that I used a "Directory" in the example above, but that is only to avoid extra chdir-ing around in the test script; calling a bare Git->repository() triggers the same behavior in the right directory). So your patch is definitely still the right thing to do, but it feels like a hole in the safe-directory mechanism, at least when called via Git.pm. +cc folks who worked on that. -Peff
> Yeah, this test is particularly confusing because unlike most of our > suite, it drives the test harness using a separate perl script. So you > have setup in one file and tests in another. Oh good, I'm glad it wasn't just me; this was very helpful, thanks. > > But curiously this still does not pass after your patch, because we seem > to actually open the repository! I think this is because the C code > allows an explicit GIT_DIR to override the safe-directory checks. But in > this case that GIT_DIR is set by Git.pm searching for the directory, not > because the user desires it. Aha; this gets to what I was saying in the cover letter, which is that my patch only fixes the runtime error from perl, and Git.pm happily carries on in an unsafe directory. Fixing _that_ is probably beyond my knowledge! > So your patch is definitely still the right thing to do, but it feels > like a hole in the safe-directory mechanism, at least when called via > Git.pm. +cc folks who worked on that. If we wanted a test for just the runtime error, something like the following (including your earlier suggestion to set up the bare repo) demonstrates the bug and my fix. It doesn't seem like a particularly valuable test on its own, IMO, but I'm happy to reroll if we want it. Thanks! diff --git a/t/t9700/test.pl b/t/t9700/test.pl index e046f7db..72681849 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -30,6 +30,11 @@ sub adjust_dirsep { # set up our $abs_repo_dir = cwd(); ok(our $r = Git->repository(Directory => "."), "open repository"); +{ + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; + eval { Git->repository(Directory => "$abs_repo_dir/bare.git") }; + unlike($@, qr/as a HASH ref/i, "no error from perl"); +} # config is($r->config("test.string"), "value", "config scalar: string");
Hi Peff, On Mon, 17 Oct 2022, Jeff King wrote: > [... talking about safe.directory ...] > > But curiously this still does not pass after your patch, because we seem > to actually open the repository! I think this is because the C code > allows an explicit GIT_DIR to override the safe-directory checks. Yes, I remember that this was something we discussed at some length during the embargo: what to do with the explicitly-specified `GIT_DIR` when verifying the ownership, and my recollection is that we asserted that setting `GIT_DIR` qualifies for "they know what they're doing" (in particular when it is done in a script, not interactively). Ciao, Dscho
On Thu, Nov 10, 2022 at 04:10:22PM +0100, Johannes Schindelin wrote: > Hi Peff, > > On Mon, 17 Oct 2022, Jeff King wrote: > > > [... talking about safe.directory ...] > > > > But curiously this still does not pass after your patch, because we seem > > to actually open the repository! I think this is because the C code > > allows an explicit GIT_DIR to override the safe-directory checks. > > Yes, I remember that this was something we discussed at some length during > the embargo: what to do with the explicitly-specified `GIT_DIR` when > verifying the ownership, and my recollection is that we asserted that > setting `GIT_DIR` qualifies for "they know what they're doing" (in > particular when it is done in a script, not interactively). Thanks for confirming. I'm not sure if you read the rest of the thread, but the bug turned out to be in Git.pm, which sets GIT_DIR without knowing what it's doing. :) We ended up with 20da61f25f (Git.pm: trust rev-parse to find bare repositories, 2022-10-22) as the fix. -Peff
diff --git a/perl/Git.pm b/perl/Git.pm index 080cdc2a..cf15ead6 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -217,7 +217,7 @@ sub repository { } catch Git::Error::Command with { # Mimic git-rev-parse --git-dir error message: throw Error::Simple("fatal: Not a git repository: $dir"); - } + }; $opts{Repository} = Cwd::abs_path($dir); }
When attempting to initialize a repository object in an unsafe directory, a syntax error is reported (Can't use string as a HASH ref while strict refs in use). Fix this runtime error by adding the required semicolon after the catch statement. Without the semicolon, the result of the following line (i.e., the result of Cwd::abs_path) is passed as the third argument to Error.pm's catch function. That function expects that its third argument, $clauses, is a hash reference, and trying to access a string as a hash reference is a fatal error. [1] https://lore.kernel.org/git/20221011182607.f1113fff-9333-427d-ba45-741a78fa6040@korelogic.com/ Reported-by: Hank Leininger <hlein@korelogic.com> Signed-off-by: Michael McClimon <michael@mcclimon.org> --- perl/Git.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)