Bug: `git maintenance start` is likely broken in 2.47

Bug: `git maintenance start` is likely broken in 2.47

[Shubham Kanodia]

In the most recent release of git (2.47.0), running `git maintenance
start` results in a segmentation fault error both on mac & ubuntu.

`b6c3f8e12c` seems to be the first bad commit.

Re: Bug: `git maintenance start` is likely broken in 2.47

[Patrick Steinhardt]

On Tue, Oct 08, 2024 at 04:20:19PM +0530, Shubham Kanodia wrote:
> In the most recent release of git (2.47.0), running `git maintenance
> start` results in a segmentation fault error both on mac & ubuntu.
> 
> `b6c3f8e12c` seems to be the first bad commit.

This is embarassingly easy to reproduce. None of our tests catch the
issue because we are forced to stub out the commands that run, and the
error only happens when _not_ stubbing it out. I'll send a patch later
today to address the issue and will also think about a way to test this.

Patrick

[PATCH] builtin/gc: fix crash when running `git maintenance start`

[Patrick Steinhardt]

It was reported on the mailing list that running `git maintenance start`
immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix
leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is
trivial to reproduce up to a point where one is scratching their head
why we didn't catch this regression in our test suite.

The root cause of this error is `get_schedule_cmd()`, which does not
populate the `out` parameter in all cases anymore starting with the
mentioned commit. Callers do assume it to always be populated though and
will e.g. call `strvec_split()` on the returned value, which will of
course segfault when the variable is uninitialized.

So why didn't we catch this trivial regression? The reason is that our
tests always set up the "GIT_TEST_MAINT_SCHEDULER" environment variable
via "t/test-lib.sh", which allows us to override the scheduler command
with a custom one so that we don't accidentally modify the developer's
system. But the faulty code where we don't set the `out` parameter will
only get hit in case that environment variable is _not_ set, which is
never the case when executing our tests.

