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

builtin/maintenance: fix leak in `get_schedule_cmd()` 

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>
maint
Patrick Steinhardt 2024-09-26 13:47:02 +02:00 committed by Junio C Hamano
parent 84e9fc361d
commit b6c3f8e12c
2 changed files with 81 additions and 47 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

129
builtin/gc.c
View File

@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)
* * If $GIT_TEST_MAINT_SCHEDULER is set, return true. * * If $GIT_TEST_MAINT_SCHEDULER is set, return true.
* In this case, the *cmd value is read as input. * 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 * * 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 * item, then *is_available is set to true and *out is set to
* the mock command. * the mock command.
* *
* * if the input value *cmd isn’t the key of any of the comma-separated list * * 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.: * Ex.:
* GIT_TEST_MAINT_SCHEDULER not set * GIT_TEST_MAINT_SCHEDULER not set
* +-------+-------------------------------------------------+ * +-------+-------------------------------------------------+
* | Input | Output | * | 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” * GIT_TEST_MAINT_SCHEDULER set to “foo:./mock_foo.sh,bar:./mock_bar.sh”
* +-------+-------------------------------------------------+ * +-------+-------------------------------------------------+
* | Input | Output | * | Input | Output |
* | *cmd | return code | *cmd | *is_available | * | *cmd | return code | *out | *is_available |
* +-------+-------------+-------------------+---------------+ * +-------+-------------+-------------------+---------------+
* | "foo" | true | "./mock.foo.sh" | true | * | "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")); char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER"));
struct string_list_item *item; struct string_list_item *item;
@ -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) if (string_list_split_in_place(&pair, item->string, ":", 2) != 2)
continue; continue;


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


string_list_clear(&pair, 0);
}

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

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


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


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


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


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


result = finish_command(&child); result = finish_command(&child);


free(cmd);
free(uid); free(uid);
return result; return result;
} }
@ -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; static unsigned long lock_file_timeout_ms = ULONG_MAX;
struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT; struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;
struct stat st; struct stat st;
const char *cmd = "launchctl"; char *cmd;
int minute = get_random_minute(); int minute = get_random_minute();


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


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


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


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


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


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


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


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


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


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


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


@ -2276,21 +2285,28 @@ static int check_crontab_process(const char *cmd)


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


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


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

out:
free(cmd);
return ret;
} }


#define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE" #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"
@ -2298,7 +2314,7 @@ static int is_crontab_available(void)


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


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


if (start_command(&crontab_list)) if (start_command(&crontab_list)) {
return error(_("failed to run 'crontab -l'; your system might not support 'cron'")); 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. */ /* Ignore exit code, as an empty crontab will return error. */
finish_command(&crontab_list); finish_command(&crontab_list);
@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd)
result = error(_("'crontab' died")); result = error(_("'crontab' died"));
else else
fclose(cron_list); fclose(cron_list);

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


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


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


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


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


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


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


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

if (finish_command(&child)) {
/* /*
* Disabling an already disabled systemd unit makes * Disabling an already disabled systemd unit makes
* systemctl fail. * systemctl fail.
@ -2628,9 +2653,17 @@ static int systemd_timer_enable_unit(int enable,
* *
* Enabling an enabled systemd unit doesn't fail. * Enabling an enabled systemd unit doesn't fail.
*/ */
if (enable) if (enable) {
return error(_("failed to run systemctl")); ret = error(_("failed to run systemctl"));
return 0; goto out;
}
}

ret = 0;

out:
free(cmd);
return ret;
} }


/* /*

1
t/t7900-maintenance.sh
View File

@ -2,6 +2,7 @@


test_description='git maintenance builtin' test_description='git maintenance builtin'


TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh . ./test-lib.sh


GIT_TEST_COMMIT_GRAPH=0 GIT_TEST_COMMIT_GRAPH=0