@matheusbsbs / matheus-bagliano / commits / b6c3f8e12c

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>

Patrick Steinhardt committed Sep 26, 2024 at 13:47 UTC b6c3f8e12c0a521450923ddbcf7a19a81aa3c4e7

2 files changed +81 -47

builtin/gc.c

+80 -47

@@ -1780,32 +1780,33 @@ static const char *get_frequency(enum schedule_priority schedule)

1780 * * If $GIT_TEST_MAINT_SCHEDULER is set, return true.

1781 * In this case, the *cmd value is read as input.

1782 *

1783 - * * if the input value *cmd is the key of one of the comma-separated list

1784 - * item, then *is_available is set to true and *cmd is modified and becomes

1783 + * * if the input value cmd is the key of one of the comma-separated list

1784 + * item, then *is_available is set to true and *out is set to

1785 * the mock command.

1786 *

1787 * * if the input value *cmd isn’t the key of any of the comma-separated list

1788 - * item, then *is_available is set to false.

1788 + * item, then *is_available is set to false and *out is set to the original

1789 + * command.

1790 *

1791 * Ex.:

1792 * GIT_TEST_MAINT_SCHEDULER not set

1793 * +-------+-------------------------------------------------+

1794 * | Input | Output |

1794 - * | *cmd | return code | *cmd | *is_available |

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”

1801 * +-------+-------------------------------------------------+

1802 * | Input | Output |

1802 - * | *cmd | return code | *cmd | *is_available |

1803 + * | *cmd | return code | *out | *is_available |

1804 * +-------+-------------+-------------------+---------------+

1807 * +-------+-------------+-------------------+---------------+

1808 */

1808 -static int get_schedule_cmd(const char **cmd, int *is_available)

1809 +static int get_schedule_cmd(const char *cmd, int *is_available, char **out)