Fix the regression by again unconditionally allocating the value in the
`out` parameter, if provided. Add a test that unsets the environment
variable to catch future regressions in this area.

Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/gc.c           |  7 +++++--
 t/t7900-maintenance.sh | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 6a7a2da006..d52735354c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1832,7 +1832,7 @@ static const char *get_extra_launchctl_strings(void) {
  *     | Input |                     Output                      |
  *     | *cmd  | return code |       *out        | *is_available |
  *     +-------+-------------+-------------------+---------------+
- *     | "foo" |    false    | NULL              |  (unchanged)  |
+ *     | "foo" |    false    | "foo" (allocated) |  (unchanged)  |
  *     +-------+-------------+-------------------+---------------+
  *
  *   GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”
@@ -1850,8 +1850,11 @@ static int get_schedule_cmd(const char *cmd, int *is_available, char **out)
 	struct string_list_item *item;
 	struct string_list list = STRING_LIST_INIT_NODUP;
 
-	if (!testing)
+	if (!testing) {
+		if (out)
+			*out = xstrdup(cmd);
 		return 0;
+	}
 
 	if (is_available)
 		*is_available = 0;
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index a66d0e089d..4008e4e45e 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '
 		maintenance.repo "$(pwd)/$META"
 '
 
+test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
+	test_when_finished "rm -rf crontab.log script repo" &&
+	mkdir script &&
+	write_script script/crontab <<-EOF &&
+	echo "\$*" >>"$(pwd)"/crontab.log
+	EOF
+	git init repo &&
+	(
+		cd repo &&
+		sane_unset GIT_TEST_MAINT_SCHEDULER &&
+		PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=crontab
+	) &&
+	test_grep -- -l crontab.log &&
+	test_grep -- git_cron_edit_tmp crontab.log
+'
+
 test_expect_success 'start --scheduler=<scheduler>' '
 	test_expect_code 129 git maintenance start --scheduler=foo 2>err &&
 	test_grep "unrecognized --scheduler argument" err &&
-- 
2.47.0.rc1.33.g90fe3800b9.dirty

Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`

[Derrick Stolee]

On 10/8/24 8:15 AM, Patrick Steinhardt wrote:
> It was reported on the mailing list that running `git maintenance start`
> immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix
> leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is
> trivial to reproduce up to a point where one is scratching their head
> why we didn't catch this regression in our test suite.

> +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
> +	test_when_finished "rm -rf crontab.log script repo" &&
> +	mkdir script &&
> +	write_script script/crontab <<-EOF &&
> +	echo "\$*" >>"$(pwd)"/crontab.log
> +	EOF
> +	git init repo &&
> +	(
> +		cd repo &&
> +		sane_unset GIT_TEST_MAINT_SCHEDULER &&
> +		PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=crontab
> +	) &&
> +	test_grep -- -l crontab.log &&
> +	test_grep -- git_cron_edit_tmp crontab.log
> +'
> +
I see why we didn't catch this immediately. This is a good way to work
around this issue of "mocking" the scheduler.

Thanks for the fast response.
-Stolee

Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> So why didn't we catch this trivial regression? The reason is that our
> tests always set up the "GIT_TEST_MAINT_SCHEDULER" environment variable
> via "t/test-lib.sh", which allows us to override the scheduler command
> with a custom one so that we don't accidentally modify the developer's
> system. But the faulty code where we don't set the `out` parameter will
> only get hit in case that environment variable is _not_ set, which is
> never the case when executing our tests.
>
> Fix the regression by again unconditionally allocating the value in the
> `out` parameter, if provided. Add a test that unsets the environment
> variable to catch future regressions in this area.
>
> Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

Thanks for quickly reporting and addressing this one, both of you.

Will queue.

Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`

[Derrick Stolee]

On 10/8/24 2:30 PM, Derrick Stolee wrote:
> On 10/8/24 8:15 AM, Patrick Steinhardt wrote:
>> It was reported on the mailing list that running `git maintenance start`
>> immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix
>> leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is
>> trivial to reproduce up to a point where one is scratching their head
>> why we didn't catch this regression in our test suite.
> 
>> +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
>> +    test_when_finished "rm -rf crontab.log script repo" &&
>> +    mkdir script &&
>> +    write_script script/crontab <<-EOF &&
>> +    echo "\$*" >>"$(pwd)"/crontab.log
>> +    EOF
>> +    git init repo &&
>> +    (
>> +        cd repo &&
>> +        sane_unset GIT_TEST_MAINT_SCHEDULER &&
>> +        PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=crontab
>> +    ) &&
>> +    test_grep -- -l crontab.log &&
>> +    test_grep -- git_cron_edit_tmp crontab.log
>> +'
>> +
> I see why we didn't catch this immediately. This is a good way to work
> around this issue of "mocking" the scheduler.

Unfortunately, this test is broken on macOS and Windows. Those platforms will
fail when asked for 'crontab' without the test variable.

Here is a potential fixup that will make your test succeed:

--- >8 ---

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4008e4e45e..86bc77e73f 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -646,7 +646,7 @@ test_expect_success !MINGW 'register and unregister with 
regex metacharacters' '
  		maintenance.repo "$(pwd)/$META"
  '

-test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
+test_expect_success !MINGW,!DARWIN 'start without GIT_TEST_MAINT_SCHEDULER' '
  	test_when_finished "rm -rf crontab.log script repo" &&
  	mkdir script &&
  	write_script script/crontab <<-EOF &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index b1a8ee5c00..f12f3a7609 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1715,6 +1715,12 @@ case $uname_s in
  	test_set_prereq GREP_STRIPS_CR
  	test_set_prereq WINDOWS
  	;;
+*Darwin*)
+	test_set_prereq POSIXPERM
+	test_set_prereq BSLASHPSPEC
+	test_set_prereq EXECKEEPSPID
+	test_set_prereq DARWIN
+	;;
  *)
  	test_set_prereq POSIXPERM
  	test_set_prereq BSLASHPSPEC

Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`

[Patrick Steinhardt]

On Tue, Oct 08, 2024 at 10:58:06PM -0400, Derrick Stolee wrote:
> On 10/8/24 2:30 PM, Derrick Stolee wrote:
> > On 10/8/24 8:15 AM, Patrick Steinhardt wrote:
> > > It was reported on the mailing list that running `git maintenance start`
> > > immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix
> > > leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is
> > > trivial to reproduce up to a point where one is scratching their head
> > > why we didn't catch this regression in our test suite.
> > 
> > > +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
> > > +    test_when_finished "rm -rf crontab.log script repo" &&
> > > +    mkdir script &&
> > > +    write_script script/crontab <<-EOF &&
> > > +    echo "\$*" >>"$(pwd)"/crontab.log
> > > +    EOF
> > > +    git init repo &&
> > > +    (
> > > +        cd repo &&
> > > +        sane_unset GIT_TEST_MAINT_SCHEDULER &&
> > > +        PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=crontab
> > > +    ) &&
> > > +    test_grep -- -l crontab.log &&
> > > +    test_grep -- git_cron_edit_tmp crontab.log
> > > +'
> > > +
> > I see why we didn't catch this immediately. This is a good way to work
> > around this issue of "mocking" the scheduler.
> 
> Unfortunately, this test is broken on macOS and Windows. Those platforms will
> fail when asked for 'crontab' without the test variable.
> 
> Here is a potential fixup that will make your test succeed:

Oh, indeed, thanks for flagging this. The issue is that those platforms
do not make `crontab` available at all.

I think we can land at a better fix though: the systemctl-based
scheduler _is_ available on all platforms if the systemctl binary is
found. So let me adapt the script accordingly.

Patrick

[PATCH v2] builtin/gc: fix crash when running `git maintenance start`

[Patrick Steinhardt]

It was reported on the mailing list that running `git maintenance start`
immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix
leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is
trivial to reproduce up to a point where one is scratching their head
why we didn't catch this regression in our test suite.

The root cause of this error is `get_schedule_cmd()`, which does not
populate the `out` parameter in all cases anymore starting with the
mentioned commit. Callers do assume it to always be populated though and
will e.g. call `strvec_split()` on the returned value, which will of
course segfault when the variable is uninitialized.

So why didn't we catch this trivial regression? The reason is that our
tests always set up the "GIT_TEST_MAINT_SCHEDULER" environment variable
via "t/test-lib.sh", which allows us to override the scheduler command
with a custom one so that we don't accidentally modify the developer's
system. But the faulty code where we don't set the `out` parameter will
only get hit in case that environment variable is _not_ set, which is
never the case when executing our tests.

Fix the regression by again unconditionally allocating the value in the
`out` parameter, if provided. Add a test that unsets the environment
variable to catch future regressions in this area.

Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

There's a single fix compared to v1, where Stolee mentioned that the
added test fails on both Windows and macOS because the "crontab"
scheduler isn't available there. I've adapted the test to instead use
the "systemd" scheduler, which _is_ available on all platforms when the
systemctl(1) binary can be found.

Thanks!

Patrick

 builtin/gc.c           |  7 +++++--
 t/t7900-maintenance.sh | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 6a7a2da006..d52735354c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1832,7 +1832,7 @@ static const char *get_extra_launchctl_strings(void) {
  *     | Input |                     Output                      |
  *     | *cmd  | return code |       *out        | *is_available |
  *     +-------+-------------+-------------------+---------------+
- *     | "foo" |    false    | NULL              |  (unchanged)  |
+ *     | "foo" |    false    | "foo" (allocated) |  (unchanged)  |
  *     +-------+-------------+-------------------+---------------+
  *
  *   GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”
@@ -1850,8 +1850,11 @@ static int get_schedule_cmd(const char *cmd, int *is_available, char **out)
 	struct string_list_item *item;
 	struct string_list list = STRING_LIST_INIT_NODUP;
 
-	if (!testing)
+	if (!testing) {
+		if (out)
+			*out = xstrdup(cmd);
 		return 0;
+	}
 
 	if (is_available)
 		*is_available = 0;
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index a66d0e089d..e75b485319 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '
 		maintenance.repo "$(pwd)/$META"
 '
 
+test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
+	test_when_finished "rm -rf systemctl.log script repo" &&
+	mkdir script &&
+	write_script script/systemctl <<-EOF &&
+	echo "\$*" >>"$(pwd)"/systemctl.log
+	EOF
+	git init repo &&
+	(
+		cd repo &&
+		sane_unset GIT_TEST_MAINT_SCHEDULER &&
+		PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=systemd
+	) &&
+	test_grep -- "--user list-timers" systemctl.log &&
+	test_grep -- "enable --now git-maintenance@" systemctl.log
+'
+
 test_expect_success 'start --scheduler=<scheduler>' '
 	test_expect_code 129 git maintenance start --scheduler=foo 2>err &&
 	test_grep "unrecognized --scheduler argument" err &&

Range-diff against v1:
1:  976c97081a ! 1:  5798c31e1e builtin/gc: fix crash when running `git maintenance start`
    @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'register and unregister with
      '
      
     +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
    -+	test_when_finished "rm -rf crontab.log script repo" &&
    ++	test_when_finished "rm -rf systemctl.log script repo" &&
     +	mkdir script &&
    -+	write_script script/crontab <<-EOF &&
    -+	echo "\$*" >>"$(pwd)"/crontab.log
    ++	write_script script/systemctl <<-EOF &&
    ++	echo "\$*" >>"$(pwd)"/systemctl.log
     +	EOF
     +	git init repo &&
     +	(
     +		cd repo &&
     +		sane_unset GIT_TEST_MAINT_SCHEDULER &&
    -+		PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=crontab
    ++		PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=systemd
     +	) &&
    -+	test_grep -- -l crontab.log &&
    -+	test_grep -- git_cron_edit_tmp crontab.log
    ++	test_grep -- "--user list-timers" systemctl.log &&
    ++	test_grep -- "enable --now git-maintenance@" systemctl.log
     +'
     +
      test_expect_success 'start --scheduler=<scheduler>' '
-- 
2.47.0.rc1.33.g90fe3800b9.dirty

Re: [PATCH v2] builtin/gc: fix crash when running `git maintenance start`

[Derrick Stolee]

On 10/9/24 3:08 AM, Patrick Steinhardt wrote:

> There's a single fix compared to v1, where Stolee mentioned that the
> added test fails on both Windows and macOS because the "crontab"
> scheduler isn't available there. I've adapted the test to instead use
> the "systemd" scheduler, which _is_ available on all platforms when the
> systemctl(1) binary can be found.

I do appreciate that you are making the test work on all platforms
instead of my workaround to avoid the test on mac and Windows.

I was initially confused by your description saying that systemctl is
available on these platforms, because it isn't for my machines. But
a way to rephrase what you mean is "Git has compile-time expectations
that crontab doesn't exist for macOS and Windows, but Git checks for
'systemctl' on the PATH regardless of compiled platform."

Thus, placing a systemctl script works for these platforms even when
they don't have systemctl available normally.

I'm happy with this version.

Thanks,
-Stolee

Re: [PATCH v2] builtin/gc: fix crash when running `git maintenance start`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index a66d0e089d..e75b485319 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '
>  		maintenance.repo "$(pwd)/$META"
>  '
>  
> +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
> +	test_when_finished "rm -rf systemctl.log script repo" &&
> +	mkdir script &&
> +	write_script script/systemctl <<-EOF &&
> +	echo "\$*" >>"$(pwd)"/systemctl.log
> +	EOF

We deliberately include one SP in the name of the trash directory
when we create it under t/ so that we can catch a bug easily when we
forget quoting "$TRASH_DIRECTORY", but otherwise the name under t/
is safe.  

But this use of "$(pwd)" is a bit iffy.  It is expanded when the
script is written, which means the script is designed to write to
the systemctl.log file in a directory whose absolute path is known
and hardcoded in the script.  We assume that the leading directories
down to the t/ directory from the root can also safely quoted by
simply enclosing inside "a pair of double quotes", which might be
iffy (obviously if $(pwd) had a double-quote in it, the quoting
would break).

I'll let it pass (and will forget blissfully if nobody screams).

Other than that, looking good.  It certainly is good that there is
something more widely supported than "cron" so that we can write
this test as a generic one, even if some of us might not want to
touch "systemd" in the real life ;-)

Thanks, will queue.

Re: [PATCH v2] builtin/gc: fix crash when running `git maintenance start`

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> ... But
> a way to rephrase what you mean is "Git has compile-time expectations
> that crontab doesn't exist for macOS and Windows, but Git checks for
> 'systemctl' on the PATH regardless of compiled platform."
>
> Thus, placing a systemctl script works for these platforms even when
> they don't have systemctl available normally.

Very nice rewrite.  Thanks for pointing out what your initial
reading hiccupped, as it is likely the same would happen to others.

Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> Fix the regression by again unconditionally allocating the value in the
>> `out` parameter, if provided. Add a test that unsets the environment
>> variable to catch future regressions in this area.
>>
>> Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>
>> Signed-off-by: Patrick Steinhardt <ps@pks.im>
>> ---
>
> Thanks for quickly reporting and addressing this one, both of you.
>
> Will queue.

We seem to be getting:

  git maintenance start --scheduler=systemd
  D:/a/git/git/git.exe: error while loading shared libraries: ?: cannot open shared object file: No such file or directory

https://github.com/git/git/actions/runs/11264159690/job/31323795058#step:5:327

Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`

