{"thread":{"id":"54053","subject":"[PATCH 0/3] Clean up some memory leaks in and around dir.c","startedAt":"2020-08-16T06:59:16Z","lastAt":"2020-08-19T13:51:44Z","messageCount":15,"participants":["Elijah Newren via GitGitGadget","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"403786","messageId":"pull.831.git.git.1597561152.gitgitgadget@gmail.com","threadId":"54053","inReplyTo":null,"subject":"[PATCH 0/3] Clean up some memory leaks in and around dir.c","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-16T06:59:08Z","receivedAt":"2020-08-16T06:59:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Some memory leaks in dir.c were making it hard for me to find my own leaks\nin merge-ort. I followed a few threads and tried to both clean up the leaks\nand make the API less prone to incorrect use.\n\nElijah Newren (3):\n  dir: fix leak of parent_hashmap and recursive_hashmap\n  dir: make clear_directory() free all relevant memory\n  dir: fix problematic API to avoid memory leaks\n\n builtin/add.c          |  4 ++--\n builtin/check-ignore.c |  4 ++--\n builtin/clean.c        | 12 ++++--------\n builtin/grep.c         |  3 ++-\n builtin/ls-files.c     |  4 ++--\n builtin/stash.c        |  7 ++-----\n dir.c                  | 23 ++++++++++++++++++-----\n dir.h                  | 21 +++++++++++----------\n merge.c                |  3 ++-\n wt-status.c            |  8 ++------\n 10 files changed, 47 insertions(+), 42 deletions(-)\n\n\nbase-commit: 878e727637ec5815ccb3301eb994a54df95b21b8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-831%2Fnewren%2Fdir_memory_cleanup-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-831/newren/dir_memory_cleanup-v1\nPull-Request: https://github.com/git/git/pull/831\n-- \ngitgitgadget\n"},{"id":"403787","messageId":"932741d7598ca2934dbca40f715ba2d3819fcc51.1597561152.git.gitgitgadget@gmail.com","threadId":"54053","inReplyTo":"pull.831.git.git.1597561152.gitgitgadget@gmail.com","subject":"[PATCH 1/3] dir: fix leak of parent_hashmap and recursive_hashmap","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-16T06:59:09Z","receivedAt":"2020-08-16T06:59:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nCommit 96cc8ab531 (\"sparse-checkout: use hashmaps for cone patterns\",\n2019-11-21) added a parent_hashmap and recursive_hashmap to each struct\npattern_list and initialized these in add_patterns_from_buffer() but did\nnot make sure to add necessary code to clear_pattern_list() to free\nthese new structures.  Call hashmap_free_() on each to plug this memory\nleak.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n dir.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/dir.c b/dir.c\nindex fe64be30ed..08df469bf7 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -916,6 +916,10 @@ void clear_pattern_list(struct pattern_list *pl)\n \t\tfree(pl->patterns[i]);\n \tfree(pl->patterns);\n \tfree(pl->filebuf);\n+\thashmap_free_(&pl->parent_hashmap,\n+\t\t      offsetof(struct pattern_entry, ent));\n+\thashmap_free_(&pl->recursive_hashmap,\n+\t\t      offsetof(struct pattern_entry, ent));\n \n \tmemset(pl, 0, sizeof(*pl));\n }\n-- \ngitgitgadget\n\n"},{"id":"403788","messageId":"b9310e9941e91336258edd97a913e5908180720e.1597561152.git.gitgitgadget@gmail.com","threadId":"54053","inReplyTo":"pull.831.git.git.1597561152.gitgitgadget@gmail.com","subject":"[PATCH 3/3] dir: fix problematic API to avoid memory leaks","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-16T06:59:11Z","receivedAt":"2020-08-16T06:59:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nTwo commits ago, we noticed that parent_hashmap and recursive_hashmap\nwere being leaked and fixed it by making clear_pattern_list() free those\nhashmaps.  One commit ago, we noticed that clear_directory() was only\ntaking responsibility for a subset of fields within dir_struct, despite\nthe fact that entries[] and ignored[] we allocated internally to dir.c,\nresulting in many callers either leaking or haphazardly trying to free\nthese arrays and their contents.\n\nDigging further, I found that despite the pretty clear documentation\nnear the top of dir.h that folks were supposed to call clear_directory()\nwhen the user no longer needed the dir_struct, there were four callers\nthat didn't bother doing that at all.  However, two of them clearly\nthought about leaks since they had an UNLEAK(dir) directive, which to me\nsuggests that the method to free the data was too unclear.  I suspect\nthe non-obviousness of the API and its holes led folks to avoid it,\nwhich then snowballed into further problems with the entries[],\nignored[], parent_hashmap, and recursive_hashmap problems.\n\nRename clear_directory() to dir_free() to be more in line with other\ndata structures in git, and introduce a dir_init() to handle the\nsuggested memsetting of dir_struct to all zeroes.  I hope that a name\nlike \"dir_free()\" is more clear, and that the presence of dir_init()\nwill provide a hint to those looking at the code that there may be a\ncorresponding dir_free() that they need to call.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/add.c          |  4 ++--\n builtin/check-ignore.c |  4 ++--\n builtin/clean.c        |  8 ++++----\n builtin/grep.c         |  3 ++-\n builtin/ls-files.c     |  4 ++--\n builtin/stash.c        |  4 ++--\n dir.c                  |  7 ++++++-\n dir.h                  | 19 ++++++++++---------\n merge.c                |  3 ++-\n wt-status.c            |  4 ++--\n 10 files changed, 34 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ab39a60a0d..46c9a0f552 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -534,11 +534,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tdie_in_unpopulated_submodule(&the_index, prefix);\n \tdie_path_inside_submodule(&the_index, &pathspec);\n \n+\tdir_init(&dir);\n \tif (add_new_files) {\n \t\tint baselen;\n \n \t\t/* Set up the default git porcelain excludes */\n-\t\tmemset(&dir, 0, sizeof(dir));\n \t\tif (!ignored_too) {\n \t\t\tdir.flags |= DIR_COLLECT_IGNORED;\n \t\t\tsetup_standard_excludes(&dir);\n@@ -611,7 +611,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"Unable to write new index file\"));\n \n+\tdir_free(&dir);\n \tUNLEAK(pathspec);\n-\tUNLEAK(dir);\n \treturn exit_status;\n }\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex ea5d0ae3a6..29b464a1fa 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -180,7 +180,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)\n \tif (!no_index && read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tsetup_standard_excludes(&dir);\n \n \tif (stdin_paths) {\n@@ -190,7 +190,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)\n \t\tmaybe_flush_or_die(stdout, \"ignore to stdout\");\n \t}\n \n-\tclear_directory(&dir);\n+\tdir_free(&dir);\n \n \treturn !num_ignored;\n }\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 4ffe00dd7f..a4cf58af74 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -667,7 +667,7 @@ static int filter_by_patterns_cmd(void)\n \t\tif (!confirm.len)\n \t\t\tbreak;\n \n-\t\tmemset(&dir, 0, sizeof(dir));\n+\t\tdir_init(&dir);\n \t\tpl = add_pattern_list(&dir, EXC_CMDL, \"manual exclude\");\n \t\tignore_list = strbuf_split_max(&confirm, ' ', 0);\n \n@@ -698,7 +698,7 @@ static int filter_by_patterns_cmd(void)\n \t\t}\n \n \t\tstrbuf_list_free(ignore_list);\n-\t\tclear_directory(&dir);\n+\t\tdir_free(&dir);\n \t}\n \n \tstrbuf_release(&confirm);\n@@ -923,7 +923,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t     0);\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tif (!interactive && !dry_run && !force) {\n \t\tif (config_set)\n \t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n@@ -1021,7 +1021,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tstring_list_append(&del_list, rel);\n \t}\n \n-\tclear_directory(&dir);\n+\tdir_free(&dir);\n \n \tif (interactive && del_list.nr > 0)\n \t\tinteractive_main_loop();\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex cee9db3477..fef2cc05ca 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -693,7 +693,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \tstruct dir_struct dir;\n \tint i, hit = 0;\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tif (!use_index)\n \t\tdir.flags |= DIR_NO_GITLINKS;\n \tif (exc_std)\n@@ -705,6 +705,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\tif (hit && opt->status_only)\n \t\t\tbreak;\n \t}\n+\tdir_free(&dir);\n \treturn hit;\n }\n \ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 30a4c10334..740feff53e 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -584,7 +584,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tprefix = cmd_prefix;\n \tif (prefix)\n \t\tprefix_len = strlen(prefix);\n@@ -688,6 +688,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\treturn bad ? 1 : 0;\n \t}\n \n-\tUNLEAK(dir);\n+\tdir_free(&dir);\n \treturn 0;\n }\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex da48533d49..85b646b7ad 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -864,7 +864,7 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,\n \tint found = 0;\n \tstruct dir_struct dir;\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tif (include_untracked != INCLUDE_ALL_FILES)\n \t\tsetup_standard_excludes(&dir);\n \n@@ -877,7 +877,7 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,\n \t\tstrbuf_addch(untracked_files, '\\0');\n \t}\n \n-\tclear_directory(&dir);\n+\tdir_free(&dir);\n \treturn found;\n }\n \ndiff --git a/dir.c b/dir.c\nindex b136c037d9..e3c8e7ffd6 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -54,6 +54,11 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n static int resolve_dtype(int dtype, struct index_state *istate,\n \t\t\t const char *path, int len);\n \n+void dir_init(struct dir_struct *dir)\n+{\n+\tmemset(dir, 0, sizeof(*dir));\n+}\n+\n int count_slashes(const char *s)\n {\n \tint cnt = 0;\n@@ -3013,7 +3018,7 @@ int remove_path(const char *name)\n }\n \n /* Frees memory within dir which was allocated.  Does not free dir itself. */\n-void clear_directory(struct dir_struct *dir)\n+void dir_free(struct dir_struct *dir)\n {\n \tint i, j;\n \tstruct exclude_list_group *group;\ndiff --git a/dir.h b/dir.h\nindex 7d76d0644f..7c55c1a460 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -19,22 +19,21 @@\n  * CE_SKIP_WORKTREE marked. If you want to exclude files, make sure you have\n  * loaded the index first.\n  *\n- * - Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,\n- * sizeof(dir))`.\n+ * - Prepare `struct dir_struct dir` using `dir_init()` function.\n  *\n  * - To add single exclude pattern, call `add_pattern_list()` and then\n  *   `add_pattern()`.\n  *\n  * - To add patterns from a file (e.g. `.git/info/exclude`), call\n- *   `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`.  A\n- *   short-hand function `setup_standard_excludes()` can be used to set\n- *   up the standard set of exclude settings.\n+ *   `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`.\n  *\n- * - Set options described in the Data Structure section above.\n+ * - A short-hand function `setup_standard_excludes()` can be used to set\n+ *   up the standard set of exclude settings, instead of manually calling\n+ *   the add_pattern*() family of functions.\n  *\n- * - Call `read_directory()`.\n+ * - Call `fill_directory()`.\n  *\n- * - Use `dir.entries[]`.\n+ * - Use `dir.entries[]` and `dir.ignored[]`.\n  *\n  * - Call `clear_directory()` when the contained elements are no longer in use.\n  *\n@@ -362,6 +361,8 @@ int match_pathspec(const struct index_state *istate,\n int report_path_error(const char *ps_matched, const struct pathspec *pathspec);\n int within_depth(const char *name, int namelen, int depth, int max_depth);\n \n+void dir_init(struct dir_struct *dir);\n+\n int fill_directory(struct dir_struct *dir,\n \t\t   struct index_state *istate,\n \t\t   const struct pathspec *pathspec);\n@@ -428,7 +429,7 @@ void parse_path_pattern(const char **string, int *patternlen, unsigned *flags, i\n void add_pattern(const char *string, const char *base,\n \t\t int baselen, struct pattern_list *pl, int srcpos);\n void clear_pattern_list(struct pattern_list *pl);\n-void clear_directory(struct dir_struct *dir);\n+void dir_free(struct dir_struct *dir);\n \n int repo_file_exists(struct repository *repo, const char *path);\n int file_exists(const char *);\ndiff --git a/merge.c b/merge.c\nindex 753e461659..ee149b87ea 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -80,8 +80,8 @@ int checkout_fast_forward(struct repository *r,\n \t}\n \n \tmemset(&opts, 0, sizeof(opts));\n+\tdir_init(&dir);\n \tif (overwrite_ignore) {\n-\t\tmemset(&dir, 0, sizeof(dir));\n \t\tdir.flags |= DIR_SHOW_IGNORED;\n \t\tsetup_standard_excludes(&dir);\n \t\topts.dir = &dir;\n@@ -102,6 +102,7 @@ int checkout_fast_forward(struct repository *r,\n \t\tclear_unpack_trees_porcelain(&opts);\n \t\treturn -1;\n \t}\n+\tdir_free(&dir);\n \tclear_unpack_trees_porcelain(&opts);\n \n \tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\ndiff --git a/wt-status.c b/wt-status.c\nindex c00ea3e06a..5624a8ff7f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -703,7 +703,7 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \tif (!s->show_untracked_files)\n \t\treturn;\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tif (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)\n \t\tdir.flags |=\n \t\t\tDIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n@@ -732,7 +732,7 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \t\t\tstring_list_insert(&s->ignored, ent->name);\n \t}\n \n-\tclear_directory(&dir);\n+\tdir_free(&dir);\n \n \tif (advice_status_u_option)\n \t\ts->untracked_in_ms = (getnanotime() - t_begin) / 1000000;\n-- \ngitgitgadget\n"},{"id":"403789","messageId":"068e097e22fa42b79e70b0346cc7460f1a3cbcff.1597561152.git.gitgitgadget@gmail.com","threadId":"54053","inReplyTo":"pull.831.git.git.1597561152.gitgitgadget@gmail.com","subject":"[PATCH 2/3] dir: make clear_directory() free all relevant memory","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-16T06:59:10Z","receivedAt":"2020-08-16T06:59:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nThe calling convention for the dir API is supposed to end with a call to\nclear_directory() to free up no longer needed memory.  However,\nclear_directory() didn't free dir->entries or dir->ignored.  I believe\nthis was oversight, but a number of callers noticed memory leaks and\nstarted free'ing these, but often somewhat haphazardly (sometimes\nfreeing the entries in the arrays, and sometimes only free'ing the\narrays themselves).  This suggests the callers weren't trying to make\nsure any possible memory used might be free'd, but just the memory they\nnoticed their usecase definitely had allocated.  This also caused the\nextra memory deallocations to be duplicated in many places.\n\nFix this mess by moving all the duplicated free'ing logic into\nclear_directory().\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/clean.c |  6 +-----\n builtin/stash.c |  3 ---\n dir.c           | 12 ++++++++----\n dir.h           |  2 +-\n wt-status.c     |  4 ----\n 5 files changed, 10 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 5a9c29a558..4ffe00dd7f 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -1021,11 +1021,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tstring_list_append(&del_list, rel);\n \t}\n \n-\tfor (i = 0; i < dir.nr; i++)\n-\t\tfree(dir.entries[i]);\n-\n-\tfor (i = 0; i < dir.ignored_nr; i++)\n-\t\tfree(dir.ignored[i]);\n+\tclear_directory(&dir);\n \n \tif (interactive && del_list.nr > 0)\n \t\tinteractive_main_loop();\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 10d87630cd..da48533d49 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -875,11 +875,8 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,\n \t\tstrbuf_addstr(untracked_files, ent->name);\n \t\t/* NUL-terminate: will be fed to update-index -z */\n \t\tstrbuf_addch(untracked_files, '\\0');\n-\t\tfree(ent);\n \t}\n \n-\tfree(dir.entries);\n-\tfree(dir.ignored);\n \tclear_directory(&dir);\n \treturn found;\n }\ndiff --git a/dir.c b/dir.c\nindex 08df469bf7..b136c037d9 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3012,10 +3012,7 @@ int remove_path(const char *name)\n \treturn 0;\n }\n \n-/*\n- * Frees memory within dir which was allocated for exclude lists and\n- * the exclude_stack.  Does not free dir itself.\n- */\n+/* Frees memory within dir which was allocated.  Does not free dir itself. */\n void clear_directory(struct dir_struct *dir)\n {\n \tint i, j;\n@@ -3034,6 +3031,13 @@ void clear_directory(struct dir_struct *dir)\n \t\tfree(group->pl);\n \t}\n \n+\tfor (i = 0; i < dir->ignored_nr; i++)\n+\t\tfree(dir->ignored[i]);\n+\tfor (i = 0; i < dir->nr; i++)\n+\t\tfree(dir->entries[i]);\n+\tfree(dir->ignored);\n+\tfree(dir->entries);\n+\n \tstk = dir->exclude_stack;\n \twhile (stk) {\n \t\tstruct exclude_stack *prev = stk->prev;\ndiff --git a/dir.h b/dir.h\nindex 5855c065a6..7d76d0644f 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -36,7 +36,7 @@\n  *\n  * - Use `dir.entries[]`.\n  *\n- * - Call `clear_directory()` when none of the contained elements are no longer in use.\n+ * - Call `clear_directory()` when the contained elements are no longer in use.\n  *\n  */\n \ndiff --git a/wt-status.c b/wt-status.c\nindex d75399085d..c00ea3e06a 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -724,18 +724,14 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \t\tstruct dir_entry *ent = dir.entries[i];\n \t\tif (index_name_is_other(istate, ent->name, ent->len))\n \t\t\tstring_list_insert(&s->untracked, ent->name);\n-\t\tfree(ent);\n \t}\n \n \tfor (i = 0; i < dir.ignored_nr; i++) {\n \t\tstruct dir_entry *ent = dir.ignored[i];\n \t\tif (index_name_is_other(istate, ent->name, ent->len))\n \t\t\tstring_list_insert(&s->ignored, ent->name);\n-\t\tfree(ent);\n \t}\n \n-\tfree(dir.entries);\n-\tfree(dir.ignored);\n \tclear_directory(&dir);\n \n \tif (advice_status_u_option)\n-- \ngitgitgadget\n\n"},{"id":"403790","messageId":"20200816084328.GA1221900@coredump.intra.peff.net","threadId":"54053","inReplyTo":"932741d7598ca2934dbca40f715ba2d3819fcc51.1597561152.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] dir: fix leak of parent_hashmap and recursive_hashmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-16T08:43:28Z","receivedAt":"2020-08-16T08:43:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 16, 2020 at 06:59:09AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren <newren@gmail.com>\n> \n> Commit 96cc8ab531 (\"sparse-checkout: use hashmaps for cone patterns\",\n> 2019-11-21) added a parent_hashmap and recursive_hashmap to each struct\n> pattern_list and initialized these in add_patterns_from_buffer() but did\n> not make sure to add necessary code to clear_pattern_list() to free\n> these new structures.  Call hashmap_free_() on each to plug this memory\n> leak.\n\nBeat you to it. :)\n\nSee: https://lore.kernel.org/git/20200814111049.GA4101811@coredump.intra.peff.net/\n\n-Peff\n"},{"id":"403791","messageId":"20200816085406.GB1221900@coredump.intra.peff.net","threadId":"54053","inReplyTo":"068e097e22fa42b79e70b0346cc7460f1a3cbcff.1597561152.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] dir: make clear_directory() free all relevant memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-16T08:54:06Z","receivedAt":"2020-08-16T08:54:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 16, 2020 at 06:59:10AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren <newren@gmail.com>\n> \n> The calling convention for the dir API is supposed to end with a call to\n> clear_directory() to free up no longer needed memory.  However,\n> clear_directory() didn't free dir->entries or dir->ignored.  I believe\n> this was oversight, but a number of callers noticed memory leaks and\n> started free'ing these, but often somewhat haphazardly (sometimes\n> freeing the entries in the arrays, and sometimes only free'ing the\n> arrays themselves).  This suggests the callers weren't trying to make\n> sure any possible memory used might be free'd, but just the memory they\n> noticed their usecase definitely had allocated.  This also caused the\n> extra memory deallocations to be duplicated in many places.\n> \n> Fix this mess by moving all the duplicated free'ing logic into\n> clear_directory().\n\nMakes sense. I don't know the dir.c code very well, so my worry would be\nthat some caller really wanted the other fields left untouched. But\nlooking over the callers, it seems they're all clearing it before the\nstruct goes out of scope. It's possible that they could have created\nother pointer references, but it seems unlikely (and I'd argue they\nshould stop doing that and make their own copies of the data). E.g.,\nwt_status_collect_untracked() sticks names into a string_list, but it\nsets strdup_strings to make its own copy, so it's good.\n\n> @@ -3034,6 +3031,13 @@ void clear_directory(struct dir_struct *dir)\n>  \t\tfree(group->pl);\n>  \t}\n>  \n> +\tfor (i = 0; i < dir->ignored_nr; i++)\n> +\t\tfree(dir->ignored[i]);\n> +\tfor (i = 0; i < dir->nr; i++)\n> +\t\tfree(dir->entries[i]);\n> +\tfree(dir->ignored);\n> +\tfree(dir->entries);\n> +\n>  \tstk = dir->exclude_stack;\n>  \twhile (stk) {\n>  \t\tstruct exclude_stack *prev = stk->prev;\n\nIn most of our \"clear\" functions, the struct is ready for use again\n(i.e., fields are set back to the initialized state). I don't think any\ncaller cares at this point, but it may be worth doing while we are here\nas a least-surprise thing. That would mean setting these pointers to\nNULL, and probably a few others that you aren't touching here.\n\nPerhaps even easier would be to add a dir_init() call at the end after\nyour next patch adds that function.\n\n-Peff\n"},{"id":"403792","messageId":"20200816091154.GC1221900@coredump.intra.peff.net","threadId":"54053","inReplyTo":"b9310e9941e91336258edd97a913e5908180720e.1597561152.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] dir: fix problematic API to avoid memory leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-16T09:11:54Z","receivedAt":"2020-08-16T09:11:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 16, 2020 at 06:59:11AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> Digging further, I found that despite the pretty clear documentation\n> near the top of dir.h that folks were supposed to call clear_directory()\n> when the user no longer needed the dir_struct, there were four callers\n> that didn't bother doing that at all.  However, two of them clearly\n> thought about leaks since they had an UNLEAK(dir) directive, which to me\n> suggests that the method to free the data was too unclear.  I suspect\n> the non-obviousness of the API and its holes led folks to avoid it,\n> which then snowballed into further problems with the entries[],\n> ignored[], parent_hashmap, and recursive_hashmap problems.\n\nThe UNLEAK() ones are sort-of my fault, and are a combination of:\n\n  - The commit adding them says \"in some cases (e.g., dir_struct) we\n    don't even have a function which knows how to free all of the struct\n    members\". I'm not sure if why I didn't fix clear_directory() as you\n    did. I may not have known about it, or I may have been lazy. Or more\n    likely I was simply holding the UNLEAK() hammer, so it looked like a\n    nail. ;)\n\n  - My focus was on making leak-checker output cleaner. And it wasn't\n    clear for cases where we're about to exit the program whether we\n    should be actually freeing (which has small but non-zero cost) or\n    just annotating (which is zero-cost, but clearly has confused some\n    people about how UNLEAK() was meant to be used). I think I'm leaning\n    these days towards \"free if it is easy to do so\".\n\nSo this definitely seems like a good direction to me.\n\n> Rename clear_directory() to dir_free() to be more in line with other\n> data structures in git, and introduce a dir_init() to handle the\n> suggested memsetting of dir_struct to all zeroes.  I hope that a name\n> like \"dir_free()\" is more clear, and that the presence of dir_init()\n> will provide a hint to those looking at the code that there may be a\n> corresponding dir_free() that they need to call.\n\nI think having a pair of matched calls is good. I _thought_ we had\nestablished a pattern that we should prefer \"clear\" to \"free\" for cases\nwhere the struct itself its not freed. But grepping around, I see there\nare a few exceptions (hashmap_free() is the big one, and then\noidmap_free() which is built around it seems to have inherited it).\n\nThe rest seem to follow that pattern, though: attr_check_free,\ncache_tree_free, and submodule_cache_free all actually free the pointer\npassed in. And \"git grep '_clear(' *.h\" shows lots of\nclear-but-don't-free examples.\n\nWould dir_clear() make the pairing more obvious, but keep the clear\nname?\n\n(I also wouldn't be opposed to changing hashmap and oidmap to use the\nname \"clear\", but that's obviously a separate patch).\n\n>  builtin/add.c          |  4 ++--\n>  builtin/check-ignore.c |  4 ++--\n>  builtin/clean.c        |  8 ++++----\n>  builtin/grep.c         |  3 ++-\n>  builtin/ls-files.c     |  4 ++--\n>  builtin/stash.c        |  4 ++--\n>  dir.c                  |  7 ++++++-\n>  dir.h                  | 19 ++++++++++---------\n>  merge.c                |  3 ++-\n>  wt-status.c            |  4 ++--\n>  10 files changed, 34 insertions(+), 26 deletions(-)\n\nThat patch itself looks good except for two minor points:\n\n>  /* Frees memory within dir which was allocated.  Does not free dir itself. */\n> -void clear_directory(struct dir_struct *dir)\n> +void dir_free(struct dir_struct *dir)\n>  {\n>  \tint i, j;\n>  \tstruct exclude_list_group *group;\n\nAs I mentioned in my previous email, I think it would be nice if this\ncalled dir_init() at the end, so that the struct is always in a\nconsistent state.\n\n> diff --git a/dir.h b/dir.h\n> index 7d76d0644f..7c55c1a460 100644\n> --- a/dir.h\n> +++ b/dir.h\n> [...]\n> - * - Use `dir.entries[]`.\n> + * - Use `dir.entries[]` and `dir.ignored[]`.\n>   *\n>   * - Call `clear_directory()` when the contained elements are no longer in use.\n>   *\n\nThis last line should become dir_free() / dir_clear().\n\n-Peff\n"},{"id":"403837","messageId":"CABPp-BHyCVdb5AueF+tTwTsgAA5LkPEj-mPLoX_F+WYgPqFcNw@mail.gmail.com","threadId":"54053","inReplyTo":"20200816091154.GC1221900@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] dir: fix problematic API to avoid memory leaks","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-08-17T17:19:03Z","receivedAt":"2020-08-17T17:27:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Aug 16, 2020 at 2:11 AM Jeff King <peff@peff.net> wrote:\n>\n> On Sun, Aug 16, 2020 at 06:59:11AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > Digging further, I found that despite the pretty clear documentation\n> > near the top of dir.h that folks were supposed to call clear_directory()\n> > when the user no longer needed the dir_struct, there were four callers\n> > that didn't bother doing that at all.  However, two of them clearly\n> > thought about leaks since they had an UNLEAK(dir) directive, which to me\n> > suggests that the method to free the data was too unclear.  I suspect\n> > the non-obviousness of the API and its holes led folks to avoid it,\n> > which then snowballed into further problems with the entries[],\n> > ignored[], parent_hashmap, and recursive_hashmap problems.\n>\n> The UNLEAK() ones are sort-of my fault, and are a combination of:\n>\n>   - The commit adding them says \"in some cases (e.g., dir_struct) we\n>     don't even have a function which knows how to free all of the struct\n>     members\". I'm not sure if why I didn't fix clear_directory() as you\n>     did. I may not have known about it, or I may have been lazy. Or more\n>     likely I was simply holding the UNLEAK() hammer, so it looked like a\n>     nail. ;)\n>\n>   - My focus was on making leak-checker output cleaner. And it wasn't\n>     clear for cases where we're about to exit the program whether we\n>     should be actually freeing (which has small but non-zero cost) or\n>     just annotating (which is zero-cost, but clearly has confused some\n>     people about how UNLEAK() was meant to be used). I think I'm leaning\n>     these days towards \"free if it is easy to do so\".\n>\n> So this definitely seems like a good direction to me.\n>\n> > Rename clear_directory() to dir_free() to be more in line with other\n> > data structures in git, and introduce a dir_init() to handle the\n> > suggested memsetting of dir_struct to all zeroes.  I hope that a name\n> > like \"dir_free()\" is more clear, and that the presence of dir_init()\n> > will provide a hint to those looking at the code that there may be a\n> > corresponding dir_free() that they need to call.\n>\n> I think having a pair of matched calls is good. I _thought_ we had\n> established a pattern that we should prefer \"clear\" to \"free\" for cases\n> where the struct itself its not freed. But grepping around, I see there\n> are a few exceptions (hashmap_free() is the big one, and then\n> oidmap_free() which is built around it seems to have inherited it).\n>\n> The rest seem to follow that pattern, though: attr_check_free,\n> cache_tree_free, and submodule_cache_free all actually free the pointer\n> passed in. And \"git grep '_clear(' *.h\" shows lots of\n> clear-but-don't-free examples.\n>\n> Would dir_clear() make the pairing more obvious, but keep the clear\n> name?\n\nSure, that works for me.  The case where having an actual _free()\nmakes sense to me -- possibly in addition to a _clear() -- is when\nsome memory has to be allocated before first use, and thus a\nfoo_clear() would leave that memory allocated.  Then you really do\nneed a foo_free() for use when the data structure won't be used again.\n\n> (I also wouldn't be opposed to changing hashmap and oidmap to use the\n> name \"clear\", but that's obviously a separate patch).\n\nhashmap is one of the cases that needs to have a free construct,\nbecause the table in which to stuff the entries has to be allocated\nand thus a hashmap_clear() would have to leave the table allocated if\nit wants to be ready for re-use.  If someone really is done with a\nhashmap, then to avoid leaking, both the entries and the table need to\nbe deallocated.\n\nI keep getting confused by the hashmap API, and what pieces it frees\n-- it looks like my earlier comments today were wrong and\nhashmap_free_entries() does free the table.  So...perhaps I should\ncreate a patch to make that clearer, and also submit the patch I've\nhad for a while to introduce a hashmap_clear() function (which is\nsimilar to hashmap_free_entries, in that it frees the entries and\nzeros out most of the map, but it leaves the table allocated and ready\nfor use).\n\nI really wish hashmap_free() did what hashmap_free_entries() did.  So\nannoying and counter-intuitive...\n\n> >  builtin/add.c          |  4 ++--\n> >  builtin/check-ignore.c |  4 ++--\n> >  builtin/clean.c        |  8 ++++----\n> >  builtin/grep.c         |  3 ++-\n> >  builtin/ls-files.c     |  4 ++--\n> >  builtin/stash.c        |  4 ++--\n> >  dir.c                  |  7 ++++++-\n> >  dir.h                  | 19 ++++++++++---------\n> >  merge.c                |  3 ++-\n> >  wt-status.c            |  4 ++--\n> >  10 files changed, 34 insertions(+), 26 deletions(-)\n>\n> That patch itself looks good except for two minor points:\n>\n> >  /* Frees memory within dir which was allocated.  Does not free dir itself. */\n> > -void clear_directory(struct dir_struct *dir)\n> > +void dir_free(struct dir_struct *dir)\n> >  {\n> >       int i, j;\n> >       struct exclude_list_group *group;\n>\n> As I mentioned in my previous email, I think it would be nice if this\n> called dir_init() at the end, so that the struct is always in a\n> consistent state.\n>\n> > diff --git a/dir.h b/dir.h\n> > index 7d76d0644f..7c55c1a460 100644\n> > --- a/dir.h\n> > +++ b/dir.h\n> > [...]\n> > - * - Use `dir.entries[]`.\n> > + * - Use `dir.entries[]` and `dir.ignored[]`.\n> >   *\n> >   * - Call `clear_directory()` when the contained elements are no longer in use.\n> >   *\n>\n> This last line should become dir_free() / dir_clear().\n\nI'll fix these up.\n\nElijah\n"},{"id":"403839","messageId":"CABPp-BHUN=f-oGmjUpbRRs=h9OZ75jFZUZbQ7_sFO4cCKuVK_Q@mail.gmail.com","threadId":"54053","inReplyTo":"20200816085406.GB1221900@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] dir: make clear_directory() free all relevant memory","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-08-17T16:58:53Z","receivedAt":"2020-08-17T17:30:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Aug 16, 2020 at 1:54 AM Jeff King <peff@peff.net> wrote:\n>\n> On Sun, Aug 16, 2020 at 06:59:10AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren <newren@gmail.com>\n> >\n> > The calling convention for the dir API is supposed to end with a call to\n> > clear_directory() to free up no longer needed memory.  However,\n> > clear_directory() didn't free dir->entries or dir->ignored.  I believe\n> > this was oversight, but a number of callers noticed memory leaks and\n> > started free'ing these, but often somewhat haphazardly (sometimes\n> > freeing the entries in the arrays, and sometimes only free'ing the\n> > arrays themselves).  This suggests the callers weren't trying to make\n> > sure any possible memory used might be free'd, but just the memory they\n> > noticed their usecase definitely had allocated.  This also caused the\n> > extra memory deallocations to be duplicated in many places.\n> >\n> > Fix this mess by moving all the duplicated free'ing logic into\n> > clear_directory().\n>\n> Makes sense. I don't know the dir.c code very well, so my worry would be\n> that some caller really wanted the other fields left untouched. But\n> looking over the callers, it seems they're all clearing it before the\n> struct goes out of scope. It's possible that they could have created\n> other pointer references, but it seems unlikely (and I'd argue they\n> should stop doing that and make their own copies of the data). E.g.,\n> wt_status_collect_untracked() sticks names into a string_list, but it\n> sets strdup_strings to make its own copy, so it's good.\n>\n> > @@ -3034,6 +3031,13 @@ void clear_directory(struct dir_struct *dir)\n> >               free(group->pl);\n> >       }\n> >\n> > +     for (i = 0; i < dir->ignored_nr; i++)\n> > +             free(dir->ignored[i]);\n> > +     for (i = 0; i < dir->nr; i++)\n> > +             free(dir->entries[i]);\n> > +     free(dir->ignored);\n> > +     free(dir->entries);\n> > +\n> >       stk = dir->exclude_stack;\n> >       while (stk) {\n> >               struct exclude_stack *prev = stk->prev;\n>\n> In most of our \"clear\" functions, the struct is ready for use again\n> (i.e., fields are set back to the initialized state). I don't think any\n> caller cares at this point, but it may be worth doing while we are here\n> as a least-surprise thing. That would mean setting these pointers to\n> NULL, and probably a few others that you aren't touching here.\n>\n> Perhaps even easier would be to add a dir_init() call at the end after\n> your next patch adds that function.\n\nMake sense; I'll fix it up.\n"},{"id":"403841","messageId":"CABPp-BFBcMaPTvxE_PYqpHzo5zUG=Lv7Y+Oq2yCRYxRTgRQgLA@mail.gmail.com","threadId":"54053","inReplyTo":"20200816084328.GA1221900@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] dir: fix leak of parent_hashmap and recursive_hashmap","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-08-17T16:57:39Z","receivedAt":"2020-08-17T17:31:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Aug 16, 2020 at 1:43 AM Jeff King <peff@peff.net> wrote:\n>\n> On Sun, Aug 16, 2020 at 06:59:09AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren <newren@gmail.com>\n> >\n> > Commit 96cc8ab531 (\"sparse-checkout: use hashmaps for cone patterns\",\n> > 2019-11-21) added a parent_hashmap and recursive_hashmap to each struct\n> > pattern_list and initialized these in add_patterns_from_buffer() but did\n> > not make sure to add necessary code to clear_pattern_list() to free\n> > these new structures.  Call hashmap_free_() on each to plug this memory\n> > leak.\n>\n> Beat you to it. :)\n>\n> See: https://lore.kernel.org/git/20200814111049.GA4101811@coredump.intra.peff.net/\n\nDoh!  However, you do need to take care to free the hash table in\naddition to the hash entries, or you'll still have a leak (I actually\nmade the same mistake)...  I'll add those comments on your patch,\nthough.\n"},{"id":"403868","messageId":"20200817190031.GB1278968@coredump.intra.peff.net","threadId":"54053","inReplyTo":"CABPp-BHyCVdb5AueF+tTwTsgAA5LkPEj-mPLoX_F+WYgPqFcNw@mail.gmail.com","subject":"Re: [PATCH 3/3] dir: fix problematic API to avoid memory leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-17T19:00:31Z","receivedAt":"2020-08-17T19:00:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 17, 2020 at 10:19:03AM -0700, Elijah Newren wrote:\n\n> > (I also wouldn't be opposed to changing hashmap and oidmap to use the\n> > name \"clear\", but that's obviously a separate patch).\n> \n> hashmap is one of the cases that needs to have a free construct,\n> because the table in which to stuff the entries has to be allocated\n> and thus a hashmap_clear() would have to leave the table allocated if\n> it wants to be ready for re-use.  If someone really is done with a\n> hashmap, then to avoid leaking, both the entries and the table need to\n> be deallocated.\n\nHmm, you're right. oidmap() will lazy-initialize the table, but hashmap\nwill not. You _do_ have to initialize a hashmap because somebody has to\nset the comparison function. But that could be fixed, and would make it\nmore like the rest of our code. I.e., you should be able to do:\n\n  struct hashmap foo = HASHMAP_INIT(my_cmpfn);\n\n  if (bar)\n\treturn; /* no leak, because we never put anything in the map! */\n\n  ... add some stuff to the map ...\n\n  hashmap_clear(&foo);\n\n  ... now it's empty and nothing allocated; we could return\n      here without a leak or we could add more stuff to it ...\n\nI don't even think it would be that big a change. Just translate a NULL\ntable to \"not found\" on the read side, and lazily call alloc_table() on\nthe write side. And have hashmap_free() not very the _whole_ struct, but\nleave the cmpfn in place.\n\n> I keep getting confused by the hashmap API, and what pieces it frees\n> -- it looks like my earlier comments today were wrong and\n> hashmap_free_entries() does free the table.  So...perhaps I should\n> create a patch to make that clearer, and also submit the patch I've\n> had for a while to introduce a hashmap_clear() function (which is\n> similar to hashmap_free_entries, in that it frees the entries and\n> zeros out most of the map, but it leaves the table allocated and ready\n> for use).\n> \n> I really wish hashmap_free() did what hashmap_free_entries() did.  So\n> annoying and counter-intuitive...\n\nI left some comments in my other reply, but in case you do pursue this:\nthe obvious thing to have is a free_entries boolean parameter to the\nfunction, so that each caller is clear about what they want. And we used\nto have that. But it's awkward because \"free the entries\" isn't a\nboolean anymore; it's \"free the entries you can find by moving backwards\nto this offset from the hashmap_entry pointer\". So callers who don't\nwant to free them have to pass some sentinel value there. And that's how\nwe ended up with two separate wrapper functions.\n\nI think your main complaint is just about the naming though. If we had:\n\n  /* drop all entries, freeing any hashmap-specific memory */\n  hashmap_clear();\n\n  /* ditto, but also free the entries themselves */\n  hashmap_clear_and_free_entires();\n\nthat would be a bit more obvious (though I imagine it would still be\neasy to forget that \"clear\" doesn't drop the entries). Another approach\nwould be to have a flag in the map for \"do I own the entry memory\". Most\ncallers are happy to hand off ownership of the entries when they're\nadded. And it may even be that this would open up the possibility of\nmore convenience functions on the adding/allocation side. I didn't think\nit through carefully, though.\n\n-Peff\n"},{"id":"403981","messageId":"pull.831.v2.git.git.1597791506.gitgitgadget@gmail.com","threadId":"54053","inReplyTo":"pull.831.git.git.1597561152.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Clean up some memory leaks in and around dir.c","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-18T22:58:24Z","receivedAt":"2020-08-18T22:58:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Some memory leaks in dir.c were making it hard for me to find my own leaks\nin merge-ort. I followed a few threads and tried to both clean up the leaks\nand make the API less prone to incorrect use.\n\nChanges since v1:\n\n * dropped the first patch, since Peff submitted an identical patch a day\n   before me\n * implemented Peff's suggestions to rename dir_free() to dir_clear() and\n   have it call dir_init() at the end so that folks can re-use the struct\n   after dir_clear() if they so choose.\n\nElijah Newren (2):\n  dir: make clear_directory() free all relevant memory\n  dir: fix problematic API to avoid memory leaks\n\n builtin/add.c          |  4 ++--\n builtin/check-ignore.c |  4 ++--\n builtin/clean.c        | 12 ++++--------\n builtin/grep.c         |  3 ++-\n builtin/ls-files.c     |  4 ++--\n builtin/stash.c        |  7 ++-----\n dir.c                  | 20 +++++++++++++++++---\n dir.h                  | 21 +++++++++++----------\n merge.c                |  3 ++-\n wt-status.c            |  8 ++------\n 10 files changed, 46 insertions(+), 40 deletions(-)\n\n\nbase-commit: 878e727637ec5815ccb3301eb994a54df95b21b8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-831%2Fnewren%2Fdir_memory_cleanup-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-831/newren/dir_memory_cleanup-v2\nPull-Request: https://github.com/git/git/pull/831\n\nRange-diff vs v1:\n\n 1:  932741d759 < -:  ---------- dir: fix leak of parent_hashmap and recursive_hashmap\n 2:  068e097e22 ! 1:  f890ac0279 dir: make clear_directory() free all relevant memory\n     @@ Commit message\n          The calling convention for the dir API is supposed to end with a call to\n          clear_directory() to free up no longer needed memory.  However,\n          clear_directory() didn't free dir->entries or dir->ignored.  I believe\n     -    this was oversight, but a number of callers noticed memory leaks and\n     -    started free'ing these, but often somewhat haphazardly (sometimes\n     -    freeing the entries in the arrays, and sometimes only free'ing the\n     -    arrays themselves).  This suggests the callers weren't trying to make\n     -    sure any possible memory used might be free'd, but just the memory they\n     -    noticed their usecase definitely had allocated.  This also caused the\n     -    extra memory deallocations to be duplicated in many places.\n     +    this was an oversight, but a number of callers noticed memory leaks and\n     +    started free'ing these.  Unfortunately, they did so somewhat haphazardly\n     +    (sometimes freeing the entries in the arrays, and sometimes only\n     +    free'ing the arrays themselves).  This suggests the callers weren't\n     +    trying to make sure any possible memory used might be free'd, but just\n     +    the memory they noticed their usecase definitely had allocated.\n      \n          Fix this mess by moving all the duplicated free'ing logic into\n     -    clear_directory().\n     +    clear_directory().  End by resetting dir to a pristine state so it could\n     +    be reused if desired.\n      \n          Signed-off-by: Elijah Newren <newren@gmail.com>\n      \n     @@ builtin/stash.c: static int get_untracked_files(const struct pathspec *ps, int i\n      \n       ## dir.c ##\n      @@ dir.c: int remove_path(const char *name)\n     - \treturn 0;\n       }\n       \n     --/*\n     + /*\n      - * Frees memory within dir which was allocated for exclude lists and\n      - * the exclude_stack.  Does not free dir itself.\n     -- */\n     -+/* Frees memory within dir which was allocated.  Does not free dir itself. */\n     ++ * Frees memory within dir which was allocated, and resets fields for further\n     ++ * use.  Does not free dir itself.\n     +  */\n       void clear_directory(struct dir_struct *dir)\n       {\n     - \tint i, j;\n      @@ dir.c: void clear_directory(struct dir_struct *dir)\n       \t\tfree(group->pl);\n       \t}\n     @@ dir.c: void clear_directory(struct dir_struct *dir)\n       \tstk = dir->exclude_stack;\n       \twhile (stk) {\n       \t\tstruct exclude_stack *prev = stk->prev;\n     +@@ dir.c: void clear_directory(struct dir_struct *dir)\n     + \t\tstk = prev;\n     + \t}\n     + \tstrbuf_release(&dir->basebuf);\n     ++\n     ++\tmemset(&dir, 0, sizeof(*dir));\n     + }\n     + \n     + struct ondisk_untracked_cache {\n      \n       ## dir.h ##\n      @@\n 3:  b9310e9941 ! 2:  dcd72eef48 dir: fix problematic API to avoid memory leaks\n     @@ Metadata\n       ## Commit message ##\n          dir: fix problematic API to avoid memory leaks\n      \n     -    Two commits ago, we noticed that parent_hashmap and recursive_hashmap\n     -    were being leaked and fixed it by making clear_pattern_list() free those\n     -    hashmaps.  One commit ago, we noticed that clear_directory() was only\n     -    taking responsibility for a subset of fields within dir_struct, despite\n     -    the fact that entries[] and ignored[] we allocated internally to dir.c,\n     -    resulting in many callers either leaking or haphazardly trying to free\n     -    these arrays and their contents.\n     +    The dir structure seemed to have a number of leaks and problems around\n     +    it.  First I noticed that parent_hashmap and recursive_hashmap were\n     +    being leaked (though Peff noticed and submitted fixes before me).  Then\n     +    I noticed in the previous commit that clear_directory() was only taking\n     +    responsibility for a subset of fields within dir_struct, despite the\n     +    fact that entries[] and ignored[] we allocated internally to dir.c.\n     +    That, of course, resulted in many callers either leaking or haphazardly\n     +    trying to free these arrays and their contents.\n      \n          Digging further, I found that despite the pretty clear documentation\n          near the top of dir.h that folks were supposed to call clear_directory()\n     @@ Commit message\n          which then snowballed into further problems with the entries[],\n          ignored[], parent_hashmap, and recursive_hashmap problems.\n      \n     -    Rename clear_directory() to dir_free() to be more in line with other\n     +    Rename clear_directory() to dir_clear() to be more in line with other\n          data structures in git, and introduce a dir_init() to handle the\n          suggested memsetting of dir_struct to all zeroes.  I hope that a name\n     -    like \"dir_free()\" is more clear, and that the presence of dir_init()\n     -    will provide a hint to those looking at the code that there may be a\n     -    corresponding dir_free() that they need to call.\n     +    like \"dir_clear()\" is more clear, and that the presence of dir_init()\n     +    will provide a hint to those looking at the code that they need to look\n     +    for either a dir_clear() or a dir_free() and lead them to find\n     +    dir_clear().\n      \n          Signed-off-by: Elijah Newren <newren@gmail.com>\n      \n     @@ builtin/add.c: int cmd_add(int argc, const char **argv, const char *prefix)\n       \t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n       \t\tdie(_(\"Unable to write new index file\"));\n       \n     -+\tdir_free(&dir);\n     ++\tdir_clear(&dir);\n       \tUNLEAK(pathspec);\n      -\tUNLEAK(dir);\n       \treturn exit_status;\n     @@ builtin/check-ignore.c: int cmd_check_ignore(int argc, const char **argv, const\n       \t}\n       \n      -\tclear_directory(&dir);\n     -+\tdir_free(&dir);\n     ++\tdir_clear(&dir);\n       \n       \treturn !num_ignored;\n       }\n     @@ builtin/clean.c: static int filter_by_patterns_cmd(void)\n       \n       \t\tstrbuf_list_free(ignore_list);\n      -\t\tclear_directory(&dir);\n     -+\t\tdir_free(&dir);\n     ++\t\tdir_clear(&dir);\n       \t}\n       \n       \tstrbuf_release(&confirm);\n     @@ builtin/clean.c: int cmd_clean(int argc, const char **argv, const char *prefix)\n       \t}\n       \n      -\tclear_directory(&dir);\n     -+\tdir_free(&dir);\n     ++\tdir_clear(&dir);\n       \n       \tif (interactive && del_list.nr > 0)\n       \t\tinteractive_main_loop();\n     @@ builtin/grep.c: static int grep_directory(struct grep_opt *opt, const struct pat\n       \t\tif (hit && opt->status_only)\n       \t\t\tbreak;\n       \t}\n     -+\tdir_free(&dir);\n     ++\tdir_clear(&dir);\n       \treturn hit;\n       }\n       \n     @@ builtin/ls-files.c: int cmd_ls_files(int argc, const char **argv, const char *cm\n       \t}\n       \n      -\tUNLEAK(dir);\n     -+\tdir_free(&dir);\n     ++\tdir_clear(&dir);\n       \treturn 0;\n       }\n      \n     @@ builtin/stash.c: static int get_untracked_files(const struct pathspec *ps, int i\n       \t}\n       \n      -\tclear_directory(&dir);\n     -+\tdir_free(&dir);\n     ++\tdir_clear(&dir);\n       \treturn found;\n       }\n       \n     @@ dir.c: static enum path_treatment read_directory_recursive(struct dir_struct *di\n       {\n       \tint cnt = 0;\n      @@ dir.c: int remove_path(const char *name)\n     - }\n     - \n     - /* Frees memory within dir which was allocated.  Does not free dir itself. */\n     +  * Frees memory within dir which was allocated, and resets fields for further\n     +  * use.  Does not free dir itself.\n     +  */\n      -void clear_directory(struct dir_struct *dir)\n     -+void dir_free(struct dir_struct *dir)\n     ++void dir_clear(struct dir_struct *dir)\n       {\n       \tint i, j;\n       \tstruct exclude_list_group *group;\n     +@@ dir.c: void clear_directory(struct dir_struct *dir)\n     + \t}\n     + \tstrbuf_release(&dir->basebuf);\n     + \n     +-\tmemset(&dir, 0, sizeof(*dir));\n     ++\tdir_init(dir);\n     + }\n     + \n     + struct ondisk_untracked_cache {\n      \n       ## dir.h ##\n      @@\n     @@ dir.h\n      - * - Use `dir.entries[]`.\n      + * - Use `dir.entries[]` and `dir.ignored[]`.\n        *\n     -  * - Call `clear_directory()` when the contained elements are no longer in use.\n     +- * - Call `clear_directory()` when the contained elements are no longer in use.\n     ++ * - Call `dir_clear()` when the contained elements are no longer in use.\n        *\n     +  */\n     + \n      @@ dir.h: int match_pathspec(const struct index_state *istate,\n       int report_path_error(const char *ps_matched, const struct pathspec *pathspec);\n       int within_depth(const char *name, int namelen, int depth, int max_depth);\n     @@ dir.h: void parse_path_pattern(const char **string, int *patternlen, unsigned *f\n       \t\t int baselen, struct pattern_list *pl, int srcpos);\n       void clear_pattern_list(struct pattern_list *pl);\n      -void clear_directory(struct dir_struct *dir);\n     -+void dir_free(struct dir_struct *dir);\n     ++void dir_clear(struct dir_struct *dir);\n       \n       int repo_file_exists(struct repository *repo, const char *path);\n       int file_exists(const char *);\n     @@ merge.c: int checkout_fast_forward(struct repository *r,\n       \t\tclear_unpack_trees_porcelain(&opts);\n       \t\treturn -1;\n       \t}\n     -+\tdir_free(&dir);\n     ++\tdir_clear(&dir);\n       \tclear_unpack_trees_porcelain(&opts);\n       \n       \tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\n     @@ wt-status.c: static void wt_status_collect_untracked(struct wt_status *s)\n       \t}\n       \n      -\tclear_directory(&dir);\n     -+\tdir_free(&dir);\n     ++\tdir_clear(&dir);\n       \n       \tif (advice_status_u_option)\n       \t\ts->untracked_in_ms = (getnanotime() - t_begin) / 1000000;\n\n-- \ngitgitgadget\n"},{"id":"403982","messageId":"f890ac027936350df8e2818552485268afd83aa6.1597791506.git.gitgitgadget@gmail.com","threadId":"54053","inReplyTo":"pull.831.v2.git.git.1597791506.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] dir: make clear_directory() free all relevant memory","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-18T22:58:25Z","receivedAt":"2020-08-18T22:58:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nThe calling convention for the dir API is supposed to end with a call to\nclear_directory() to free up no longer needed memory.  However,\nclear_directory() didn't free dir->entries or dir->ignored.  I believe\nthis was an oversight, but a number of callers noticed memory leaks and\nstarted free'ing these.  Unfortunately, they did so somewhat haphazardly\n(sometimes freeing the entries in the arrays, and sometimes only\nfree'ing the arrays themselves).  This suggests the callers weren't\ntrying to make sure any possible memory used might be free'd, but just\nthe memory they noticed their usecase definitely had allocated.\n\nFix this mess by moving all the duplicated free'ing logic into\nclear_directory().  End by resetting dir to a pristine state so it could\nbe reused if desired.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/clean.c |  6 +-----\n builtin/stash.c |  3 ---\n dir.c           | 13 +++++++++++--\n dir.h           |  2 +-\n wt-status.c     |  4 ----\n 5 files changed, 13 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 5a9c29a558..4ffe00dd7f 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -1021,11 +1021,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tstring_list_append(&del_list, rel);\n \t}\n \n-\tfor (i = 0; i < dir.nr; i++)\n-\t\tfree(dir.entries[i]);\n-\n-\tfor (i = 0; i < dir.ignored_nr; i++)\n-\t\tfree(dir.ignored[i]);\n+\tclear_directory(&dir);\n \n \tif (interactive && del_list.nr > 0)\n \t\tinteractive_main_loop();\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 10d87630cd..da48533d49 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -875,11 +875,8 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,\n \t\tstrbuf_addstr(untracked_files, ent->name);\n \t\t/* NUL-terminate: will be fed to update-index -z */\n \t\tstrbuf_addch(untracked_files, '\\0');\n-\t\tfree(ent);\n \t}\n \n-\tfree(dir.entries);\n-\tfree(dir.ignored);\n \tclear_directory(&dir);\n \treturn found;\n }\ndiff --git a/dir.c b/dir.c\nindex fe64be30ed..aa96030031 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3009,8 +3009,8 @@ int remove_path(const char *name)\n }\n \n /*\n- * Frees memory within dir which was allocated for exclude lists and\n- * the exclude_stack.  Does not free dir itself.\n+ * Frees memory within dir which was allocated, and resets fields for further\n+ * use.  Does not free dir itself.\n  */\n void clear_directory(struct dir_struct *dir)\n {\n@@ -3030,6 +3030,13 @@ void clear_directory(struct dir_struct *dir)\n \t\tfree(group->pl);\n \t}\n \n+\tfor (i = 0; i < dir->ignored_nr; i++)\n+\t\tfree(dir->ignored[i]);\n+\tfor (i = 0; i < dir->nr; i++)\n+\t\tfree(dir->entries[i]);\n+\tfree(dir->ignored);\n+\tfree(dir->entries);\n+\n \tstk = dir->exclude_stack;\n \twhile (stk) {\n \t\tstruct exclude_stack *prev = stk->prev;\n@@ -3037,6 +3044,8 @@ void clear_directory(struct dir_struct *dir)\n \t\tstk = prev;\n \t}\n \tstrbuf_release(&dir->basebuf);\n+\n+\tmemset(&dir, 0, sizeof(*dir));\n }\n \n struct ondisk_untracked_cache {\ndiff --git a/dir.h b/dir.h\nindex 5855c065a6..7d76d0644f 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -36,7 +36,7 @@\n  *\n  * - Use `dir.entries[]`.\n  *\n- * - Call `clear_directory()` when none of the contained elements are no longer in use.\n+ * - Call `clear_directory()` when the contained elements are no longer in use.\n  *\n  */\n \ndiff --git a/wt-status.c b/wt-status.c\nindex d75399085d..c00ea3e06a 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -724,18 +724,14 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \t\tstruct dir_entry *ent = dir.entries[i];\n \t\tif (index_name_is_other(istate, ent->name, ent->len))\n \t\t\tstring_list_insert(&s->untracked, ent->name);\n-\t\tfree(ent);\n \t}\n \n \tfor (i = 0; i < dir.ignored_nr; i++) {\n \t\tstruct dir_entry *ent = dir.ignored[i];\n \t\tif (index_name_is_other(istate, ent->name, ent->len))\n \t\t\tstring_list_insert(&s->ignored, ent->name);\n-\t\tfree(ent);\n \t}\n \n-\tfree(dir.entries);\n-\tfree(dir.ignored);\n \tclear_directory(&dir);\n \n \tif (advice_status_u_option)\n-- \ngitgitgadget\n\n"},{"id":"403983","messageId":"dcd72eef4844535839e03d7d6fa0bf18a2b3add9.1597791506.git.gitgitgadget@gmail.com","threadId":"54053","inReplyTo":"pull.831.v2.git.git.1597791506.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] dir: fix problematic API to avoid memory leaks","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-18T22:58:26Z","receivedAt":"2020-08-18T22:58:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nThe dir structure seemed to have a number of leaks and problems around\nit.  First I noticed that parent_hashmap and recursive_hashmap were\nbeing leaked (though Peff noticed and submitted fixes before me).  Then\nI noticed in the previous commit that clear_directory() was only taking\nresponsibility for a subset of fields within dir_struct, despite the\nfact that entries[] and ignored[] we allocated internally to dir.c.\nThat, of course, resulted in many callers either leaking or haphazardly\ntrying to free these arrays and their contents.\n\nDigging further, I found that despite the pretty clear documentation\nnear the top of dir.h that folks were supposed to call clear_directory()\nwhen the user no longer needed the dir_struct, there were four callers\nthat didn't bother doing that at all.  However, two of them clearly\nthought about leaks since they had an UNLEAK(dir) directive, which to me\nsuggests that the method to free the data was too unclear.  I suspect\nthe non-obviousness of the API and its holes led folks to avoid it,\nwhich then snowballed into further problems with the entries[],\nignored[], parent_hashmap, and recursive_hashmap problems.\n\nRename clear_directory() to dir_clear() to be more in line with other\ndata structures in git, and introduce a dir_init() to handle the\nsuggested memsetting of dir_struct to all zeroes.  I hope that a name\nlike \"dir_clear()\" is more clear, and that the presence of dir_init()\nwill provide a hint to those looking at the code that they need to look\nfor either a dir_clear() or a dir_free() and lead them to find\ndir_clear().\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/add.c          |  4 ++--\n builtin/check-ignore.c |  4 ++--\n builtin/clean.c        |  8 ++++----\n builtin/grep.c         |  3 ++-\n builtin/ls-files.c     |  4 ++--\n builtin/stash.c        |  4 ++--\n dir.c                  |  9 +++++++--\n dir.h                  | 21 +++++++++++----------\n merge.c                |  3 ++-\n wt-status.c            |  4 ++--\n 10 files changed, 36 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ab39a60a0d..b36a99eb7c 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -534,11 +534,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tdie_in_unpopulated_submodule(&the_index, prefix);\n \tdie_path_inside_submodule(&the_index, &pathspec);\n \n+\tdir_init(&dir);\n \tif (add_new_files) {\n \t\tint baselen;\n \n \t\t/* Set up the default git porcelain excludes */\n-\t\tmemset(&dir, 0, sizeof(dir));\n \t\tif (!ignored_too) {\n \t\t\tdir.flags |= DIR_COLLECT_IGNORED;\n \t\t\tsetup_standard_excludes(&dir);\n@@ -611,7 +611,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\tdie(_(\"Unable to write new index file\"));\n \n+\tdir_clear(&dir);\n \tUNLEAK(pathspec);\n-\tUNLEAK(dir);\n \treturn exit_status;\n }\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex ea5d0ae3a6..3c652748d5 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -180,7 +180,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)\n \tif (!no_index && read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tsetup_standard_excludes(&dir);\n \n \tif (stdin_paths) {\n@@ -190,7 +190,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)\n \t\tmaybe_flush_or_die(stdout, \"ignore to stdout\");\n \t}\n \n-\tclear_directory(&dir);\n+\tdir_clear(&dir);\n \n \treturn !num_ignored;\n }\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 4ffe00dd7f..e53ea52d89 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -667,7 +667,7 @@ static int filter_by_patterns_cmd(void)\n \t\tif (!confirm.len)\n \t\t\tbreak;\n \n-\t\tmemset(&dir, 0, sizeof(dir));\n+\t\tdir_init(&dir);\n \t\tpl = add_pattern_list(&dir, EXC_CMDL, \"manual exclude\");\n \t\tignore_list = strbuf_split_max(&confirm, ' ', 0);\n \n@@ -698,7 +698,7 @@ static int filter_by_patterns_cmd(void)\n \t\t}\n \n \t\tstrbuf_list_free(ignore_list);\n-\t\tclear_directory(&dir);\n+\t\tdir_clear(&dir);\n \t}\n \n \tstrbuf_release(&confirm);\n@@ -923,7 +923,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t     0);\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tif (!interactive && !dry_run && !force) {\n \t\tif (config_set)\n \t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\n@@ -1021,7 +1021,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tstring_list_append(&del_list, rel);\n \t}\n \n-\tclear_directory(&dir);\n+\tdir_clear(&dir);\n \n \tif (interactive && del_list.nr > 0)\n \t\tinteractive_main_loop();\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex cee9db3477..f58979bc3f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -693,7 +693,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \tstruct dir_struct dir;\n \tint i, hit = 0;\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tif (!use_index)\n \t\tdir.flags |= DIR_NO_GITLINKS;\n \tif (exc_std)\n@@ -705,6 +705,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\tif (hit && opt->status_only)\n \t\t\tbreak;\n \t}\n+\tdir_clear(&dir);\n \treturn hit;\n }\n \ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 30a4c10334..c8eae899b8 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -584,7 +584,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tprefix = cmd_prefix;\n \tif (prefix)\n \t\tprefix_len = strlen(prefix);\n@@ -688,6 +688,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\treturn bad ? 1 : 0;\n \t}\n \n-\tUNLEAK(dir);\n+\tdir_clear(&dir);\n \treturn 0;\n }\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex da48533d49..4bdfaf8397 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -864,7 +864,7 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,\n \tint found = 0;\n \tstruct dir_struct dir;\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tif (include_untracked != INCLUDE_ALL_FILES)\n \t\tsetup_standard_excludes(&dir);\n \n@@ -877,7 +877,7 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,\n \t\tstrbuf_addch(untracked_files, '\\0');\n \t}\n \n-\tclear_directory(&dir);\n+\tdir_clear(&dir);\n \treturn found;\n }\n \ndiff --git a/dir.c b/dir.c\nindex aa96030031..8d6c59a9dc 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -54,6 +54,11 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n static int resolve_dtype(int dtype, struct index_state *istate,\n \t\t\t const char *path, int len);\n \n+void dir_init(struct dir_struct *dir)\n+{\n+\tmemset(dir, 0, sizeof(*dir));\n+}\n+\n int count_slashes(const char *s)\n {\n \tint cnt = 0;\n@@ -3012,7 +3017,7 @@ int remove_path(const char *name)\n  * Frees memory within dir which was allocated, and resets fields for further\n  * use.  Does not free dir itself.\n  */\n-void clear_directory(struct dir_struct *dir)\n+void dir_clear(struct dir_struct *dir)\n {\n \tint i, j;\n \tstruct exclude_list_group *group;\n@@ -3045,7 +3050,7 @@ void clear_directory(struct dir_struct *dir)\n \t}\n \tstrbuf_release(&dir->basebuf);\n \n-\tmemset(&dir, 0, sizeof(*dir));\n+\tdir_init(dir);\n }\n \n struct ondisk_untracked_cache {\ndiff --git a/dir.h b/dir.h\nindex 7d76d0644f..a3c40dec51 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -19,24 +19,23 @@\n  * CE_SKIP_WORKTREE marked. If you want to exclude files, make sure you have\n  * loaded the index first.\n  *\n- * - Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,\n- * sizeof(dir))`.\n+ * - Prepare `struct dir_struct dir` using `dir_init()` function.\n  *\n  * - To add single exclude pattern, call `add_pattern_list()` and then\n  *   `add_pattern()`.\n  *\n  * - To add patterns from a file (e.g. `.git/info/exclude`), call\n- *   `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`.  A\n- *   short-hand function `setup_standard_excludes()` can be used to set\n- *   up the standard set of exclude settings.\n+ *   `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`.\n  *\n- * - Set options described in the Data Structure section above.\n+ * - A short-hand function `setup_standard_excludes()` can be used to set\n+ *   up the standard set of exclude settings, instead of manually calling\n+ *   the add_pattern*() family of functions.\n  *\n- * - Call `read_directory()`.\n+ * - Call `fill_directory()`.\n  *\n- * - Use `dir.entries[]`.\n+ * - Use `dir.entries[]` and `dir.ignored[]`.\n  *\n- * - Call `clear_directory()` when the contained elements are no longer in use.\n+ * - Call `dir_clear()` when the contained elements are no longer in use.\n  *\n  */\n \n@@ -362,6 +361,8 @@ int match_pathspec(const struct index_state *istate,\n int report_path_error(const char *ps_matched, const struct pathspec *pathspec);\n int within_depth(const char *name, int namelen, int depth, int max_depth);\n \n+void dir_init(struct dir_struct *dir);\n+\n int fill_directory(struct dir_struct *dir,\n \t\t   struct index_state *istate,\n \t\t   const struct pathspec *pathspec);\n@@ -428,7 +429,7 @@ void parse_path_pattern(const char **string, int *patternlen, unsigned *flags, i\n void add_pattern(const char *string, const char *base,\n \t\t int baselen, struct pattern_list *pl, int srcpos);\n void clear_pattern_list(struct pattern_list *pl);\n-void clear_directory(struct dir_struct *dir);\n+void dir_clear(struct dir_struct *dir);\n \n int repo_file_exists(struct repository *repo, const char *path);\n int file_exists(const char *);\ndiff --git a/merge.c b/merge.c\nindex 753e461659..5fb88af102 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -80,8 +80,8 @@ int checkout_fast_forward(struct repository *r,\n \t}\n \n \tmemset(&opts, 0, sizeof(opts));\n+\tdir_init(&dir);\n \tif (overwrite_ignore) {\n-\t\tmemset(&dir, 0, sizeof(dir));\n \t\tdir.flags |= DIR_SHOW_IGNORED;\n \t\tsetup_standard_excludes(&dir);\n \t\topts.dir = &dir;\n@@ -102,6 +102,7 @@ int checkout_fast_forward(struct repository *r,\n \t\tclear_unpack_trees_porcelain(&opts);\n \t\treturn -1;\n \t}\n+\tdir_clear(&dir);\n \tclear_unpack_trees_porcelain(&opts);\n \n \tif (write_locked_index(r->index, &lock_file, COMMIT_LOCK))\ndiff --git a/wt-status.c b/wt-status.c\nindex c00ea3e06a..7ce58b8aae 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -703,7 +703,7 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \tif (!s->show_untracked_files)\n \t\treturn;\n \n-\tmemset(&dir, 0, sizeof(dir));\n+\tdir_init(&dir);\n \tif (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)\n \t\tdir.flags |=\n \t\t\tDIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n@@ -732,7 +732,7 @@ static void wt_status_collect_untracked(struct wt_status *s)\n \t\t\tstring_list_insert(&s->ignored, ent->name);\n \t}\n \n-\tclear_directory(&dir);\n+\tdir_clear(&dir);\n \n \tif (advice_status_u_option)\n \t\ts->untracked_in_ms = (getnanotime() - t_begin) / 1000000;\n-- \ngitgitgadget\n"},{"id":"404009","messageId":"20200819135109.GA2498279@coredump.intra.peff.net","threadId":"54053","inReplyTo":"pull.831.v2.git.git.1597791506.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Clean up some memory leaks in and around dir.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-19T13:51:09Z","receivedAt":"2020-08-19T13:51:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 18, 2020 at 10:58:24PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> Some memory leaks in dir.c were making it hard for me to find my own leaks\n> in merge-ort. I followed a few threads and tried to both clean up the leaks\n> and make the API less prone to incorrect use.\n> \n> Changes since v1:\n> \n>  * dropped the first patch, since Peff submitted an identical patch a day\n>    before me\n>  * implemented Peff's suggestions to rename dir_free() to dir_clear() and\n>    have it call dir_init() at the end so that folks can re-use the struct\n>    after dir_clear() if they so choose.\n\nThanks, both patches look good to me.\n\n-Peff\n"}]}