| Message ID | a5b1433abfd84cb627efc17f52e0d644ee207bb0.1728538282.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c95547a394a35dc26afa686454086d2db6e51ea4 |
| Headers | show |
| Series | [v3] builtin/gc: fix crash when running `git maintenance start` | expand |
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.
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).
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
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
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 &&
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(-) 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