{"thread":{"id":"55655","subject":"[PATCH] maintenance: fix two memory leaks","startedAt":"2021-05-09T22:17:09Z","lastAt":"2021-05-11T15:13:16Z","messageCount":9,"participants":["Lénaïc Huard","Junio C Hamano","lilinchao@oschina.cn","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"423983","messageId":"20210509221613.474887-1-lenaic@lhuard.fr","threadId":"55655","inReplyTo":null,"subject":"[PATCH] maintenance: fix two memory leaks","fromName":"Lénaïc Huard","fromEmail":"lenaic@lhuard.fr","sentAt":"2021-05-09T22:16:13Z","receivedAt":"2021-05-09T22:17:09Z","isPatch":true,"sender":{"key":"lenaic@lhuard.fr","avatar":"https://avatars.githubusercontent.com/u/1437785?v=4"},"body":"Fixes two memory leaks when running `git maintenance start` or `git\nmaintenance stop` in `update_background_schedule`:\n\n$ valgrind --leak-check=full ~/git/bin/git maintenance start\n==76584== Memcheck, a memory error detector\n==76584== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.\n==76584== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info\n==76584== Command: /home/lenaic/git/bin/git maintenance start\n==76584==\n==76584==\n==76584== HEAP SUMMARY:\n==76584==     in use at exit: 34,880 bytes in 252 blocks\n==76584==   total heap usage: 820 allocs, 568 frees, 146,414 bytes allocated\n==76584==\n==76584== 65 bytes in 1 blocks are definitely lost in loss record 17 of 39\n==76584==    at 0x483E6AF: malloc (vg_replace_malloc.c:306)\n==76584==    by 0x3DC39C: xrealloc (wrapper.c:126)\n==76584==    by 0x3992CC: strbuf_grow (strbuf.c:98)\n==76584==    by 0x39A473: strbuf_vaddf (strbuf.c:392)\n==76584==    by 0x39BC54: xstrvfmt (strbuf.c:979)\n==76584==    by 0x39BD2C: xstrfmt (strbuf.c:989)\n==76584==    by 0x18451B: update_background_schedule (gc.c:1977)\n==76584==    by 0x1846F6: maintenance_start (gc.c:2011)\n==76584==    by 0x1847B4: cmd_maintenance (gc.c:2030)\n==76584==    by 0x127A2E: run_builtin (git.c:453)\n==76584==    by 0x127E81: handle_builtin (git.c:704)\n==76584==    by 0x128142: run_argv (git.c:771)\n==76584==\n==76584== 240 bytes in 1 blocks are definitely lost in loss record 29 of 39\n==76584==    at 0x4840D7B: realloc (vg_replace_malloc.c:834)\n==76584==    by 0x491CE5D: getdelim (in /usr/lib/libc-2.33.so)\n==76584==    by 0x39ADD7: strbuf_getwholeline (strbuf.c:635)\n==76584==    by 0x39AF31: strbuf_getdelim (strbuf.c:706)\n==76584==    by 0x39B064: strbuf_getline_lf (strbuf.c:727)\n==76584==    by 0x184273: crontab_update_schedule (gc.c:1919)\n==76584==    by 0x184678: update_background_schedule (gc.c:1997)\n==76584==    by 0x1846F6: maintenance_start (gc.c:2011)\n==76584==    by 0x1847B4: cmd_maintenance (gc.c:2030)\n==76584==    by 0x127A2E: run_builtin (git.c:453)\n==76584==    by 0x127E81: handle_builtin (git.c:704)\n==76584==    by 0x128142: run_argv (git.c:771)\n==76584==\n==76584== LEAK SUMMARY:\n==76584==    definitely lost: 305 bytes in 2 blocks\n==76584==    indirectly lost: 0 bytes in 0 blocks\n==76584==      possibly lost: 0 bytes in 0 blocks\n==76584==    still reachable: 34,575 bytes in 250 blocks\n==76584==         suppressed: 0 bytes in 0 blocks\n==76584== Reachable blocks (those to which a pointer was found) are not shown.\n==76584== To see them, rerun with: --leak-check=full --show-leak-kinds=all\n==76584==\n==76584== For lists of detected and suppressed errors, rerun with: -s\n==76584== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)\n\nSigned-off-by: Lénaïc Huard <lenaic@lhuard.fr>\n---\n builtin/gc.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ef7226d7bc..2574068ae2 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1947,6 +1947,7 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)\n \t\tfprintf(cron_in, \"\\n%s\\n\", END_LINE);\n \t}\n \n+\tstrbuf_release(&line);\n \tfflush(cron_in);\n \tfclose(cron_in);\n \tclose(crontab_edit.in);\n@@ -1999,6 +2000,7 @@ static int update_background_schedule(int enable)\n \t\tdie(\"unknown background scheduler: %s\", scheduler);\n \n \trollback_lock_file(&lk);\n+\tfree(lock_path);\n \tfree(testing);\n \treturn result;\n }\n-- \n2.31.1\n\n"},{"id":"424004","messageId":"xmqqpmxzqeqd.fsf@gitster.g","threadId":"55655","inReplyTo":"20210509221613.474887-1-lenaic@lhuard.fr","subject":"Re: [PATCH] maintenance: fix two memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-10T06:34:18Z","receivedAt":"2021-05-10T06:34:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lénaïc Huard <lenaic@lhuard.fr> writes:\n\n> Fixes two memory leaks when running `git maintenance start` or `git\n> maintenance stop` in `update_background_schedule`:\n\nThanks, both places look correct, but I have one minor \"hmph\"\ncomment.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index ef7226d7bc..2574068ae2 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1947,6 +1947,7 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)\n>  \t\tfprintf(cron_in, \"\\n%s\\n\", END_LINE);\n>  \t}\n>  \n> +\tstrbuf_release(&line);\n>  \tfflush(cron_in);\n>  \tfclose(cron_in);\n>  \tclose(crontab_edit.in);\n\nThis is somewhat a curious placement---the loop that iterates over\nthe cron_list FILE with \"while (!strbuf_getline_lf(&line, cron_list)\"\nis the only place the list strbuf is used, and I wonder if it makes\nmore sense to do this immediately after the loop.\n\n> @@ -1999,6 +2000,7 @@ static int update_background_schedule(int enable)\n>  \t\tdie(\"unknown background scheduler: %s\", scheduler);\n>  \n>  \trollback_lock_file(&lk);\n> +\tfree(lock_path);\n>  \tfree(testing);\n>  \treturn result;\n>  }\n\nThis one looks quite natural.\n"},{"id":"424005","messageId":"67ed4ca6b15a11eba8d80024e87935e7@oschina.cn","threadId":"55655","inReplyTo":"20210509221613.474887-1-lenaic@lhuard.fr","subject":"Re: [PATCH] maintenance: fix two memory leaks","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2021-05-10T06:38:56Z","receivedAt":"2021-05-10T06:39:06Z","isPatch":true,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":"Hi,\n\n>Fixes two memory leaks when running `git maintenance start` or `git\n>maintenance stop` in `update_background_schedule`:\n>\n>$ valgrind --leak-check=full ~/git/bin/git maintenance start\n>==76584== Memcheck, a memory error detector\n>==76584== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.\n>==76584== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info\n>==76584== Command: /home/lenaic/git/bin/git maintenance start\n>==76584==\n>==76584==\n>==76584== HEAP SUMMARY:\n>==76584==     in use at exit: 34,880 bytes in 252 blocks\n>==76584==   total heap usage: 820 allocs, 568 frees, 146,414 bytes allocated\n>==76584==\n>==76584== 65 bytes in 1 blocks are definitely lost in loss record 17 of 39\n>==76584==    at 0x483E6AF: malloc (vg_replace_malloc.c:306)\n>==76584==    by 0x3DC39C: xrealloc (wrapper.c:126)\n>==76584==    by 0x3992CC: strbuf_grow (strbuf.c:98)\n>==76584==    by 0x39A473: strbuf_vaddf (strbuf.c:392)\n>==76584==    by 0x39BC54: xstrvfmt (strbuf.c:979)\n>==76584==    by 0x39BD2C: xstrfmt (strbuf.c:989)\n>==76584==    by 0x18451B: update_background_schedule (gc.c:1977)\n>==76584==    by 0x1846F6: maintenance_start (gc.c:2011)\n>==76584==    by 0x1847B4: cmd_maintenance (gc.c:2030)\n>==76584==    by 0x127A2E: run_builtin (git.c:453)\n>==76584==    by 0x127E81: handle_builtin (git.c:704)\n>==76584==    by 0x128142: run_argv (git.c:771)\n>==76584==\n>==76584== 240 bytes in 1 blocks are definitely lost in loss record 29 of 39\n>==76584==    at 0x4840D7B: realloc (vg_replace_malloc.c:834)\n>==76584==    by 0x491CE5D: getdelim (in /usr/lib/libc-2.33.so)\n>==76584==    by 0x39ADD7: strbuf_getwholeline (strbuf.c:635)\n>==76584==    by 0x39AF31: strbuf_getdelim (strbuf.c:706)\n>==76584==    by 0x39B064: strbuf_getline_lf (strbuf.c:727)\n>==76584==    by 0x184273: crontab_update_schedule (gc.c:1919)\n>==76584==    by 0x184678: update_background_schedule (gc.c:1997)\n>==76584==    by 0x1846F6: maintenance_start (gc.c:2011)\n>==76584==    by 0x1847B4: cmd_maintenance (gc.c:2030)\n>==76584==    by 0x127A2E: run_builtin (git.c:453)\n>==76584==    by 0x127E81: handle_builtin (git.c:704)\n>==76584==    by 0x128142: run_argv (git.c:771)\n>==76584==\n>==76584== LEAK SUMMARY:\n>==76584==    definitely lost: 305 bytes in 2 blocks\n>==76584==    indirectly lost: 0 bytes in 0 blocks\n>==76584==      possibly lost: 0 bytes in 0 blocks\n>==76584==    still reachable: 34,575 bytes in 250 blocks\n>==76584==         suppressed: 0 bytes in 0 blocks\n>==76584== Reachable blocks (those to which a pointer was found) are not shown.\n>==76584== To see them, rerun with: --leak-check=full --show-leak-kinds=all\n>==76584==\n>==76584== For lists of detected and suppressed errors, rerun with: -s\n>==76584== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)\n>\n>Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr>\n>---\n> builtin/gc.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n>diff --git a/builtin/gc.c b/builtin/gc.c\n>index ef7226d7bc..2574068ae2 100644\n>--- a/builtin/gc.c\n>+++ b/builtin/gc.c\n>@@ -1947,6 +1947,7 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)\n> fprintf(cron_in, \"\\n%s\\n\", END_LINE);\n> }\n>\n>+\tstrbuf_release(&line); \n> fflush(cron_in);\n> fclose(cron_in);\n> close(crontab_edit.in);\n>@@ -1999,6 +2000,7 @@ static int update_background_schedule(int enable)\n> die(\"unknown background scheduler: %s\", scheduler);\n>\n> rollback_lock_file(&lk);\n>+\tfree(lock_path); \nBased on your change, I think when \"hold_lock_file_for_update()<0\", we should also free local_path\n\n> free(testing);\n> return result;\n> }\n>--\n>2.31.1\n>\n> \n\nThanks"},{"id":"424006","messageId":"xmqqlf8nqczk.fsf@gitster.g","threadId":"55655","inReplyTo":"67ed4ca6b15a11eba8d80024e87935e7@oschina.cn","subject":"Re: [PATCH] maintenance: fix two memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-10T07:11:59Z","receivedAt":"2021-05-10T07:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"lilinchao@oschina.cn\" <lilinchao@oschina.cn> writes:\n\n>>@@ -1999,6 +2000,7 @@ static int update_background_schedule(int enable)\n>> die(\"unknown background scheduler: %s\", scheduler);\n>>\n>> rollback_lock_file(&lk);\n>>+\tfree(lock_path); \n> Based on your change, I think when \"hold_lock_file_for_update()<0\", we should also free local_path\n> Thanks\n\nMeaning something like this?\n\n\n builtin/gc.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git c/builtin/gc.c w/builtin/gc.c\nindex 98a803196b..50565c37c7 100644\n--- c/builtin/gc.c\n+++ w/builtin/gc.c\n@@ -1971,8 +1971,10 @@ static int update_background_schedule(int enable)\n \t\tcmd = sep + 1;\n \t}\n \n-\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)\n-\t\treturn error(_(\"another process is scheduling background maintenance\"));\n+\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n+\t\tresult = error(_(\"another process is scheduling background maintenance\"));\n+\t\tgoto cleanup;\n+\t}\n \n \tif (!strcmp(scheduler, \"launchctl\"))\n \t\tresult = launchctl_update_schedule(enable, get_lock_file_fd(&lk), cmd);\n@@ -1984,6 +1986,9 @@ static int update_background_schedule(int enable)\n \t\tdie(\"unknown background scheduler: %s\", scheduler);\n \n \trollback_lock_file(&lk);\n+\n+cleanup:\n+\tfree(lock_path);\n \tfree(testing);\n \treturn result;\n }\n"},{"id":"424010","messageId":"5abbc3beb16411eba07a0024e87935e7@oschina.cn","threadId":"55655","inReplyTo":"0c04f7c2b15f11eb82baa4badb2c2b1178978@pobox.com","subject":"Re: Re: [PATCH] maintenance: fix two memory leaks","fromName":"lilinchao@oschina.cn","fromEmail":"lilinchao@oschina.cn","sentAt":"2021-05-10T07:50:08Z","receivedAt":"2021-05-10T07:50:21Z","isPatch":true,"sender":{"key":"lilinchao@oschina.cn","avatar":null},"body":">\"lilinchao@oschina.cn\" <lilinchao@oschina.cn> writes:\n>\n>>>@@ -1999,6 +2000,7 @@ static int update_background_schedule(int enable)\n>>> die(\"unknown background scheduler: %s\", scheduler);\n>>>\n>>> rollback_lock_file(&lk);\n>>>+\tfree(lock_path);\n>> Based on your change, I think when \"hold_lock_file_for_update()<0\", we should also free local_path\n>> Thanks\n>\n>Meaning something like this?\n>\n>\n> builtin/gc.c | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n>\n>diff --git c/builtin/gc.c w/builtin/gc.c\n>index 98a803196b..50565c37c7 100644\n>--- c/builtin/gc.c\n>+++ w/builtin/gc.c\n>@@ -1971,8 +1971,10 @@ static int update_background_schedule(int enable)\n> cmd = sep + 1;\n> }\n>\n>-\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)\n>-\treturn error(_(\"another process is scheduling background maintenance\"));\n>+\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n>+\tresult = error(_(\"another process is scheduling background maintenance\"));\n>+\tgoto cleanup;\n>+\t}\n>\n> if (!strcmp(scheduler, \"launchctl\"))\n> result = launchctl_update_schedule(enable, get_lock_file_fd(&lk), cmd);\n>@@ -1984,6 +1986,9 @@ static int update_background_schedule(int enable)\n> die(\"unknown background scheduler: %s\", scheduler);\n>\n> rollback_lock_file(&lk);\n>+\n>+cleanup:\n>+\tfree(lock_path);\n> free(testing);\n> return result;\n> }\n\nYes, it's almost like this, and your is better than I thought.\nI just referred to this function(L1266-L1321): \n\nstatic int maintenance_run_tasks(struct maintenance_run_opts *opts)\n{\n        ...\n\nif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n                ...\n\nfree(lock_path);\nreturn 0;\n}\nfree(lock_path);\n\n        ...\n        ...\n\nrollback_lock_file(&lk);\nreturn result;\n} \n\nand thought we should also apply it to \"update_background_schedule()\".\n\n\n"},{"id":"424074","messageId":"20210510195909.621534-1-lenaic@lhuard.fr","threadId":"55655","inReplyTo":"20210509221613.474887-1-lenaic@lhuard.fr","subject":"[PATCH v2 0/1] maintenance: fix two memory leaks","fromName":"Lénaïc Huard","fromEmail":"lenaic@lhuard.fr","sentAt":"2021-05-10T19:59:08Z","receivedAt":"2021-05-10T20:01:47Z","isPatch":true,"sender":{"key":"lenaic@lhuard.fr","avatar":"https://avatars.githubusercontent.com/u/1437785?v=4"},"body":"Hi!\n\nThank you for the code review.\n\nI’ve moved the `strbuf_release(&line);` closer to the last point where\n`line` is used.\n\nAnd I’ve included Junio’s patch to address the missing\n`free(local_path)` when `hold_lock_file_for_update()<0`.\n\nLénaïc Huard (1):\n  maintenance: fix two memory leaks\n\n builtin/gc.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\n-- \n2.31.1\n\n"},{"id":"424075","messageId":"20210510195909.621534-2-lenaic@lhuard.fr","threadId":"55655","inReplyTo":"20210510195909.621534-1-lenaic@lhuard.fr","subject":"[PATCH v2 1/1] maintenance: fix two memory leaks","fromName":"Lénaïc Huard","fromEmail":"lenaic@lhuard.fr","sentAt":"2021-05-10T19:59:09Z","receivedAt":"2021-05-10T20:01:47Z","isPatch":true,"sender":{"key":"lenaic@lhuard.fr","avatar":"https://avatars.githubusercontent.com/u/1437785?v=4"},"body":"Fixes two memory leaks when running `git maintenance start` or `git\nmaintenance stop` in `update_background_schedule`:\n\n$ valgrind --leak-check=full ~/git/bin/git maintenance start\n==76584== Memcheck, a memory error detector\n==76584== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.\n==76584== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info\n==76584== Command: /home/lenaic/git/bin/git maintenance start\n==76584==\n==76584==\n==76584== HEAP SUMMARY:\n==76584==     in use at exit: 34,880 bytes in 252 blocks\n==76584==   total heap usage: 820 allocs, 568 frees, 146,414 bytes allocated\n==76584==\n==76584== 65 bytes in 1 blocks are definitely lost in loss record 17 of 39\n==76584==    at 0x483E6AF: malloc (vg_replace_malloc.c:306)\n==76584==    by 0x3DC39C: xrealloc (wrapper.c:126)\n==76584==    by 0x3992CC: strbuf_grow (strbuf.c:98)\n==76584==    by 0x39A473: strbuf_vaddf (strbuf.c:392)\n==76584==    by 0x39BC54: xstrvfmt (strbuf.c:979)\n==76584==    by 0x39BD2C: xstrfmt (strbuf.c:989)\n==76584==    by 0x18451B: update_background_schedule (gc.c:1977)\n==76584==    by 0x1846F6: maintenance_start (gc.c:2011)\n==76584==    by 0x1847B4: cmd_maintenance (gc.c:2030)\n==76584==    by 0x127A2E: run_builtin (git.c:453)\n==76584==    by 0x127E81: handle_builtin (git.c:704)\n==76584==    by 0x128142: run_argv (git.c:771)\n==76584==\n==76584== 240 bytes in 1 blocks are definitely lost in loss record 29 of 39\n==76584==    at 0x4840D7B: realloc (vg_replace_malloc.c:834)\n==76584==    by 0x491CE5D: getdelim (in /usr/lib/libc-2.33.so)\n==76584==    by 0x39ADD7: strbuf_getwholeline (strbuf.c:635)\n==76584==    by 0x39AF31: strbuf_getdelim (strbuf.c:706)\n==76584==    by 0x39B064: strbuf_getline_lf (strbuf.c:727)\n==76584==    by 0x184273: crontab_update_schedule (gc.c:1919)\n==76584==    by 0x184678: update_background_schedule (gc.c:1997)\n==76584==    by 0x1846F6: maintenance_start (gc.c:2011)\n==76584==    by 0x1847B4: cmd_maintenance (gc.c:2030)\n==76584==    by 0x127A2E: run_builtin (git.c:453)\n==76584==    by 0x127E81: handle_builtin (git.c:704)\n==76584==    by 0x128142: run_argv (git.c:771)\n==76584==\n==76584== LEAK SUMMARY:\n==76584==    definitely lost: 305 bytes in 2 blocks\n==76584==    indirectly lost: 0 bytes in 0 blocks\n==76584==      possibly lost: 0 bytes in 0 blocks\n==76584==    still reachable: 34,575 bytes in 250 blocks\n==76584==         suppressed: 0 bytes in 0 blocks\n==76584== Reachable blocks (those to which a pointer was found) are not shown.\n==76584== To see them, rerun with: --leak-check=full --show-leak-kinds=all\n==76584==\n==76584== For lists of detected and suppressed errors, rerun with: -s\n==76584== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)\n\nSigned-off-by: Lénaïc Huard <lenaic@lhuard.fr>\n---\n builtin/gc.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex ef7226d7bc..484fe983d3 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1924,6 +1924,7 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)\n \t\telse if (!in_old_region)\n \t\t\tfprintf(cron_in, \"%s\\n\", line.buf);\n \t}\n+\tstrbuf_release(&line);\n \n \tif (run_maintenance) {\n \t\tstruct strbuf line_format = STRBUF_INIT;\n@@ -1986,8 +1987,10 @@ static int update_background_schedule(int enable)\n \t\tcmd = sep + 1;\n \t}\n \n-\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)\n-\t\treturn error(_(\"another process is scheduling background maintenance\"));\n+\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n+\t\tresult = error(_(\"another process is scheduling background maintenance\"));\n+\t\tgoto cleanup;\n+\t}\n \n \tif (!strcmp(scheduler, \"launchctl\"))\n \t\tresult = launchctl_update_schedule(enable, get_lock_file_fd(&lk), cmd);\n@@ -1999,6 +2002,9 @@ static int update_background_schedule(int enable)\n \t\tdie(\"unknown background scheduler: %s\", scheduler);\n \n \trollback_lock_file(&lk);\n+\n+cleanup:\n+\tfree(lock_path);\n \tfree(testing);\n \treturn result;\n }\n-- \n2.31.1\n\n"},{"id":"424094","messageId":"xmqq5yzqosm0.fsf@gitster.g","threadId":"55655","inReplyTo":"20210510195909.621534-1-lenaic@lhuard.fr","subject":"Re: [PATCH v2 0/1] maintenance: fix two memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T03:29:43Z","receivedAt":"2021-05-11T03:29:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lénaïc Huard <lenaic@lhuard.fr> writes:\n\n> Hi!\n>\n> Thank you for the code review.\n>\n> I’ve moved the `strbuf_release(&line);` closer to the last point where\n> `line` is used.\n>\n> And I’ve included Junio’s patch to address the missing\n> `free(local_path)` when `hold_lock_file_for_update()<0`.\n>\n> Lénaïc Huard (1):\n>   maintenance: fix two memory leaks\n>\n>  builtin/gc.c | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n\nThanks, will queue.\n"},{"id":"424138","messageId":"01dccb51-cb2e-ea6f-a05c-d76bb7ed725f@gmail.com","threadId":"55655","inReplyTo":"20210510195909.621534-2-lenaic@lhuard.fr","subject":"Re: [PATCH v2 1/1] maintenance: fix two memory leaks","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-05-11T15:13:12Z","receivedAt":"2021-05-11T15:13:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/10/2021 3:59 PM, Lénaïc Huard wrote:\n> Fixes two memory leaks when running `git maintenance start` or `git\n> maintenance stop` in `update_background_schedule`:\n\nThanks for finding these leaks.\n\n> ---\n>  builtin/gc.c | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index ef7226d7bc..484fe983d3 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -1924,6 +1924,7 @@ static int crontab_update_schedule(int run_maintenance, int fd, const char *cmd)\n>  \t\telse if (!in_old_region)\n>  \t\t\tfprintf(cron_in, \"%s\\n\", line.buf);\n>  \t}\n> +\tstrbuf_release(&line);\n>  \n>  \tif (run_maintenance) {\n>  \t\tstruct strbuf line_format = STRBUF_INIT;\n> @@ -1986,8 +1987,10 @@ static int update_background_schedule(int enable)\n>  \t\tcmd = sep + 1;\n>  \t}\n>  \n> -\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)\n> -\t\treturn error(_(\"another process is scheduling background maintenance\"));\n> +\tif (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {\n> +\t\tresult = error(_(\"another process is scheduling background maintenance\"));\n> +\t\tgoto cleanup;\n> +\t}\n>  \n>  \tif (!strcmp(scheduler, \"launchctl\"))\n>  \t\tresult = launchctl_update_schedule(enable, get_lock_file_fd(&lk), cmd);\n> @@ -1999,6 +2002,9 @@ static int update_background_schedule(int enable)\n>  \t\tdie(\"unknown background scheduler: %s\", scheduler);\n>  \n>  \trollback_lock_file(&lk);\n> +\n> +cleanup:\n> +\tfree(lock_path);\n\nAnd I agree that this version looks good. \n\nThanks,\n-Stolee\n"}]}