{"thread":{"id":"54645","subject":"[PATCH] chdir-notify: UNLEAK registrated callback entries","startedAt":"2020-11-14T21:40:30Z","lastAt":"2020-11-17T18:46:30Z","messageCount":8,"participants":["René Scharfe","Junio C Hamano","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"409965","messageId":"8a9cf9ba-f615-3ff5-8d35-c24c22d5d859@web.de","threadId":"54645","inReplyTo":null,"subject":"[PATCH] chdir-notify: UNLEAK registrated callback entries","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-14T21:40:01Z","receivedAt":"2020-11-14T21:40:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"chdir_notify_register() allows registering functions to notify when\nchdir() is called.  There is no way to unsubscribe or shut this\nmechanism down, so these entries are present until the program ends.\n\nValgrind reports allocations for these registrations as \"possibly lost\",\nprobably because it doesn't see through list.h's offsetof tricks.\nAnnotate them using UNLEAK, which causes Valgrind to report them as\n\"still reachable\" instead.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n chdir-notify.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/chdir-notify.c b/chdir-notify.c\nindex 5f7f2c2ac2..b236288416 100644\n--- a/chdir-notify.c\n+++ b/chdir-notify.c\n@@ -16,6 +16,7 @@ void chdir_notify_register(const char *name,\n \t\t\t   void *data)\n {\n \tstruct chdir_notify_entry *e = xmalloc(sizeof(*e));\n+\tUNLEAK(e);\n \te->name = name;\n \te->cb = cb;\n \te->data = data;\n--\n2.29.2\n"},{"id":"409966","messageId":"58f36a04-afa7-3cf3-ce0a-ad53004dd774@web.de","threadId":"54645","inReplyTo":"8a9cf9ba-f615-3ff5-8d35-c24c22d5d859@web.de","subject":"Re: [PATCH] chdir-notify: UNLEAK registrated callback entries","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-14T21:53:04Z","receivedAt":"2020-11-14T21:53:56Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.11.20 um 22:40 schrieb René Scharfe:\n> chdir_notify_register() allows registering functions to notify when\n> chdir() is called.  There is no way to unsubscribe or shut this\n> mechanism down, so these entries are present until the program ends.\n>\n> Valgrind reports allocations for these registrations as \"possibly lost\",\n> probably because it doesn't see through list.h's offsetof tricks.\n\nRight, avoiding list.h like below lets Valgrind classify the memory as\n\"still reachable\", without UNLEAK.  Not sure if it's worth it, though.\n\nNote my somewhat concerning knee-jerk reaction to write some macros. ;)\n\n---\n chdir-notify.c | 25 ++++++++++++++-----------\n 1 file changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/chdir-notify.c b/chdir-notify.c\nindex b236288416..4ce1f1120b 100644\n--- a/chdir-notify.c\n+++ b/chdir-notify.c\n@@ -1,26 +1,32 @@\n #include \"cache.h\"\n #include \"chdir-notify.h\"\n-#include \"list.h\"\n #include \"strbuf.h\"\n\n struct chdir_notify_entry {\n \tconst char *name;\n \tchdir_notify_callback cb;\n \tvoid *data;\n-\tstruct list_head list;\n };\n-static LIST_HEAD(chdir_notify_entries);\n+\n+#define VECTOR_TYPE(elemtype) struct { elemtype *v; size_t alloc, nr; }\n+#define VECTOR_FOR_EACH(x, p) for (p = (x)->v; p < (x)->v + (x)->nr; p++)\n+#define VECTOR_NEW_ENTRY(x, p) do {\t\t\t\\\n+\tALLOC_GROW_BY((x)->v, (x)->nr, 1, (x)->alloc);\t\\\n+\tp = (x)->v + (x)->nr - 1;\t\t\t\\\n+} while (0)\n+\n+static VECTOR_TYPE(struct chdir_notify_entry) chdir_notify_entries;\n\n void chdir_notify_register(const char *name,\n \t\t\t   chdir_notify_callback cb,\n \t\t\t   void *data)\n {\n-\tstruct chdir_notify_entry *e = xmalloc(sizeof(*e));\n-\tUNLEAK(e);\n+\tstruct chdir_notify_entry *e;\n+\n+\tVECTOR_NEW_ENTRY(&chdir_notify_entries, e);\n \te->name = name;\n \te->cb = cb;\n \te->data = data;\n-\tlist_add_tail(&e->list, &chdir_notify_entries);\n }\n\n static void reparent_cb(const char *name,\n@@ -52,7 +58,7 @@ void chdir_notify_reparent(const char *name, char **path)\n int chdir_notify(const char *new_cwd)\n {\n \tstruct strbuf old_cwd = STRBUF_INIT;\n-\tstruct list_head *pos;\n+\tstruct chdir_notify_entry *e;\n\n \tif (strbuf_getcwd(&old_cwd) < 0)\n \t\treturn -1;\n@@ -67,11 +73,8 @@ int chdir_notify(const char *new_cwd)\n \t\t\t \"setup: chdir from '%s' to '%s'\",\n \t\t\t old_cwd.buf, new_cwd);\n\n-\tlist_for_each(pos, &chdir_notify_entries) {\n-\t\tstruct chdir_notify_entry *e =\n-\t\t\tlist_entry(pos, struct chdir_notify_entry, list);\n+\tVECTOR_FOR_EACH(&chdir_notify_entries, e)\n \t\te->cb(e->name, old_cwd.buf, new_cwd, e->data);\n-\t}\n\n \tstrbuf_release(&old_cwd);\n \treturn 0;\n--\n2.29.2\n"},{"id":"410044","messageId":"xmqqzh3h6jz9.fsf@gitster.c.googlers.com","threadId":"54645","inReplyTo":"58f36a04-afa7-3cf3-ce0a-ad53004dd774@web.de","subject":"Re: [PATCH] chdir-notify: UNLEAK registrated callback entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-16T21:59:54Z","receivedAt":"2020-11-16T22:00:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Right, avoiding list.h like below lets Valgrind classify the memory as\n> \"still reachable\", without UNLEAK.  Not sure if it's worth it, though.\n>\n> Note my somewhat concerning knee-jerk reaction to write some macros. ;)\n\nWe have used such 3-tuple of <vector, alloc, nr> in many places from\nvery early time, and these macros might be worth using if we were to\nrewrite all of them.\n\nAs to this particular instance, given that the entries is either a\nlist or a vector of at most one element in practice, I do not think\nit matters very much either way.  Just a single UNLEAK is certainly\nsimpler ;-)\n\nThanks.\n\n\n\n\n>  struct chdir_notify_entry {\n>  \tconst char *name;\n>  \tchdir_notify_callback cb;\n>  \tvoid *data;\n> -\tstruct list_head list;\n>  };\n> -static LIST_HEAD(chdir_notify_entries);\n> +\n> +#define VECTOR_TYPE(elemtype) struct { elemtype *v; size_t alloc, nr; }\n> +#define VECTOR_FOR_EACH(x, p) for (p = (x)->v; p < (x)->v + (x)->nr; p++)\n> +#define VECTOR_NEW_ENTRY(x, p) do {\t\t\t\\\n> +\tALLOC_GROW_BY((x)->v, (x)->nr, 1, (x)->alloc);\t\\\n> +\tp = (x)->v + (x)->nr - 1;\t\t\t\\\n> +} while (0)\n> +\n> +static VECTOR_TYPE(struct chdir_notify_entry) chdir_notify_entries;\n>\n>  void chdir_notify_register(const char *name,\n>  \t\t\t   chdir_notify_callback cb,\n>  \t\t\t   void *data)\n>  {\n> -\tstruct chdir_notify_entry *e = xmalloc(sizeof(*e));\n> -\tUNLEAK(e);\n> +\tstruct chdir_notify_entry *e;\n> +\n> +\tVECTOR_NEW_ENTRY(&chdir_notify_entries, e);\n>  \te->name = name;\n>  \te->cb = cb;\n>  \te->data = data;\n> -\tlist_add_tail(&e->list, &chdir_notify_entries);\n>  }\n>\n>  static void reparent_cb(const char *name,\n> @@ -52,7 +58,7 @@ void chdir_notify_reparent(const char *name, char **path)\n>  int chdir_notify(const char *new_cwd)\n>  {\n>  \tstruct strbuf old_cwd = STRBUF_INIT;\n> -\tstruct list_head *pos;\n> +\tstruct chdir_notify_entry *e;\n>\n>  \tif (strbuf_getcwd(&old_cwd) < 0)\n>  \t\treturn -1;\n> @@ -67,11 +73,8 @@ int chdir_notify(const char *new_cwd)\n>  \t\t\t \"setup: chdir from '%s' to '%s'\",\n>  \t\t\t old_cwd.buf, new_cwd);\n>\n> -\tlist_for_each(pos, &chdir_notify_entries) {\n> -\t\tstruct chdir_notify_entry *e =\n> -\t\t\tlist_entry(pos, struct chdir_notify_entry, list);\n> +\tVECTOR_FOR_EACH(&chdir_notify_entries, e)\n>  \t\te->cb(e->name, old_cwd.buf, new_cwd, e->data);\n> -\t}\n>\n>  \tstrbuf_release(&old_cwd);\n>  \treturn 0;\n> --\n> 2.29.2\n"},{"id":"410051","messageId":"20201117002435.GA13516@coredump.intra.peff.net","threadId":"54645","inReplyTo":"8a9cf9ba-f615-3ff5-8d35-c24c22d5d859@web.de","subject":"Re: [PATCH] chdir-notify: UNLEAK registrated callback entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-17T00:24:35Z","receivedAt":"2020-11-17T00:24:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 14, 2020 at 10:40:01PM +0100, René Scharfe wrote:\n\n> chdir_notify_register() allows registering functions to notify when\n> chdir() is called.  There is no way to unsubscribe or shut this\n> mechanism down, so these entries are present until the program ends.\n> \n> Valgrind reports allocations for these registrations as \"possibly lost\",\n> probably because it doesn't see through list.h's offsetof tricks.\n> Annotate them using UNLEAK, which causes Valgrind to report them as\n> \"still reachable\" instead.\n\nI can't say I'm excited to see UNLEAK used here. It was really intended\nfor items going out of scope that weren't worth cleaning up. But here\nwe're papering over a failure in the memory checking tool for something\nthat _is_ in scope.\n\nI guess I'm not too surprised that valgrind has trouble with list.h. We\nhave pointers into a heap-allocated block, but not the start of it.\nCuriously, ASan/LSan get this case right. So my first instinct is: use\nthose tools, they're better. :)\n\nIf we did want to paper over this case for valgrind, I think this is a\nbetter way to do so:\n\ndiff --git a/chdir-notify.c b/chdir-notify.c\nindex 5f7f2c2ac2..ddfe703b1a 100644\n--- a/chdir-notify.c\n+++ b/chdir-notify.c\n@@ -4,10 +4,10 @@\n #include \"strbuf.h\"\n \n struct chdir_notify_entry {\n+\tstruct list_head list;\n \tconst char *name;\n \tchdir_notify_callback cb;\n \tvoid *data;\n-\tstruct list_head list;\n };\n static LIST_HEAD(chdir_notify_entries);\n \n\nI also wonder if valgrind _is_ aware of the distinction, and that's why\nthese show up as only \"possibly lost\". And indeed, the faq[1] says:\n\n - \"possibly lost\" means your program is leaking memory, unless you're\n   doing unusual things with pointers that could cause them to point\n   into the middle of an allocated block; see the user manual for some\n   possible causes. Use --show-possibly-lost=no if you don't want to see\n   these reports.\n\nand the user manual[2] has a more elaborate example that calls these\n\"interior pointers\". So I think that's exactly what is going on here.\n\nBut then I'm not sure why we'd want this patch. List pointers (and now\nhashmap entries, which also contain a linked-list chain) are used in\nlots of data structures. Fixing this one case manually is not that\ninteresting. If we're going to use valgrind, we probably need to accept\nthat its \"possibly lost\" distinction is not useful for our code and turn\nit off.\n\n-Peff\n\n[1] https://valgrind.org/docs/manual/faq.html#faq.deflost\n\n[2] https://valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks\n"},{"id":"410065","messageId":"20201117044923.3703483-1-newren@gmail.com","threadId":"54645","inReplyTo":"58f36a04-afa7-3cf3-ce0a-ad53004dd774@web.de","subject":"Re: [PATCH] chdir-notify: UNLEAK registrated callback entries","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-11-17T04:49:23Z","receivedAt":"2020-11-17T04:49:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Sat, Nov 14, 2020 at 1:55 PM René Scharfe <l.s.r@web.de> wrote:\n>\n> Am 14.11.20 um 22:40 schrieb René Scharfe:\n> > chdir_notify_register() allows registering functions to notify when\n> > chdir() is called.  There is no way to unsubscribe or shut this\n> > mechanism down, so these entries are present until the program ends.\n> >\n> > Valgrind reports allocations for these registrations as \"possibly lost\",\n> > probably because it doesn't see through list.h's offsetof tricks.\n>\n> Right, avoiding list.h like below lets Valgrind classify the memory as\n> \"still reachable\", without UNLEAK.  Not sure if it's worth it, though.\n>\n> Note my somewhat concerning knee-jerk reaction to write some macros. ;)\n\nI've got a local patch that changes these to actually be freed as yet\nanother route we could go.  It's mixed with a few (half-baked)\nrevision-related memory cleanups that should be separated, and I'm\nunsure if there are other places that would need a finalize_globals()\ncall too, but it's another approach to consider...\n\n\n-- >8 --\nSubject: [RFC PATCH] Make merge-ort run leak check free\n\nTweaks to revision walking, adding a chdir_deregister_all() (so that all\nwatchers can be cleared as well as free'ing their associated memory),\nand maybe other stuff.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/fast-rebase.c |  2 ++\n cache.h               |  3 +++\n chdir-notify.c        | 15 +++++++++++++++\n chdir-notify.h        |  5 +++++\n git.c                 | 10 ++++++++--\n revision.c            | 11 +++++++++++\n revision.h            |  6 ++++++\n setup.c               |  5 +++++\n 8 files changed, 55 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fast-rebase.c b/builtin/fast-rebase.c\nindex 19333ca42d..32cab36ad3 100644\n--- a/builtin/fast-rebase.c\n+++ b/builtin/fast-rebase.c\n@@ -173,6 +173,8 @@ int cmd_fast_rebase(int argc, const char **argv, const char *prefix)\n \t\tlast_commit = create_commit(result.tree, commit, last_commit);\n \t}\n \tfprintf(stderr, \"\\nDone.\\n\");\n+\trev_info_free(&revs);\n+\tmemset(&revs, 0, sizeof(revs));\n \n \tmerge_switch_to_result(&merge_opt, head_tree, &result, 1, !result.clean);\n \ndiff --git a/cache.h b/cache.h\nindex c0072d43b1..b9b543f75b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -604,6 +604,9 @@ const char *setup_git_directory(void);\n char *prefix_path(const char *prefix, int len, const char *path);\n char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path);\n \n+/* Release resources related to global config. */\n+void finalize_globals(void);\n+\n /*\n  * Concatenate \"prefix\" (if len is non-zero) and \"path\", with no\n  * connecting characters (so \"prefix\" should end with a \"/\").\ndiff --git a/chdir-notify.c b/chdir-notify.c\nindex 5f7f2c2ac2..ea7e52cfea 100644\n--- a/chdir-notify.c\n+++ b/chdir-notify.c\n@@ -91,3 +91,18 @@ char *reparent_relative_path(const char *old_cwd,\n \n \treturn ret;\n }\n+\n+void chdir_deregister_all(void)\n+{\n+\tstruct list_head *pos, *tmp;\n+\n+\tif (list_empty(&chdir_notify_entries))\n+\t\treturn;\n+\n+\tlist_for_each_safe(pos, tmp, &chdir_notify_entries) {\n+\t\tstruct chdir_notify_entry *e =\n+\t\t\tlist_entry(pos, struct chdir_notify_entry, list);\n+\t\tfree(e);\n+\t}\n+\tmemset(&chdir_notify_entries, 0, sizeof(chdir_notify_entries));\n+}\ndiff --git a/chdir-notify.h b/chdir-notify.h\nindex 366e4c1ee9..0cd3e1f633 100644\n--- a/chdir-notify.h\n+++ b/chdir-notify.h\n@@ -70,4 +70,9 @@ char *reparent_relative_path(const char *old_cwd,\n \t\t\t     const char *new_cwd,\n \t\t\t     const char *path);\n \n+/*\n+ * Deregister all subscribers and free any associated memory.\n+ */\n+void chdir_deregister_all(void);\n+\n #endif /* CHDIR_NOTIFY_H */\ndiff --git a/git.c b/git.c\nindex af84f11e69..4b04988909 100644\n--- a/git.c\n+++ b/git.c\n@@ -358,6 +358,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n \n \t\t\tret = run_command(&child);\n+\t\t\tfinalize_globals();\n \t\t\tif (ret >= 0)   /* normal exit */\n \t\t\t\texit(ret);\n \n@@ -697,8 +698,11 @@ static void handle_builtin(int argc, const char **argv)\n \t}\n \n \tbuiltin = get_builtin(cmd);\n-\tif (builtin)\n-\t\texit(run_builtin(builtin, argc, argv));\n+\tif (builtin) {\n+\t\tint result = run_builtin(builtin, argc, argv);\n+\t\tfinalize_globals();\n+\t\texit(result);\n+\t}\n \tstrvec_clear(&args);\n }\n \n@@ -742,6 +746,7 @@ static void execv_dashed_external(const char **argv)\n \t * generic string as our trace2 command verb to indicate that we\n \t * launched a dashed command.\n \t */\n+\tfinalize_globals();\n \tif (status >= 0)\n \t\texit(status);\n \telse if (errno != ENOENT)\n@@ -796,6 +801,7 @@ static int run_argv(int *argcp, const char ***argv)\n \t\t\t */\n \t\t\ti = run_command_v_opt_tr2(args.v, RUN_SILENT_EXEC_FAILURE |\n \t\t\t\t\t\t  RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, \"git_alias\");\n+\t\t\tfinalize_globals();\n \t\t\tif (i >= 0 || errno != ENOENT)\n \t\t\t\texit(i);\n \t\t\tdie(\"could not execute builtin %s\", **argv);\ndiff --git a/revision.c b/revision.c\nindex c6e169e3eb..b4f276d854 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1419,6 +1419,8 @@ static int limit_list(struct rev_info *revs)\n \t\t\tslop = still_interesting(list, date, slop, &interesting_cache);\n \t\t\tif (slop)\n \t\t\t\tcontinue;\n+\t\t\tfree_commit_list(list);\n+\t\t\trevs->commits = NULL;\n \t\t\tbreak;\n \t\t}\n \t\tif (revs->min_age != -1 && (commit->date > revs->min_age) &&\n@@ -3281,6 +3283,15 @@ static void set_children(struct rev_info *revs)\n \t}\n }\n \n+void rev_info_free(struct rev_info *revs)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < revs->cmdline.nr; i++)\n+\t\tfree((char*)revs->cmdline.rev[i].name);\n+\tfree(revs->cmdline.rev);\n+}\n+\n void reset_revision_walk(void)\n {\n \tclear_object_flags(SEEN | ADDED | SHOWN | TOPO_WALK_EXPLORED | TOPO_WALK_INDEGREE);\ndiff --git a/revision.h b/revision.h\nindex f6bf860d19..5dd98f2f6d 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -362,6 +362,12 @@ void repo_init_revisions(struct repository *r,\n int setup_revisions(int argc, const char **argv, struct rev_info *revs,\n \t\t    struct setup_revision_opt *);\n \n+/**\n+ * Free information associated with a rev_info once no longer needed.\n+ * Will not free revs itself.\n+ */\n+void rev_info_free(struct rev_info *revs);\n+\n void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \t\t\tconst struct option *options,\n \t\t\tconst char * const usagestr[]);\ndiff --git a/setup.c b/setup.c\nindex c04cd25a30..cd958f5f5c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1411,6 +1411,11 @@ void sanitize_stdfds(void)\n \t\tclose(fd);\n }\n \n+void finalize_globals(void)\n+{\n+\tchdir_deregister_all();\n+}\n+\n int daemonize(void)\n {\n #ifdef NO_POSIX_GOODIES\n-- \n2.29.2.529.gc090dab8c5\n\n"},{"id":"410072","messageId":"20201117065304.GA41136@coredump.intra.peff.net","threadId":"54645","inReplyTo":"20201117044923.3703483-1-newren@gmail.com","subject":"Re: [PATCH] chdir-notify: UNLEAK registrated callback entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-17T06:53:04Z","receivedAt":"2020-11-17T06:53:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 16, 2020 at 08:49:23PM -0800, Elijah Newren wrote:\n\n> > Right, avoiding list.h like below lets Valgrind classify the memory as\n> > \"still reachable\", without UNLEAK.  Not sure if it's worth it, though.\n> >\n> > Note my somewhat concerning knee-jerk reaction to write some macros. ;)\n> \n> I've got a local patch that changes these to actually be freed as yet\n> another route we could go.  It's mixed with a few (half-baked)\n> revision-related memory cleanups that should be separated, and I'm\n> unsure if there are other places that would need a finalize_globals()\n> call too, but it's another approach to consider...\n\nHere are a few thoughts...\n\n> diff --git a/builtin/fast-rebase.c b/builtin/fast-rebase.c\n> index 19333ca42d..32cab36ad3 100644\n> --- a/builtin/fast-rebase.c\n> +++ b/builtin/fast-rebase.c\n> @@ -173,6 +173,8 @@ int cmd_fast_rebase(int argc, const char **argv, const char *prefix)\n>  \t\tlast_commit = create_commit(result.tree, commit, last_commit);\n>  \t}\n>  \tfprintf(stderr, \"\\nDone.\\n\");\n> +\trev_info_free(&revs);\n> +\tmemset(&revs, 0, sizeof(revs));\n\nThis one could easily be an UNLEAK(), since we're presumably exiting\nfrom cmd_fast_rebase() soon after. But I certainly don't mind a real\ncleanup.\n\nProbably rev_info_free() should leave the rev_info in a reasonable state\n(so callers don't have to re-zero it).\n\n> diff --git a/cache.h b/cache.h\n> index c0072d43b1..b9b543f75b 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -604,6 +604,9 @@ const char *setup_git_directory(void);\n>  char *prefix_path(const char *prefix, int len, const char *path);\n>  char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path);\n>  \n> +/* Release resources related to global config. */\n> +void finalize_globals(void);\n\nYuck. Remembering to put things in this, and remembering to call it in\nall of the right places seems ugly.\n\nIf we want to go this route, then I think individual pieces of code\ncould register their own atexit() handlers to clean up. But...\n\n> +void chdir_deregister_all(void)\n> +{\n> +\tstruct list_head *pos, *tmp;\n> +\n> +\tif (list_empty(&chdir_notify_entries))\n> +\t\treturn;\n> +\n> +\tlist_for_each_safe(pos, tmp, &chdir_notify_entries) {\n> +\t\tstruct chdir_notify_entry *e =\n> +\t\t\tlist_entry(pos, struct chdir_notify_entry, list);\n> +\t\tfree(e);\n> +\t}\n> +\tmemset(&chdir_notify_entries, 0, sizeof(chdir_notify_entries));\n> +}\n\nI disagree that this is actually a leak. The memory is still referenced\nby the list, just like a ton of other global variables (e.g., quite a\nlot of config or environment variables that are loaded throughout a\nnormal program).\n\nSo my inclination is to leave referenced globals like this. If there are\ntools that are confused by our linked list structure, then we should\nadjust our tools (either using ASan, or turning off valgrind's \"possibly\nlost\" report).\n\n(My preference is to use ASan in general, because it's _so_ much\nfaster. Running the whole suite under valgrind is pretty painful, and\nideally we'd eventually hit a point where it was possible to run the\nwhole suite with leak-checking enabled).\n\n> diff --git a/git.c b/git.c\n> index af84f11e69..4b04988909 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -358,6 +358,7 @@ static int handle_alias(int *argcp, const char ***argv)\n>  \t\t\ttrace2_cmd_name(\"_run_shell_alias_\");\n>  \n>  \t\t\tret = run_command(&child);\n> +\t\t\tfinalize_globals();\n>  \t\t\tif (ret >= 0)   /* normal exit */\n>  \t\t\t\texit(ret);\n\nI thought common-main.c might be a better place to put something like\nthis, but any time we exit() it gets weird (of course, atexit() would\ntake care of this).\n\nOTOH, if we call exit() then most things are still on the stack (or in\nglobals), so we're not actually leaking those things. I think the root\nof this chdir-notify issue is not that we don't still have a handle to\nthe memory, but that the linked list confuses valgrind.  But the same\nwould be true if we were holding another linked list (or hashmap) in a\nstack variable, referenced via the heap, etc, and called exit().\n\n> +void rev_info_free(struct rev_info *revs)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < revs->cmdline.nr; i++)\n> +\t\tfree((char*)revs->cmdline.rev[i].name);\n> +\tfree(revs->cmdline.rev);\n> +}\n\nI suspect there's quite a bit more needed here (René already found a\ncase where pathspecs needed to be freed). But I don't mind if we start\nsmall and add to it as leak-checking notices items.\n\n-Peff\n"},{"id":"410076","messageId":"CABPp-BGNws7mdnBL2j-hvh1fVwxNjJ1scynzUZtMWtEtJTdkDA@mail.gmail.com","threadId":"54645","inReplyTo":"20201117065304.GA41136@coredump.intra.peff.net","subject":"Re: [PATCH] chdir-notify: UNLEAK registrated callback entries","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-11-17T08:39:02Z","receivedAt":"2020-11-17T08:40:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 16, 2020 at 10:53 PM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, Nov 16, 2020 at 08:49:23PM -0800, Elijah Newren wrote:\n>\n> > > Right, avoiding list.h like below lets Valgrind classify the memory as\n> > > \"still reachable\", without UNLEAK.  Not sure if it's worth it, though.\n> > >\n> > > Note my somewhat concerning knee-jerk reaction to write some macros. ;)\n> >\n> > I've got a local patch that changes these to actually be freed as yet\n> > another route we could go.  It's mixed with a few (half-baked)\n> > revision-related memory cleanups that should be separated, and I'm\n> > unsure if there are other places that would need a finalize_globals()\n> > call too, but it's another approach to consider...\n>\n> Here are a few thoughts...\n>\n> > diff --git a/builtin/fast-rebase.c b/builtin/fast-rebase.c\n> > index 19333ca42d..32cab36ad3 100644\n> > --- a/builtin/fast-rebase.c\n> > +++ b/builtin/fast-rebase.c\n> > @@ -173,6 +173,8 @@ int cmd_fast_rebase(int argc, const char **argv, const char *prefix)\n> >               last_commit = create_commit(result.tree, commit, last_commit);\n> >       }\n> >       fprintf(stderr, \"\\nDone.\\n\");\n> > +     rev_info_free(&revs);\n> > +     memset(&revs, 0, sizeof(revs));\n>\n> This one could easily be an UNLEAK(), since we're presumably exiting\n> from cmd_fast_rebase() soon after. But I certainly don't mind a real\n> cleanup.\n>\n> Probably rev_info_free() should leave the rev_info in a reasonable state\n> (so callers don't have to re-zero it).\n>\n> > diff --git a/cache.h b/cache.h\n> > index c0072d43b1..b9b543f75b 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -604,6 +604,9 @@ const char *setup_git_directory(void);\n> >  char *prefix_path(const char *prefix, int len, const char *path);\n> >  char *prefix_path_gently(const char *prefix, int len, int *remaining, const char *path);\n> >\n> > +/* Release resources related to global config. */\n> > +void finalize_globals(void);\n>\n> Yuck. Remembering to put things in this, and remembering to call it in\n> all of the right places seems ugly.\n>\n> If we want to go this route, then I think individual pieces of code\n> could register their own atexit() handlers to clean up. But...\n>\n> > +void chdir_deregister_all(void)\n> > +{\n> > +     struct list_head *pos, *tmp;\n> > +\n> > +     if (list_empty(&chdir_notify_entries))\n> > +             return;\n> > +\n> > +     list_for_each_safe(pos, tmp, &chdir_notify_entries) {\n> > +             struct chdir_notify_entry *e =\n> > +                     list_entry(pos, struct chdir_notify_entry, list);\n> > +             free(e);\n> > +     }\n> > +     memset(&chdir_notify_entries, 0, sizeof(chdir_notify_entries));\n> > +}\n>\n> I disagree that this is actually a leak. The memory is still referenced\n> by the list, just like a ton of other global variables (e.g., quite a\n> lot of config or environment variables that are loaded throughout a\n> normal program).\n>\n> So my inclination is to leave referenced globals like this. If there are\n> tools that are confused by our linked list structure, then we should\n> adjust our tools (either using ASan, or turning off valgrind's \"possibly\n> lost\" report).\n>\n> (My preference is to use ASan in general, because it's _so_ much\n> faster. Running the whole suite under valgrind is pretty painful, and\n> ideally we'd eventually hit a point where it was possible to run the\n> whole suite with leak-checking enabled).\n\nI'm aware of the differences between \"definitely lossed\" and \"still\nreachable\".  And yes, the definitely lost are more important to clean\nup.  But I'm not convinced that \"still reachable\" are always okay; I\nthink they sometimes point to latent bugs and other issues that might\nmanifest as leaks with slightly different workloads.  I also think\nthat sometimes these are easier to track down than some of the actual\nleaks, but the work to free them and their associated structures will\nnaturally clear up some of the actual leaks.  Neither of those two\ncases apply here, of course, because we've done the auditing to know\nwhy these particular \"possibly lost\"/\"still reachable\" references\nexist.  But if you have a lot of noise in the \"possibly lost\" and\n\"still reachable\" categories, it makes it unlikely you'll want to go\nthrough the remainder to find other useful signal.\n\nAll that said, I've been sitting on this and a bunch of other patches\nfor months, because I agree with some of your comments on the ugliness\nof the code, and because when I started to go down the rabbit hole for\ncleaning up rev_info I found it was not only ugly but also a very deep\nhole...and it was measurably slowing down my fast-rebase testcases too\nwhich was demotivating.\n\n\nRunning the whole testsuite under ASan sounds great, btw.\n\n> > diff --git a/git.c b/git.c\n> > index af84f11e69..4b04988909 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -358,6 +358,7 @@ static int handle_alias(int *argcp, const char ***argv)\n> >                       trace2_cmd_name(\"_run_shell_alias_\");\n> >\n> >                       ret = run_command(&child);\n> > +                     finalize_globals();\n> >                       if (ret >= 0)   /* normal exit */\n> >                               exit(ret);\n>\n> I thought common-main.c might be a better place to put something like\n> this, but any time we exit() it gets weird (of course, atexit() would\n> take care of this).\n>\n> OTOH, if we call exit() then most things are still on the stack (or in\n> globals), so we're not actually leaking those things. I think the root\n> of this chdir-notify issue is not that we don't still have a handle to\n> the memory, but that the linked list confuses valgrind.  But the same\n> would be true if we were holding another linked list (or hashmap) in a\n> stack variable, referenced via the heap, etc, and called exit().\n>\n> > +void rev_info_free(struct rev_info *revs)\n> > +{\n> > +     int i;\n> > +\n> > +     for (i = 0; i < revs->cmdline.nr; i++)\n> > +             free((char*)revs->cmdline.rev[i].name);\n> > +     free(revs->cmdline.rev);\n> > +}\n>\n> I suspect there's quite a bit more needed here (René already found a\n> case where pathspecs needed to be freed). But I don't mind if we start\n> small and add to it as leak-checking notices items.\n\nYeah, I had other patches that added on to it; I didn't have tests\nquite passing so I #ifdef'd out some blocks while testing to return\nsome leaks but undo some of the breaks.  Anyway, I had something that\nlooked like\n\n+void rev_info_free(struct rev_info *revs)\n+{\n+       int i;\n+\n+#if 0\n+       if (revs->commits)\n+               free_commit_list(revs->commits);\n+#endif\n+\n+       object_array_clear(&revs->boundary_commits);\n+\n+       for (i = 0; i < revs->cmdline.nr; i++) {\n+#if 0\n+               /* This block causes double free for 'git fetch origin' */\n+               struct object *obj = revs->cmdline.rev[i].item;\n+               if (obj->type == OBJ_COMMIT)\n+                       free_commit_list(((struct commit *)obj)->parents);\n+#endif\n+               free((char*)revs->cmdline.rev[i].name);\n+       }\n+       free(revs->cmdline.rev);\n+\n+       clear_pathspec(&revs->prune_data);\n+       clear_pathspec(&revs->pruning.pathspec);\n+       if (revs->graph)\n+               graph_free(revs->graph);\n+       free(revs->graph);\n+       clear_pathspec(&revs->diffopt.pathspec);\n+\n+       if (revs->reflog_info)\n+               reflog_walk_free(&revs->reflog_info);\n+\n+       clear_decorations(&revs->children, 1);\n+       clear_decorations(&revs->merge_simplification, 1);\n+       clear_decorations(&revs->treesame, 1);\n+       free_saved_parents(revs);\n+       free(revs->saved_parents_slab);\n+}\n\nbut, not only was it not passing tests, I knew there were other things\nto free still that I hadn't gotten too.  Also, you'll note that a\nnumber of functions listed above don't exist; I had to add them.\n\n\nSo yeah, trying to clean up rev_info will be a bit of work.  I'd be\nhappy to hand off what I do have if anyone is interested; I doubt I'll\nget back to it soon.\n"},{"id":"410116","messageId":"202c4b1a-efb7-ee70-a287-567e651ff25e@web.de","threadId":"54645","inReplyTo":"20201117002435.GA13516@coredump.intra.peff.net","subject":"Re: [PATCH] chdir-notify: UNLEAK registrated callback entries","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-11-17T18:45:42Z","receivedAt":"2020-11-17T18:46:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.11.20 um 01:24 schrieb Jeff King:\n> On Sat, Nov 14, 2020 at 10:40:01PM +0100, René Scharfe wrote:\n>\n>> chdir_notify_register() allows registering functions to notify when\n>> chdir() is called.  There is no way to unsubscribe or shut this\n>> mechanism down, so these entries are present until the program ends.\n>>\n>> Valgrind reports allocations for these registrations as \"possibly lost\",\n>> probably because it doesn't see through list.h's offsetof tricks.\n>> Annotate them using UNLEAK, which causes Valgrind to report them as\n>> \"still reachable\" instead.\n>\n> I can't say I'm excited to see UNLEAK used here. It was really intended\n> for items going out of scope that weren't worth cleaning up. But here\n> we're papering over a failure in the memory checking tool for something\n> that _is_ in scope.\n\nRight, though I'd consider every use of UNLEAK as \"papering over\".  And\nnot being able to distinguish with certainty between pointer trickery\nand accidents is more of a missing (or impossible) feature than a\nfailure in my book.\n\n> I guess I'm not too surprised that valgrind has trouble with list.h. We\n> have pointers into a heap-allocated block, but not the start of it.\n> Curiously, ASan/LSan get this case right. So my first instinct is: use\n> those tools, they're better. :)\n\nDoes Leak Sanitizer get it right or is it just as unsure, but reports\nits findings more cautiously?\n\n> If we did want to paper over this case for valgrind, I think this is a\n> better way to do so:\n>\n> diff --git a/chdir-notify.c b/chdir-notify.c\n> index 5f7f2c2ac2..ddfe703b1a 100644\n> --- a/chdir-notify.c\n> +++ b/chdir-notify.c\n> @@ -4,10 +4,10 @@\n>  #include \"strbuf.h\"\n>\n>  struct chdir_notify_entry {\n> +\tstruct list_head list;\n>  \tconst char *name;\n>  \tchdir_notify_callback cb;\n>  \tvoid *data;\n> -\tstruct list_head list;\n>  };\n>  static LIST_HEAD(chdir_notify_entries);\n\nA trick to hide the trick -- I like it.\n\n> I also wonder if valgrind _is_ aware of the distinction, and that's why\n> these show up as only \"possibly lost\". And indeed, the faq[1] says:\n>\n>  - \"possibly lost\" means your program is leaking memory, unless you're\n>    doing unusual things with pointers that could cause them to point\n>    into the middle of an allocated block; see the user manual for some\n>    possible causes. Use --show-possibly-lost=no if you don't want to see\n>    these reports.\n>\n> and the user manual[2] has a more elaborate example that calls these\n> \"interior pointers\". So I think that's exactly what is going on here.\n>\n> But then I'm not sure why we'd want this patch. List pointers (and now\n> hashmap entries, which also contain a linked-list chain) are used in\n> lots of data structures. Fixing this one case manually is not that\n> interesting. If we're going to use valgrind, we probably need to accept\n> that its \"possibly lost\" distinction is not useful for our code and turn\n> it off.\n\nThat would probably disregard real issues as well -- but it's hard to\nknow how many without finding and classifying them.\n\nLists (and hashmaps) have cleanup methods that we could call, as Elijah\nnoted.  We can skip that in main() functions for production builds, but\ncutting out the noise when SUPPRESS_ANNOTATED_LEAKS is set would surely\nmake finding real leaks easier.\n\nRené\n"}]}