Skip to content

Commit

Permalink
builtin/maintenance: fix leak in get_schedule_cmd()
Browse files Browse the repository at this point in the history 
The `get_schedule_cmd()` function allows us to override the schedule
command with a specific test command such that we can verify the
underlying logic in a platform-independent way. Its memory management is
somewhat wild though, because it basically gives up and assigns an
allocated string to the string constant output pointer. While this part
is marked with `UNLEAK()` to mask this, we also leak the local string
lists.

Rework the function such that it has a separate out parameter. If set,
we will assign it the final allocated command. Plug the other memory
leaks and create a common exit path.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
@pks-t @gitster
pks-t authored and gitster committed Sep 27, 2024
1 parent 84e9fc3 commit b6c3f8e
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 47 deletions.
127 changes: 80 additions & 47 deletions builtin/gc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)
* * If $GIT_TEST_MAINT_SCHEDULER is set, return true.
* In this case, the *cmd value is read as input.
*
* * if the input value *cmd is the key of one of the comma-separated list
* item, then *is_available is set to true and *cmd is modified and becomes
* * if the input value cmd is the key of one of the comma-separated list
* item, then *is_available is set to true and *out is set to
* the mock command.
*
* * if the input value *cmd isn’t the key of any of the comma-separated list
* item, then *is_available is set to false.
* item, then *is_available is set to false and *out is set to the original
* command.
*
* Ex.:
* GIT_TEST_MAINT_SCHEDULER not set
* +-------+-------------------------------------------------+
* | Input | Output |
* | *cmd | return code | *cmd | *is_available |
* | *cmd | return code | *out | *is_available |
* +-------+-------------+-------------------+---------------+
* | "foo" | false | "foo" (unchanged) | (unchanged) |
* | "foo" | false | NULL | (unchanged) |
* +-------+-------------+-------------------+---------------+
*
* GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”
* +-------+-------------------------------------------------+
* | Input | Output |
* | *cmd | return code | *cmd | *is_available |
* | *cmd | return code | *out | *is_available |
* +-------+-------------+-------------------+---------------+
* | "foo" | true | "./mock.foo.sh" | true |
* | "qux" | true | "qux" (unchanged) | false |
* | "qux" | true | "qux" (allocated) | false |
* +-------+-------------+-------------------+---------------+
*/
static int get_schedule_cmd(const char **cmd, int *is_available)
static int get_schedule_cmd(const char *cmd, int *is_available, char **out)
{
char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER"));
struct string_list_item *item;
Expand All @@ -1824,16 +1825,22 @@ static int get_schedule_cmd(const char **cmd, int *is_available)
if (string_list_split_in_place(&pair, item->string, ":", 2) != 2)
continue;

if (!strcmp(*cmd, pair.items[0].string)) {
*cmd = pair.items[1].string;
if (!strcmp(cmd, pair.items[0].string)) {
if (out)
*out = xstrdup(pair.items[1].string);
if (is_available)
*is_available = 1;
string_list_clear(&list, 0);
UNLEAK(testing);
return 1;
string_list_clear(&pair, 0);
goto out;
}

string_list_clear(&pair, 0);
}

if (out)
*out = xstrdup(cmd);

out:
string_list_clear(&list, 0);
free(testing);
return 1;
Expand All @@ -1850,9 +1857,8 @@ static int get_random_minute(void)

static int is_launchctl_available(void)
{
const char *cmd = "launchctl";
int is_available;
if (get_schedule_cmd(&cmd, &is_available))
if (get_schedule_cmd("launchctl", &is_available, NULL))
return is_available;

#ifdef __APPLE__
Expand Down Expand Up @@ -1890,12 +1896,12 @@ static char *launchctl_get_uid(void)

static int launchctl_boot_plist(int enable, const char *filename)
{
const char *cmd = "launchctl";
char *cmd;
int result;
struct child_process child = CHILD_PROCESS_INIT;
char *uid = launchctl_get_uid();

get_schedule_cmd(&cmd, NULL);
get_schedule_cmd("launchctl", NULL, &cmd);
strvec_split(&child.args, cmd);
strvec_pushl(&child.args, enable ? "bootstrap" : "bootout", uid,
filename, NULL);
Expand All @@ -1908,6 +1914,7 @@ static int launchctl_boot_plist(int enable, const char *filename)

result = finish_command(&child);

free(cmd);
free(uid);
return result;
}
Expand Down Expand Up @@ -1959,10 +1966,10 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
static unsigned long lock_file_timeout_ms = ULONG_MAX;
struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;
struct stat st;
const char *cmd = "launchctl";
char *cmd;
int minute = get_random_minute();

get_schedule_cmd(&cmd, NULL);
get_schedule_cmd("launchctl", NULL, &cmd);
preamble = "<?xml version=\"1.0\"?>\n"
"<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"
"<plist version=\"1.0\">"
Expand Down Expand Up @@ -2052,6 +2059,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit

free(filename);
free(name);
free(cmd);
strbuf_release(&plist);
strbuf_release(&plist2);
return 0;
Expand All @@ -2076,9 +2084,8 @@ static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)

static int is_schtasks_available(void)
{
const char *cmd = "schtasks";
int is_available;
if (get_schedule_cmd(&cmd, &is_available))
if (get_schedule_cmd("schtasks", &is_available, NULL))
return is_available;

#ifdef GIT_WINDOWS_NATIVE
Expand All @@ -2097,15 +2104,16 @@ static char *schtasks_task_name(const char *frequency)

static int schtasks_remove_task(enum schedule_priority schedule)
{
const char *cmd = "schtasks";
char *cmd;
struct child_process child = CHILD_PROCESS_INIT;
const char *frequency = get_frequency(schedule);
char *name = schtasks_task_name(frequency);

get_schedule_cmd(&cmd, NULL);
get_schedule_cmd("schtasks", NULL, &cmd);
strvec_split(&child.args, cmd);
strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL);
free(name);
free(cmd);

return run_command(&child);
}
Expand All @@ -2119,7 +2127,7 @@ static int schtasks_remove_tasks(void)

static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule)
{
const char *cmd = "schtasks";
char *cmd;
int result;
struct child_process child = CHILD_PROCESS_INIT;
const char *xml;
Expand All @@ -2129,7 +2137,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
struct strbuf tfilename = STRBUF_INIT;
int minute = get_random_minute();

get_schedule_cmd(&cmd, NULL);
get_schedule_cmd("schtasks", NULL, &cmd);

strbuf_addf(&tfilename, "%s/schedule_%s_XXXXXX",
get_git_common_dir(), frequency);
Expand Down Expand Up @@ -2235,6 +2243,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority

delete_tempfile(&tfile);
free(name);
free(cmd);
return result;
}

