{"thread":{"id":"60311","subject":"[PATCH 0/3] a few more leak fixes","startedAt":"2023-10-05T21:28:07Z","lastAt":"2023-10-06T16:45:14Z","messageCount":7,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"482720","messageId":"20231005212802.GA982892@coredump.intra.peff.net","threadId":"60311","inReplyTo":null,"subject":"[PATCH 0/3] a few more leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-05T21:28:02Z","receivedAt":"2023-10-05T21:28:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was really bothered that using clang with SANITIZE=leak found a leak\nthat gcc didn't. And then I was doubly bothered to find that there is\none that gcc finds that clang doesn't!\n\nI don't think either of these are urgent or important leaks on their\nown, but the flaky nature of the results makes it annoying while trying\nto find and clean up other leaks. So here are some fixes. Patches 1 and\n3 are those two cases, respectively. Patch 2 is a more interesting\nleak-fix enabled by the infrastructure added in patch 1.\n\n  [1/3]: decorate: add clear_decoration() function\n  [2/3]: revision: clear decoration structs during release_revisions()\n  [3/3]: daemon: free listen_addr before returning\n\n daemon.c                         | 37 ++++++++++++++++++--------------\n decorate.c                       | 15 +++++++++++++\n decorate.h                       | 10 +++++++++\n line-log.c                       | 10 +++++++++\n line-log.h                       |  2 ++\n revision.c                       |  9 ++++++++\n t/helper/test-example-decorate.c |  2 ++\n t/t4217-log-limit.sh             |  1 +\n t/t5811-proto-disable-git.sh     |  2 ++\n t/t9004-example.sh               |  2 ++\n 10 files changed, 74 insertions(+), 16 deletions(-)\n\n-Peff\n"},{"id":"482721","messageId":"20231005212902.GA986467@coredump.intra.peff.net","threadId":"60311","inReplyTo":"20231005212802.GA982892@coredump.intra.peff.net","subject":"[PATCH 1/3] decorate: add clear_decoration() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-05T21:29:02Z","receivedAt":"2023-10-05T21:29:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's not currently any way to free the resources associated with a\ndecoration struct. As a result, we have several memory leaks which\ncannot easily be plugged.\n\nLet's add a \"clear\" function and make use of it in the example code of\nt9004. This removes the only leak from that script, so we can mark it as\npassing the leak sanitizer.\n\nCuriously this leak is found only when running SANITIZE=leak with clang,\nbut not with gcc.  But it is a bog-standard leak: we allocate some\nmemory in a local variable struct, and then exit main() without\nreleasing it. I'm not sure why gcc doesn't find it. After this\npatch, both compilers report it as leak-free.\n\nNote that the clear function takes a callback to free the individual\nentries. That's not needed for our example (which is just decorating\nwith ints), but will be for real callers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n decorate.c                       | 15 +++++++++++++++\n decorate.h                       | 10 ++++++++++\n t/helper/test-example-decorate.c |  2 ++\n t/t9004-example.sh               |  2 ++\n 4 files changed, 29 insertions(+)\n\ndiff --git a/decorate.c b/decorate.c\nindex a5c43c0c14..69aeb142b4 100644\n--- a/decorate.c\n+++ b/decorate.c\n@@ -81,3 +81,18 @@ void *lookup_decoration(struct decoration *n, const struct object *obj)\n \t\t\tj = 0;\n \t}\n }\n+\n+void clear_decoration(struct decoration *n, void (*free_cb)(void *))\n+{\n+\tif (free_cb) {\n+\t\tunsigned int i;\n+\t\tfor (i = 0; i < n->size; i++) {\n+\t\t\tvoid *d = n->entries[i].decoration;\n+\t\t\tif (d)\n+\t\t\t\tfree_cb(d);\n+\t\t}\n+\t}\n+\n+\tFREE_AND_NULL(n->entries);\n+\tn->size = n->nr = 0;\n+}\ndiff --git a/decorate.h b/decorate.h\nindex ee43dee1f0..cdeb17c9df 100644\n--- a/decorate.h\n+++ b/decorate.h\n@@ -58,4 +58,14 @@ void *add_decoration(struct decoration *n, const struct object *obj, void *decor\n  */\n void *lookup_decoration(struct decoration *n, const struct object *obj);\n \n+/*\n+ * Clear all decoration entries, releasing any memory used by the structure.\n+ * If free_cb is not NULL, it is called for every decoration value currently\n+ * stored.\n+ *\n+ * After clearing, the decoration struct can be used again. The \"name\" field is\n+ * retained.\n+ */\n+void clear_decoration(struct decoration *n, void (*free_cb)(void *));\n+\n #endif\ndiff --git a/t/helper/test-example-decorate.c b/t/helper/test-example-decorate.c\nindex 2ed910adaa..8f59f6be4c 100644\n--- a/t/helper/test-example-decorate.c\n+++ b/t/helper/test-example-decorate.c\n@@ -72,5 +72,7 @@ int cmd__example_decorate(int argc UNUSED, const char **argv UNUSED)\n \tif (objects_noticed != 2)\n \t\tBUG(\"should have 2 objects\");\n \n+\tclear_decoration(&n, NULL);\n+\n \treturn 0;\n }\ndiff --git a/t/t9004-example.sh b/t/t9004-example.sh\nindex 7e8894a4a7..590aab0304 100755\n--- a/t/t9004-example.sh\n+++ b/t/t9004-example.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='check that example code compiles and runs'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'decorate' '\n-- \n2.42.0.836.g4b88bf80c5\n\n"},{"id":"482722","messageId":"20231005213014.GB986467@coredump.intra.peff.net","threadId":"60311","inReplyTo":"20231005212802.GA982892@coredump.intra.peff.net","subject":"[PATCH 2/3] revision: clear decoration structs during release_revisions()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-05T21:30:14Z","receivedAt":"2023-10-05T21:30:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The point of release_revisions() is to free memory associated with the\nrev_info struct, but we have several \"struct decoration\" members that\nare left untouched. Since the previous commit introduced a function to\ndo that, we can just call it.\n\nWe do have to provide some specialized callbacks to map the void\npointers onto real ones (the alternative would be casting the existing\nfunction pointers; this generally works because \"void *\" is usually\ninterchangeable with a struct pointer, but it is technically forbidden\nby the standard).\n\nSince the line-log code does not expose the type it stores in the\ndecoration (nor of course the function to free it), I put this behind a\ngeneric line_log_free() entry point. It's possible we may need to add\nmore line-log specific bits anyway (running t4211 shows a number of\nother leaks in the line-log code).\n\nWhile this doubtless cleans up many leaks triggered by the test suite,\nthe only script which becomes leak-free is t4217, as it does very little\nbeyond a simple traversal (its existing leak was from the use of\n--children, which is now fixed).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n line-log.c           | 10 ++++++++++\n line-log.h           |  2 ++\n revision.c           |  9 +++++++++\n t/t4217-log-limit.sh |  1 +\n 4 files changed, 22 insertions(+)\n\ndiff --git a/line-log.c b/line-log.c\nindex 790ab73212..24a1ecb677 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -1327,3 +1327,13 @@ int line_log_filter(struct rev_info *rev)\n \n \treturn 0;\n }\n+\n+static void free_void_line_log_data(void *data)\n+{\n+\tfree_line_log_data(data);\n+}\n+\n+void line_log_free(struct rev_info *rev)\n+{\n+\tclear_decoration(&rev->line_log_data, free_void_line_log_data);\n+}\ndiff --git a/line-log.h b/line-log.h\nindex adff361b1b..4291da8d79 100644\n--- a/line-log.h\n+++ b/line-log.h\n@@ -60,4 +60,6 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev,\n \n int line_log_print(struct rev_info *rev, struct commit *commit);\n \n+void line_log_free(struct rev_info *rev);\n+\n #endif /* LINE_LOG_H */\ndiff --git a/revision.c b/revision.c\nindex e789834dd1..219dc76716 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3083,6 +3083,11 @@ static void release_revisions_mailmap(struct string_list *mailmap)\n \n static void release_revisions_topo_walk_info(struct topo_walk_info *info);\n \n+static void free_void_commit_list(void *list)\n+{\n+\tfree_commit_list(list);\n+}\n+\n void release_revisions(struct rev_info *revs)\n {\n \tfree_commit_list(revs->commits);\n@@ -3100,6 +3105,10 @@ void release_revisions(struct rev_info *revs)\n \tdiff_free(&revs->pruning);\n \treflog_walk_info_release(revs->reflog_info);\n \trelease_revisions_topo_walk_info(revs->topo_walk_info);\n+\tclear_decoration(&revs->children, free_void_commit_list);\n+\tclear_decoration(&revs->merge_simplification, free);\n+\tclear_decoration(&revs->treesame, free);\n+\tline_log_free(revs);\n }\n \n static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\ndiff --git a/t/t4217-log-limit.sh b/t/t4217-log-limit.sh\nindex 6e01e2629c..613f0710e9 100755\n--- a/t/t4217-log-limit.sh\n+++ b/t/t4217-log-limit.sh\n@@ -2,6 +2,7 @@\n \n test_description='git log with filter options limiting the output'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup test' '\n-- \n2.42.0.836.g4b88bf80c5\n\n"},{"id":"482723","messageId":"20231005213326.GC986467@coredump.intra.peff.net","threadId":"60311","inReplyTo":"20231005212802.GA982892@coredump.intra.peff.net","subject":"[PATCH 3/3] daemon: free listen_addr before returning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-05T21:33:26Z","receivedAt":"2023-10-05T21:33:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We build up a string list of listen addresses from the command-line\narguments, but never free it. This causes t5811 to complain of a leak\n(though curiously it seems to do so only when compiled with gcc, not\nwith clang).\n\nTo handle this correctly, we have to do a little refactoring:\n\n  - there are two exit points from the main function, depending on\n    whether we are entering the main loop or serving a single client\n    (since rather than a traditional fork model, we re-exec ourselves\n    with the extra \"--serve\" argument to accommodate Windows).\n\n    We don't need --listen at all in the --serve case, of course, but it\n    is passed along by the parent daemon, which simply copies all of the\n    command-line options it got.\n\n  - we just \"return serve()\" to run the main loop, giving us no chance\n    to do any cleanup\n\nSo let's use a \"ret\" variable to store the return code, and give\nourselves a single exit point at the end. That gives us one place to do\ncleanup.\n\nNote that this code also uses the \"use a no-dup string-list, but\nallocate strings we add to it\" trick, meaning string_list_clear() will\nnot realize it should free them. We can fix this by switching to a \"dup\"\nstring-list, but using the \"append_nodup\" function to add to it (this is\npreferable to tweaking the strdup_strings flag before clearing, as it\nputs all the subtle memory-ownership code together).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nDiff is a little messy due to indentation. Viewing with \"-w\" makes it\nmore clear, or just viewing the post-image.\n\n daemon.c                     | 37 ++++++++++++++++++++----------------\n t/t5811-proto-disable-git.sh |  2 ++\n 2 files changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex f5e597114b..17d331b2f3 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1243,19 +1243,20 @@ static int serve(struct string_list *listen_addr, int listen_port,\n int cmd_main(int argc, const char **argv)\n {\n \tint listen_port = 0;\n-\tstruct string_list listen_addr = STRING_LIST_INIT_NODUP;\n+\tstruct string_list listen_addr = STRING_LIST_INIT_DUP;\n \tint serve_mode = 0, inetd_mode = 0;\n \tconst char *pid_file = NULL, *user_name = NULL, *group_name = NULL;\n \tint detach = 0;\n \tstruct credentials *cred = NULL;\n \tint i;\n+\tint ret;\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tconst char *v;\n \n \t\tif (skip_prefix(arg, \"--listen=\", &v)) {\n-\t\t\tstring_list_append(&listen_addr, xstrdup_tolower(v));\n+\t\t\tstring_list_append_nodup(&listen_addr, xstrdup_tolower(v));\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(arg, \"--port=\", &v)) {\n@@ -1437,22 +1438,26 @@ int cmd_main(int argc, const char **argv)\n \t\t\tdie_errno(\"failed to redirect stderr to /dev/null\");\n \t}\n \n-\tif (inetd_mode || serve_mode)\n-\t\treturn execute();\n+\tif (inetd_mode || serve_mode) {\n+\t\tret = execute();\n+\t} else {\n+\t\tif (detach) {\n+\t\t\tif (daemonize())\n+\t\t\t\tdie(\"--detach not supported on this platform\");\n+\t\t}\n \n-\tif (detach) {\n-\t\tif (daemonize())\n-\t\t\tdie(\"--detach not supported on this platform\");\n-\t}\n+\t\tif (pid_file)\n+\t\t\twrite_file(pid_file, \"%\"PRIuMAX, (uintmax_t) getpid());\n \n-\tif (pid_file)\n-\t\twrite_file(pid_file, \"%\"PRIuMAX, (uintmax_t) getpid());\n+\t\t/* prepare argv for serving-processes */\n+\t\tstrvec_push(&cld_argv, argv[0]); /* git-daemon */\n+\t\tstrvec_push(&cld_argv, \"--serve\");\n+\t\tfor (i = 1; i < argc; ++i)\n+\t\t\tstrvec_push(&cld_argv, argv[i]);\n \n-\t/* prepare argv for serving-processes */\n-\tstrvec_push(&cld_argv, argv[0]); /* git-daemon */\n-\tstrvec_push(&cld_argv, \"--serve\");\n-\tfor (i = 1; i < argc; ++i)\n-\t\tstrvec_push(&cld_argv, argv[i]);\n+\t\tret = serve(&listen_addr, listen_port, cred);\n+\t}\n \n-\treturn serve(&listen_addr, listen_port, cred);\n+\tstring_list_clear(&listen_addr, 0);\n+\treturn ret;\n }\ndiff --git a/t/t5811-proto-disable-git.sh b/t/t5811-proto-disable-git.sh\nindex 8ac6b2a1d0..ed773e7432 100755\n--- a/t/t5811-proto-disable-git.sh\n+++ b/t/t5811-proto-disable-git.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='test disabling of git-over-tcp in clone/fetch'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY/lib-proto-disable.sh\"\n . \"$TEST_DIRECTORY/lib-git-daemon.sh\"\n-- \n2.42.0.836.g4b88bf80c5\n"},{"id":"482730","messageId":"xmqq34yoekt5.fsf@gitster.g","threadId":"60311","inReplyTo":"20231005213014.GB986467@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] revision: clear decoration structs during release_revisions()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-05T23:00:54Z","receivedAt":"2023-10-05T23:08:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\nWow, nested maze of callbacks make my head spin ;-) but they all\nlook reasonable.  Thanks.\n\n> diff --git a/line-log.c b/line-log.c\n> index 790ab73212..24a1ecb677 100644\n> --- a/line-log.c\n> +++ b/line-log.c\n> @@ -1327,3 +1327,13 @@ int line_log_filter(struct rev_info *rev)\n>  \n>  \treturn 0;\n>  }\n> +\n> +static void free_void_line_log_data(void *data)\n> +{\n> +\tfree_line_log_data(data);\n> +}\n> +\n> +void line_log_free(struct rev_info *rev)\n> +{\n> +\tclear_decoration(&rev->line_log_data, free_void_line_log_data);\n> +}\n> diff --git a/line-log.h b/line-log.h\n> index adff361b1b..4291da8d79 100644\n> --- a/line-log.h\n> +++ b/line-log.h\n> @@ -60,4 +60,6 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev,\n>  \n>  int line_log_print(struct rev_info *rev, struct commit *commit);\n>  \n> +void line_log_free(struct rev_info *rev);\n> +\n>  #endif /* LINE_LOG_H */\n> diff --git a/revision.c b/revision.c\n> index e789834dd1..219dc76716 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -3083,6 +3083,11 @@ static void release_revisions_mailmap(struct string_list *mailmap)\n>  \n>  static void release_revisions_topo_walk_info(struct topo_walk_info *info);\n>  \n> +static void free_void_commit_list(void *list)\n> +{\n> +\tfree_commit_list(list);\n> +}\n> +\n>  void release_revisions(struct rev_info *revs)\n>  {\n>  \tfree_commit_list(revs->commits);\n> @@ -3100,6 +3105,10 @@ void release_revisions(struct rev_info *revs)\n>  \tdiff_free(&revs->pruning);\n>  \treflog_walk_info_release(revs->reflog_info);\n>  \trelease_revisions_topo_walk_info(revs->topo_walk_info);\n> +\tclear_decoration(&revs->children, free_void_commit_list);\n> +\tclear_decoration(&revs->merge_simplification, free);\n> +\tclear_decoration(&revs->treesame, free);\n> +\tline_log_free(revs);\n>  }\n>  \n>  static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)\n> diff --git a/t/t4217-log-limit.sh b/t/t4217-log-limit.sh\n> index 6e01e2629c..613f0710e9 100755\n> --- a/t/t4217-log-limit.sh\n> +++ b/t/t4217-log-limit.sh\n> @@ -2,6 +2,7 @@\n>  \n>  test_description='git log with filter options limiting the output'\n>  \n> +TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n>  \n>  test_expect_success 'setup test' '\n"},{"id":"482733","messageId":"20231006005132.GA992085@coredump.intra.peff.net","threadId":"60311","inReplyTo":"xmqq34yoekt5.fsf@gitster.g","subject":"Re: [PATCH 2/3] revision: clear decoration structs during release_revisions()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-06T00:51:32Z","receivedAt":"2023-10-06T00:54:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 05, 2023 at 04:00:54PM -0700, Junio C Hamano wrote:\n\n> Wow, nested maze of callbacks make my head spin ;-) but they all\n> look reasonable.  Thanks.\n\nYeah, I don't love those one-liner callbacks just to handle the cast.\n\nThe other alternative is to write some kind of for_each_decoration()\nmacro, but I think it ends up in the usual macro hell (requiring the\ncaller to provide iterator variables, hanging half-open braces, and so\non). It might be worth it if iterating could be used in other places,\nbut I don't think it is.\n\nSo I tried to choose the lesser of two evils. :)\n\n-Peff\n"},{"id":"482740","messageId":"xmqqa5svbt3e.fsf@gitster.g","threadId":"60311","inReplyTo":"20231006005132.GA992085@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] revision: clear decoration structs during release_revisions()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-06T16:42:29Z","receivedAt":"2023-10-06T16:45:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Oct 05, 2023 at 04:00:54PM -0700, Junio C Hamano wrote:\n>\n>> Wow, nested maze of callbacks make my head spin ;-) but they all\n>> look reasonable.  Thanks.\n>\n> Yeah, I don't love those one-liner callbacks just to handle the cast.\n>\n> The other alternative is to write some kind of for_each_decoration()\n> macro, but I think it ends up in the usual macro hell (requiring the\n> caller to provide iterator variables, hanging half-open braces, and so\n> on). It might be worth it if iterating could be used in other places,\n> but I don't think it is.\n>\n> So I tried to choose the lesser of two evils. :)\n\nOh, I am happy with the result.\n"}]}