@matheusbsbs / matheus-bagliano / commits / c95547a394

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

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Oct 10, 2024 at 07:33 UTC c95547a394a35dc26afa686454086d2db6e51ea4

2 files changed +21 -2

builtin/gc.c

+5 -2

@@ -1794,7 +1794,7 @@ static const char *get_frequency(enum schedule_priority schedule)

1794 * | Input | Output |

1795 * | *cmd | return code | *out | *is_available |

1796 * +-------+-------------+-------------------+---------------+

1798 * +-------+-------------+-------------------+---------------+

1799 *

1800 * GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”

@@ -1812,8 +1812,11 @@ static int get_schedule_cmd(const char *cmd, int *is_available, char **out)

1812 struct string_list_item *item;

1813 struct string_list list = STRING_LIST_INIT_NODUP;

1814

1815 - if (!testing)

1815 + if (!testing) {

1816 + if (out)

1817 + *out = xstrdup(cmd);

1818 return 0;

1819 + }

1820

1821 if (is_available)

1822 *is_available = 0;

t/t7900-maintenance.sh

+16

@@ -646,6 +646,22 @@ test_expect_success !MINGW 'register and unregister with regex metacharacters' '

646 maintenance.repo "$(pwd)/$META"

647 '

648

649 +test_expect_success 'start without GIT_TEST_MAINT_SCHEDULER' '

650 + test_when_finished "rm -rf systemctl.log script repo" &&

651 + mkdir script &&

652 + write_script script/systemctl <<-\EOF &&

653 + echo "$*" >>../systemctl.log

654 + EOF

655 + git init repo &&

656 + (

657 + cd repo &&

658 + sane_unset GIT_TEST_MAINT_SCHEDULER &&

659 + PATH="$PWD/../script:$PATH" git maintenance start --scheduler=systemd

660 + ) &&

661 + test_grep -- "--user list-timers" systemctl.log &&

662 + test_grep -- "enable --now git-maintenance@" systemctl.log

663 +'

664 +

665 test_expect_success 'start --scheduler=<scheduler>' '

666 test_expect_code 129 git maintenance start --scheduler=foo 2>err &&

667 test_grep "unrecognized --scheduler argument" err &&