kernel
/
git
mirror of https://git.kernel.org/pub/scm/git/git.git
1
0
Fork 0
Code Releases Activity

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>
maint
Patrick Steinhardt 2024-10-10 07:33:01 +02:00 committed by Junio C Hamano
parent 12dfc2475c
commit c95547a394
2 changed files with 21 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
builtin/gc.c
View File

@ -1794,7 +1794,7 @@ static const char *get_frequency(enum schedule_priority schedule)
* | 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”
@ -1812,8 +1812,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;

16
t/t7900-maintenance.sh
View File

@ -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 &&