Expand Down Expand Up @@ -2276,29 +2285,36 @@ static int check_crontab_process(const char *cmd)

static int is_crontab_available(void)
{
const char *cmd = "crontab";
char *cmd;
int is_available;
int ret;

if (get_schedule_cmd(&cmd, &is_available))
return is_available;
if (get_schedule_cmd("crontab", &is_available, &cmd)) {
ret = is_available;
goto out;
}

#ifdef __APPLE__
/*
* macOS has cron, but it requires special permissions and will
* create a UI alert when attempting to run this command.
*/
return 0;
ret = 0;
#else
return check_crontab_process(cmd);
ret = check_crontab_process(cmd);
#endif

out:
free(cmd);
return ret;
}

#define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
#define END_LINE "# END GIT MAINTENANCE SCHEDULE"

static int crontab_update_schedule(int run_maintenance, int fd)
{
const char *cmd = "crontab";
char *cmd;
int result = 0;
int in_old_region = 0;
struct child_process crontab_list = CHILD_PROCESS_INIT;
Expand All @@ -2308,15 +2324,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)
struct tempfile *tmpedit = NULL;
int minute = get_random_minute();

get_schedule_cmd(&cmd, NULL);
get_schedule_cmd("crontab", NULL, &cmd);
strvec_split(&crontab_list.args, cmd);
strvec_push(&crontab_list.args, "-l");
crontab_list.in = -1;
crontab_list.out = dup(fd);
crontab_list.git_cmd = 0;

if (start_command(&crontab_list))
return error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
if (start_command(&crontab_list)) {
result = error(_("failed to run 'crontab -l'; your system might not support 'cron'"));
goto out;
}

/* Ignore exit code, as an empty crontab will return error. */
finish_command(&crontab_list);
Expand Down Expand Up @@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd)
result = error(_("'crontab' died"));
else
fclose(cron_list);

out:
delete_tempfile(&tmpedit);
free(cmd);
return result;
}

Expand All @@ -2410,10 +2430,9 @@ static int real_is_systemd_timer_available(void)

static int is_systemd_timer_available(void)
{
const char *cmd = "systemctl";
int is_available;

if (get_schedule_cmd(&cmd, &is_available))
if (get_schedule_cmd("systemctl", &is_available, NULL))
return is_available;

return real_is_systemd_timer_available();
Expand Down Expand Up @@ -2594,9 +2613,10 @@ static int systemd_timer_enable_unit(int enable,
enum schedule_priority schedule,
int minute)
{
const char *cmd = "systemctl";
char *cmd = NULL;
struct child_process child = CHILD_PROCESS_INIT;
const char *frequency = get_frequency(schedule);
int ret;

/*
* Disabling the systemd unit while it is already disabled makes
Expand All @@ -2607,30 +2627,43 @@ static int systemd_timer_enable_unit(int enable,
* On the other hand, enabling a systemd unit which is already enabled
* produces no error.
*/
if (!enable)
if (!enable) {
child.no_stderr = 1;
else if (systemd_timer_write_timer_file(schedule, minute))
return -1;
} else if (systemd_timer_write_timer_file(schedule, minute)) {
ret = -1;
goto out;
}

get_schedule_cmd(&cmd, NULL);
get_schedule_cmd("systemctl", NULL, &cmd);
strvec_split(&child.args, cmd);
strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",
"--now", NULL);
strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer");

if (start_command(&child))
return error(_("failed to start systemctl"));
if (finish_command(&child))
if (start_command(&child)) {
ret = error(_("failed to start systemctl"));
goto out;
}

if (finish_command(&child)) {
/*
* Disabling an already disabled systemd unit makes
* systemctl fail.
* Let's ignore this failure.
*
* Enabling an enabled systemd unit doesn't fail.
*/
if (enable)
return error(_("failed to run systemctl"));
return 0;
if (enable) {
ret = error(_("failed to run systemctl"));
goto out;
}
}

ret = 0;

out:
free(cmd);
return ret;
}

/*
Expand Down
1 change: 1 addition & 0 deletions t/t7900-maintenance.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

test_description='git maintenance builtin'

TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh

GIT_TEST_COMMIT_GRAPH=0
Expand Down

0 comments on commit b6c3f8e

Please sign in to comment.