Skip to content

Commit

Permalink
builtin/gc: fix crash when running git maintenance start (#5198)
Browse files Browse the repository at this point in the history
> This patch was sent upstream by Patrick. I'm contributing it to Git
for Windows quickly to make sure it gets into microsoft/git, but also in
advance of any potential 2.47.1.

It was reported on the mailing list that running `git maintenance start`
immediately segfaults starting with b6c3f8e (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.
  • Loading branch information
dscho committed Oct 9, 2024
2 parents b8a3eb4 + 59beef8 commit 9cea034
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 1 deletion.
2 changes: 1 addition & 1 deletion t/t7900-maintenance.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down
6 changes: 6 additions & 0 deletions t/test-lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -1738,6 +1738,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
Expand Down

0 comments on commit 9cea034

Please sign in to comment.