1810 {

1811 char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER"));

1812 struct string_list_item *item;

@@ -1824,16 +1825,22 @@ static int get_schedule_cmd(const char **cmd, int *is_available)

1825 if (string_list_split_in_place(&pair, item->string, ":", 2) != 2)

1826 continue;

1827

1827 - if (!strcmp(*cmd, pair.items[0].string)) {

1828 - *cmd = pair.items[1].string;

1828 + if (!strcmp(cmd, pair.items[0].string)) {

1829 + if (out)

1830 + *out = xstrdup(pair.items[1].string);

1831 if (is_available)

1832 *is_available = 1;

1831 - string_list_clear(&list, 0);

1832 - UNLEAK(testing);

1833 - return 1;

1833 + string_list_clear(&pair, 0);

1834 + goto out;

1835 }

1836 +

1837 + string_list_clear(&pair, 0);

1838 }

1839

1840 + if (out)

1841 + *out = xstrdup(cmd);

1842 +

1843 +out:

1844 string_list_clear(&list, 0);

1845 free(testing);

1846 return 1;

@@ -1850,9 +1857,8 @@ static int get_random_minute(void)

1857

1858 static int is_launchctl_available(void)

1859 {

1853 - const char *cmd = "launchctl";

1860 int is_available;

1855 - if (get_schedule_cmd(&cmd, &is_available))

1861 + if (get_schedule_cmd("launchctl", &is_available, NULL))

1862 return is_available;

1863

1864 #ifdef __APPLE__

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

1896

1897 static int launchctl_boot_plist(int enable, const char *filename)

1898 {

1893 - const char *cmd = "launchctl";

1899 + char *cmd;

1900 int result;

1901 struct child_process child = CHILD_PROCESS_INIT;

1902 char *uid = launchctl_get_uid();

1903

1898 - get_schedule_cmd(&cmd, NULL);

1904 + get_schedule_cmd("launchctl", NULL, &cmd);

1905 strvec_split(&child.args, cmd);

1906 strvec_pushl(&child.args, enable ? "bootstrap" : "bootout", uid,

1907 filename, NULL);

@@ -1908,6 +1914,7 @@ static int launchctl_boot_plist(int enable, const char *filename)

1914

1915 result = finish_command(&child);

1916

1917 + free(cmd);

1918 free(uid);

1919 return result;

1920 }

@@ -1959,10 +1966,10 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit

1966 static unsigned long lock_file_timeout_ms = ULONG_MAX;

1967 struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT;

1968 struct stat st;

1962 - const char *cmd = "launchctl";

1969 + char *cmd;

1970 int minute = get_random_minute();

1971

1965 - get_schedule_cmd(&cmd, NULL);

1972 + get_schedule_cmd("launchctl", NULL, &cmd);

1973 preamble = "<?xml version=\"1.0\"?>\n"

1974 "<!DOCTYPE plist PUBLIC \"-//Apple//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n"

1975 "<plist version=\"1.0\">"

@@ -2052,6 +2059,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit

2059

2060 free(filename);

2061 free(name);

2062 + free(cmd);

2063 strbuf_release(&plist);

2064 strbuf_release(&plist2);

2065 return 0;

@@ -2076,9 +2084,8 @@ static int launchctl_update_schedule(int run_maintenance, int fd UNUSED)

2084

2085 static int is_schtasks_available(void)

2086 {

2079 - const char *cmd = "schtasks";

2087 int is_available;

2081 - if (get_schedule_cmd(&cmd, &is_available))

2088 + if (get_schedule_cmd("schtasks", &is_available, NULL))

2089 return is_available;

2090

2091 #ifdef GIT_WINDOWS_NATIVE

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

2104

2105 static int schtasks_remove_task(enum schedule_priority schedule)

2106 {

2100 - const char *cmd = "schtasks";

2107 + char *cmd;

2108 struct child_process child = CHILD_PROCESS_INIT;

2109 const char *frequency = get_frequency(schedule);

2110 char *name = schtasks_task_name(frequency);

2111

2105 - get_schedule_cmd(&cmd, NULL);

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

2113 strvec_split(&child.args, cmd);

2114 strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL);

2115 free(name);

2116 + free(cmd);

2117

2118 return run_command(&child);

2119 }

@@ -2119,7 +2127,7 @@ static int schtasks_remove_tasks(void)

2127

2128 static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule)

2129 {

2122 - const char *cmd = "schtasks";

2130 + char *cmd;

2131 int result;

2132 struct child_process child = CHILD_PROCESS_INIT;

2133 const char *xml;

@@ -2129,7 +2137,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority

2137 struct strbuf tfilename = STRBUF_INIT;

2138 int minute = get_random_minute();

2139

2132 - get_schedule_cmd(&cmd, NULL);

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

2141

2142 strbuf_addf(&tfilename, "%s/schedule_%s_XXXXXX",

2143 get_git_common_dir(), frequency);

@@ -2235,6 +2243,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority

2243

2244 delete_tempfile(&tfile);

2245 free(name);

2246 + free(cmd);

2247 return result;

2248 }

2249

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

2285

2286 static int is_crontab_available(void)

2287 {

2279 - const char *cmd = "crontab";

2288 + char *cmd;

2289 int is_available;

2290 + int ret;

2291

2282 - if (get_schedule_cmd(&cmd, &is_available))

2283 - return is_available;

2292 + if (get_schedule_cmd("crontab", &is_available, &cmd)) {

2293 + ret = is_available;

2294 + goto out;

2295 + }

2296

2297 #ifdef __APPLE__

2298 /*

2299 * macOS has cron, but it requires special permissions and will

2300 * create a UI alert when attempting to run this command.

2301 */

2290 - return 0;

2302 + ret = 0;

2303 #else

2292 - return check_crontab_process(cmd);

2304 + ret = check_crontab_process(cmd);

2305 #endif

2306 +

2307 +out:

2308 + free(cmd);

2309 + return ret;

2310 }

2311

2312 #define BEGIN_LINE "# BEGIN GIT MAINTENANCE SCHEDULE"

@@ -2298,7 +2314,7 @@ static int is_crontab_available(void)

2314

2315 static int crontab_update_schedule(int run_maintenance, int fd)

2316 {

2301 - const char *cmd = "crontab";

2317 + char *cmd;

2318 int result = 0;

2319 int in_old_region = 0;

2320 struct child_process crontab_list = CHILD_PROCESS_INIT;

@@ -2308,15 +2324,17 @@ static int crontab_update_schedule(int run_maintenance, int fd)

2324 struct tempfile *tmpedit = NULL;

2325 int minute = get_random_minute();

2326

2311 - get_schedule_cmd(&cmd, NULL);

2327 + get_schedule_cmd("crontab", NULL, &cmd);

2328 strvec_split(&crontab_list.args, cmd);

2329 strvec_push(&crontab_list.args, "-l");

2330 crontab_list.in = -1;

2331 crontab_list.out = dup(fd);

2332 crontab_list.git_cmd = 0;

2333

2318 - if (start_command(&crontab_list))

2319 - return error(_("failed to run 'crontab -l'; your system might not support 'cron'"));

2334 + if (start_command(&crontab_list)) {

2335 + result = error(_("failed to run 'crontab -l'; your system might not support 'cron'"));

2336 + goto out;

2337 + }

2338

2339 /* Ignore exit code, as an empty crontab will return error. */

2340 finish_command(&crontab_list);

@@ -2386,8 +2404,10 @@ static int crontab_update_schedule(int run_maintenance, int fd)

2404 result = error(_("'crontab' died"));

2405 else

2406 fclose(cron_list);

2407 +

2408 out:

2409 delete_tempfile(&tmpedit);

2410 + free(cmd);

2411 return result;

2412 }

2413

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

2430

2431 static int is_systemd_timer_available(void)

2432 {

2413 - const char *cmd = "systemctl";

2433 int is_available;

2434

2416 - if (get_schedule_cmd(&cmd, &is_available))

2435 + if (get_schedule_cmd("systemctl", &is_available, NULL))

2436 return is_available;

2437

2438 return real_is_systemd_timer_available();

@@ -2594,9 +2613,10 @@ static int systemd_timer_enable_unit(int enable,

2613 enum schedule_priority schedule,

2614 int minute)

2615 {

2597 - const char *cmd = "systemctl";

2616 + char *cmd = NULL;

2617 struct child_process child = CHILD_PROCESS_INIT;

2618 const char *frequency = get_frequency(schedule);

2619 + int ret;

2620

2621 /*

2622 * Disabling the systemd unit while it is already disabled makes

@@ -2607,20 +2627,25 @@ static int systemd_timer_enable_unit(int enable,

2627 * On the other hand, enabling a systemd unit which is already enabled

2628 * produces no error.

2629 */

2610 - if (!enable)

2630 + if (!enable) {

2631 child.no_stderr = 1;

2612 - else if (systemd_timer_write_timer_file(schedule, minute))

2613 - return -1;

2632 + } else if (systemd_timer_write_timer_file(schedule, minute)) {

2633 + ret = -1;

2634 + goto out;

2635 + }

2636

2615 - get_schedule_cmd(&cmd, NULL);

2637 + get_schedule_cmd("systemctl", NULL, &cmd);

2638 strvec_split(&child.args, cmd);

2639 strvec_pushl(&child.args, "--user", enable ? "enable" : "disable",

2640 "--now", NULL);

2641 strvec_pushf(&child.args, SYSTEMD_UNIT_FORMAT, frequency, "timer");

2642

2621 - if (start_command(&child))

2622 - return error(_("failed to start systemctl"));

2623 - if (finish_command(&child))

2643 + if (start_command(&child)) {

2644 + ret = error(_("failed to start systemctl"));

2645 + goto out;

2646 + }

2647 +

2648 + if (finish_command(&child)) {

2649 /*

2650 * Disabling an already disabled systemd unit makes

2651 * systemctl fail.

@@ -2628,9 +2653,17 @@ static int systemd_timer_enable_unit(int enable,

2653 *

2654 * Enabling an enabled systemd unit doesn't fail.

2655 */

2631 - if (enable)

2632 - return error(_("failed to run systemctl"));

2633 - return 0;

2656 + if (enable) {

2657 + ret = error(_("failed to run systemctl"));

2658 + goto out;

2659 + }

2660 + }

2661 +

2662 + ret = 0;

2663 +

2664 +out:

2665 + free(cmd);

2666 + return ret;

2667 }

2668

2669 /*

t/t7900-maintenance.sh

@@ -2,6 +2,7 @@

3 test_description='git maintenance builtin'

5 +TEST_PASSES_SANITIZE_LEAK=true

6 . ./test-lib.sh

8 GIT_TEST_COMMIT_GRAPH=0