[Patrick Steinhardt]

On Wed, Oct 09, 2024 at 04:14:38PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >> Fix the regression by again unconditionally allocating the value in the
> >> `out` parameter, if provided. Add a test that unsets the environment
> >> variable to catch future regressions in this area.
> >>
> >> Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>
> >> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> >> ---
> >
> > Thanks for quickly reporting and addressing this one, both of you.
> >
> > Will queue.
> 
> We seem to be getting:
> 
>   git maintenance start --scheduler=systemd
>   D:/a/git/git/git.exe: error while loading shared libraries: ?: cannot open shared object file: No such file or directory
> 
> https://github.com/git/git/actions/runs/11264159690/job/31323795058#step:5:327

Oh well. Lucky me that I'm spending half of my life in Windows VMs now
anyway, so I'll have a look.

Patrick

Re: [PATCH] builtin/gc: fix crash when running `git maintenance start`

[Patrick Steinhardt]

On Thu, Oct 10, 2024 at 06:54:53AM +0200, Patrick Steinhardt wrote:
> On Wed, Oct 09, 2024 at 04:14:38PM -0700, Junio C Hamano wrote:
> > Junio C Hamano <gitster@pobox.com> writes:
> > 
> > > Patrick Steinhardt <ps@pks.im> writes:
> > >
> > >> Fix the regression by again unconditionally allocating the value in the
> > >> `out` parameter, if provided. Add a test that unsets the environment
> > >> variable to catch future regressions in this area.
> > >>
> > >> Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>
> > >> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > >> ---
> > >
> > > Thanks for quickly reporting and addressing this one, both of you.
> > >
> > > Will queue.
> > 
> > We seem to be getting:
> > 
> >   git maintenance start --scheduler=systemd
> >   D:/a/git/git/git.exe: error while loading shared libraries: ?: cannot open shared object file: No such file or directory
> > 
> > https://github.com/git/git/actions/runs/11264159690/job/31323795058#step:5:327
> 
> Oh well. Lucky me that I'm spending half of my life in Windows VMs now
> anyway, so I'll have a look.
> 
> Patrick

