| Message ID | 5798c31e1ef9346e7faf73f8c80b32c436937a8a.1728455715.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] builtin/gc: fix crash when running `git maintenance start` | expand |
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
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.
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.
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 &&
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(-) 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>' '