| Message ID | 976c97081af7c62960bd71d1b70039657e7cb711.1728389731.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/gc: fix crash when running `git maintenance start` | expand |
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
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.
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
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
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
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
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
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 &&
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(-)