Ah, the difference between `pwd` and `$PWD` strikes again: the former
uses Windows-style paths, the latter uses Cygwin-style ones. And as the
Windows-style path contains a colon the resulting PATH variable was
broken.

Will send v3 in a bit, thanks!

Patrick

[PATCH v3] builtin/gc: fix crash when running `git maintenance start`

[Patrick Steinhardt]

It was reported on the mailing list that running `git maintenance start`
immediately segfaults starting with b6c3f8e12c (builtin/maintenance: fix
leak in `get_schedule_cmd()`, 2024-09-26). And indeed, this segfault is
trivial to reproduce up to a point where one is scratching their head
why we didn't catch this regression in our test suite.

The root cause of this error is `get_schedule_cmd()`, which does not
populate the `out` parameter in all cases anymore starting with the
mentioned commit. Callers do assume it to always be populated though and
will e.g. call `strvec_split()` on the returned value, which will of
course segfault when the variable is uninitialized.

So why didn't we catch this trivial regression? The reason is that our
tests always set up the "GIT_TEST_MAINT_SCHEDULER" environment variable
via "t/test-lib.sh", which allows us to override the scheduler command
with a custom one so that we don't accidentally modify the developer's
system. But the faulty code where we don't set the `out` parameter will
only get hit in case that environment variable is _not_ set, which is
never the case when executing our tests.

Fix the regression by again unconditionally allocating the value in the
`out` parameter, if provided. Add a test that unsets the environment
variable to catch future regressions in this area.

Reported-by: Shubham Kanodia <shubham.kanodia10@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

Two changes compared to v2:

  - We do not expand "$pwd" in the HERE doc anymore.

  - Fixed "$(pwd)" vs "$PWD", which broke Windows builds due to a
    misformatted PATH.

Thanks!

Patrick

 builtin/gc.c           |  7 +++++--
 t/t7900-maintenance.sh | 16 ++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 6a7a2da006..d52735354c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1832,7 +1832,7 @@ static const char *get_extra_launchctl_strings(void) {
  *     | Input |                     Output                      |
  *     | *cmd  | return code |       *out        | *is_available |
  *     +-------+-------------+-------------------+---------------+
- *     | "foo" |    false    | NULL              |  (unchanged)  |
+ *     | "foo" |    false    | "foo" (allocated) |  (unchanged)  |
  *     +-------+-------------+-------------------+---------------+
  *
  *   GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”
@@ -1850,8 +1850,11 @@ static int get_schedule_cmd(const char *cmd, int *is_available, char **out)
 	struct string_list_item *item;
 	struct string_list list = STRING_LIST_INIT_NODUP;
 
-	if (!testing)
+	if (!testing) {
+		if (out)
+			*out = xstrdup(cmd);
 		return 0;
+	}
 
 	if (is_available)
 		*is_available = 0;
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index a66d0e089d..c224c8450c 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '
 		maintenance.repo "$(pwd)/$META"
 '
 
+test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
+	test_when_finished "rm -rf systemctl.log script repo" &&
+	mkdir script &&
+	write_script script/systemctl <<-\EOF &&
+	echo "$*" >>../systemctl.log
+	EOF
+	git init repo &&
+	(
+		cd repo &&
+		sane_unset GIT_TEST_MAINT_SCHEDULER &&
+		PATH="$PWD/../script:$PATH" git maintenance start --scheduler=systemd
+	) &&
+	test_grep -- "--user list-timers" systemctl.log &&
+	test_grep -- "enable --now git-maintenance@" systemctl.log
+'
+
 test_expect_success 'start --scheduler=<scheduler>' '
 	test_expect_code 129 git maintenance start --scheduler=foo 2>err &&
 	test_grep "unrecognized --scheduler argument" err &&

Range-diff against v2:
1:  5798c31e1e ! 1:  a5b1433abf builtin/gc: fix crash when running `git maintenance start`
    @@ t/t7900-maintenance.sh: test_expect_success !MINGW 'register and unregister with
     +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '
     +	test_when_finished "rm -rf systemctl.log script repo" &&
     +	mkdir script &&
    -+	write_script script/systemctl <<-EOF &&
    -+	echo "\$*" >>"$(pwd)"/systemctl.log
    ++	write_script script/systemctl <<-\EOF &&
    ++	echo "$*" >>../systemctl.log
     +	EOF
     +	git init repo &&
     +	(
     +		cd repo &&
     +		sane_unset GIT_TEST_MAINT_SCHEDULER &&
    -+		PATH="$(pwd)/../script:$PATH" git maintenance start --scheduler=systemd
    ++		PATH="$PWD/../script:$PATH" git maintenance start --scheduler=systemd
     +	) &&
     +	test_grep -- "--user list-timers" systemctl.log &&
     +	test_grep -- "enable --now git-maintenance@" systemctl.log
-- 
2.47.0.dirty

Re: [PATCH v3] builtin/gc: fix crash when running `git maintenance start`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> +	write_script script/systemctl <<-\EOF &&
> +	echo "$*" >>../systemctl.log
> +	EOF

Ah, for the purpose of this test, we _know_ in which directory the
"systemctl" will be spawned, so this is good enough for us, of
course.

> +	git init repo &&
> +	(
> +		cd repo &&
> +		sane_unset GIT_TEST_MAINT_SCHEDULER &&
> +		PATH="$PWD/../script:$PATH" git maintenance start --scheduler=systemd

I suspect we can use the same idea and add a relative path in $PATH
for the test, perhaps, even though it is not a good coding
discipline.  If $PWD, instead of $(pwd), works, it is perfectly OK.

Will queue.  Thanks.

Re: [PATCH v3] builtin/gc: fix crash when running `git maintenance start`

[Shubham Kanodia]

On Thu, Oct 10, 2024 at 10:39 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > +     write_script script/systemctl <<-\EOF &&
> > +     echo "$*" >>../systemctl.log
> > +     EOF
>
> Ah, for the purpose of this test, we _know_ in which directory the
> "systemctl" will be spawned, so this is good enough for us, of
> course.
>
> > +     git init repo &&
> > +     (
> > +             cd repo &&
> > +             sane_unset GIT_TEST_MAINT_SCHEDULER &&
> > +             PATH="$PWD/../script:$PATH" git maintenance start --scheduler=systemd
>
> I suspect we can use the same idea and add a relative path in $PATH
> for the test, perhaps, even though it is not a good coding
> discipline.  If $PWD, instead of $(pwd), works, it is perfectly OK.
>
> Will queue.  Thanks.

Appreciate for the quick fix, Patrick.

Homebrew upgraded their formulas to 2.47 rather quickly (the next day
after release) —
https://github.com/Homebrew/homebrew-core/commit/0435f258701abd3acb9e2f4cd758cc13aa93997c

Mac users who do a `brew install git` would now install versions with
a broken maintenance command.
Fortunately, `brew` auto-updates the world every time a user installs
anything so it's likely they get to a 2.47.1 in the future,
but that still might be a while away from when they install the
current latest (2.47.0).

I'm not sure if Git has a hotfix workflow, but it might make sense to
prevent more users from getting onto the buggy version
(especially since repo admins usually set up maintenance in the
background and the error might not be evident to users).

Re: [PATCH v3] builtin/gc: fix crash when running `git maintenance start`

[Patrick Steinhardt]

On Mon, Oct 14, 2024 at 10:14:53AM +0530, Shubham Kanodia wrote:
> On Thu, Oct 10, 2024 at 10:39 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > +     write_script script/systemctl <<-\EOF &&
> > > +     echo "$*" >>../systemctl.log
> > > +     EOF
> >
> > Ah, for the purpose of this test, we _know_ in which directory the
> > "systemctl" will be spawned, so this is good enough for us, of
> > course.
> >
> > > +     git init repo &&
> > > +     (
> > > +             cd repo &&
> > > +             sane_unset GIT_TEST_MAINT_SCHEDULER &&
> > > +             PATH="$PWD/../script:$PATH" git maintenance start --scheduler=systemd
> >
> > I suspect we can use the same idea and add a relative path in $PATH
> > for the test, perhaps, even though it is not a good coding
> > discipline.  If $PWD, instead of $(pwd), works, it is perfectly OK.
> >
> > Will queue.  Thanks.
> 
> Appreciate for the quick fix, Patrick.
> 
> Homebrew upgraded their formulas to 2.47 rather quickly (the next day
> after release) —
> https://github.com/Homebrew/homebrew-core/commit/0435f258701abd3acb9e2f4cd758cc13aa93997c
> 
> Mac users who do a `brew install git` would now install versions with
> a broken maintenance command.
> Fortunately, `brew` auto-updates the world every time a user installs
> anything so it's likely they get to a 2.47.1 in the future,
> but that still might be a while away from when they install the
> current latest (2.47.0).
> 
> I'm not sure if Git has a hotfix workflow, but it might make sense to
> prevent more users from getting onto the buggy version
> (especially since repo admins usually set up maintenance in the
> background and the error might not be evident to users).

I'm not sure around the timeline for Git v2.47.1, and Junio is going to
be out of office for two weeks, so it may take a while. I'd recommend to
backport the patch for now.

Patrick

Re: [PATCH v3] builtin/gc: fix crash when running `git maintenance start`

[Taylor Blau]

On Mon, Oct 14, 2024 at 10:38:43AM +0200, Patrick Steinhardt wrote:
> I'm not sure around the timeline for Git v2.47.1, and Junio is going to
> be out of office for two weeks, so it may take a while. I'd recommend to
> backport the patch for now.

I'm not planning on cutting any release while Junio is gone, so I'd
expect that a hypothetical 2.47.1 release wouldn't occur until the
beginning of November at the earliest.

Thanks,
Taylor