dir: fix problematic API to avoid memory leaks

The dir structure seemed to have a number of leaks and problems around
it.  First I noticed that parent_hashmap and recursive_hashmap were
being leaked (though Peff noticed and submitted fixes before me).  Then
I noticed in the previous commit that clear_directory() was only taking
responsibility for a subset of fields within dir_struct, despite the
fact that entries[] and ignored[] we allocated internally to dir.c.
That, of course, resulted in many callers either leaking or haphazardly
trying to free these arrays and their contents.

Digging further, I found that despite the pretty clear documentation
near the top of dir.h that folks were supposed to call clear_directory()
when the user no longer needed the dir_struct, there were four callers
that didn't bother doing that at all.  However, two of them clearly
thought about leaks since they had an UNLEAK(dir) directive, which to me
suggests that the method to free the data was too unclear.  I suspect
the non-obviousness of the API and its holes led folks to avoid it,
which then snowballed into further problems with the entries[],
ignored[], parent_hashmap, and recursive_hashmap problems.

Rename clear_directory() to dir_clear() to be more in line with other
data structures in git, and introduce a dir_init() to handle the
suggested memsetting of dir_struct to all zeroes.  I hope that a name
like "dir_clear()" is more clear, and that the presence of dir_init()
will provide a hint to those looking at the code that they need to look
for either a dir_clear() or a dir_free() and lead them to find
dir_clear().

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/add.c b/builtin/add.c
index ab39a60a0d69e0cd1aa4f7be4166c74af9c49887..b36a99eb7c847fe3f67a716316b148c347d1792b 100644 (file)
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -534,11 +534,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
        die_in_unpopulated_submodule(&the_index, prefix);
        die_path_inside_submodule(&the_index, &pathspec);
 
+       dir_init(&dir);
        if (add_new_files) {
                int baselen;
 
                /* Set up the default git porcelain excludes */
-               memset(&dir, 0, sizeof(dir));
                if (!ignored_too) {
                        dir.flags |= DIR_COLLECT_IGNORED;
                        setup_standard_excludes(&dir);
@@ -611,7 +611,7 @@ finish:
                               COMMIT_LOCK | SKIP_IF_UNCHANGED))
                die(_("Unable to write new index file"));
 
+       dir_clear(&dir);
        UNLEAK(pathspec);
-       UNLEAK(dir);
        return exit_status;
 }

diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index ea5d0ae3a6a6fff59424909b8be47aaabb824b7c..3c652748d58c69dec4378d01de58212f65763328 100644 (file)
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -180,7 +180,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
        if (!no_index && read_cache() < 0)
                die(_("index file corrupt"));
 
-       memset(&dir, 0, sizeof(dir));
+       dir_init(&dir);
        setup_standard_excludes(&dir);
 
        if (stdin_paths) {
@@ -190,7 +190,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
                maybe_flush_or_die(stdout, "ignore to stdout");
        }
 
-       clear_directory(&dir);
+       dir_clear(&dir);
 
        return !num_ignored;
 }

diff --git a/builtin/clean.c b/builtin/clean.c
index 4ffe00dd7f3f9f9c7455054be4aceb61d6e7d68f..e53ea52d89bbb473e26666e453d549e6720a9cc2 100644 (file)
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -667,7 +667,7 @@ static int filter_by_patterns_cmd(void)
                if (!confirm.len)
                        break;
 
-               memset(&dir, 0, sizeof(dir));
+               dir_init(&dir);
                pl = add_pattern_list(&dir, EXC_CMDL, "manual exclude");
                ignore_list = strbuf_split_max(&confirm, ' ', 0);
 
@@ -698,7 +698,7 @@ static int filter_by_patterns_cmd(void)
                }
 
                strbuf_list_free(ignore_list);
-               clear_directory(&dir);
+               dir_clear(&dir);
        }
 
        strbuf_release(&confirm);
@@ -923,7 +923,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
        argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
                             0);
 
-       memset(&dir, 0, sizeof(dir));
+       dir_init(&dir);
        if (!interactive && !dry_run && !force) {
                if (config_set)
                        die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
@@ -1021,7 +1021,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
                string_list_append(&del_list, rel);
        }
 
-       clear_directory(&dir);
+       dir_clear(&dir);
 
        if (interactive && del_list.nr > 0)
                interactive_main_loop();

diff --git a/builtin/grep.c b/builtin/grep.c
index cee9db3477631f55d54b4b32d5396db8830076bd..f58979bc3f07a6ca5adbd8704b1df41bf4215048 100644 (file)
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -693,7 +693,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
        struct dir_struct dir;
        int i, hit = 0;
 
-       memset(&dir, 0, sizeof(dir));
+       dir_init(&dir);
        if (!use_index)
                dir.flags |= DIR_NO_GITLINKS;
        if (exc_std)
@@ -705,6 +705,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
                if (hit && opt->status_only)
                        break;
        }
+       dir_clear(&dir);
        return hit;
 }
 

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 30a4c10334982e9ea34729a979a83027fa83c940..c8eae899b82a83ebc3487ffa99ba2bcf73d3369f 100644 (file)
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -584,7 +584,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
        if (argc == 2 && !strcmp(argv[1], "-h"))
                usage_with_options(ls_files_usage, builtin_ls_files_options);
 
-       memset(&dir, 0, sizeof(dir));
+       dir_init(&dir);
        prefix = cmd_prefix;
        if (prefix)
                prefix_len = strlen(prefix);
@@ -688,6 +688,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
                return bad ? 1 : 0;
        }
 
-       UNLEAK(dir);
+       dir_clear(&dir);
        return 0;
 }

diff --git a/builtin/stash.c b/builtin/stash.c
index da48533d490f686c82fd85d9381cff22d0dd6d47..4bdfaf839705a69c58fa631055eb4fc8fdff0ed4 100644 (file)
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -864,7 +864,7 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,
        int found = 0;
        struct dir_struct dir;
 
-       memset(&dir, 0, sizeof(dir));
+       dir_init(&dir);
        if (include_untracked != INCLUDE_ALL_FILES)
                setup_standard_excludes(&dir);
 
@@ -877,7 +877,7 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,
                strbuf_addch(untracked_files, '\0');
        }
 
-       clear_directory(&dir);
+       dir_clear(&dir);
        return found;
 }
 

diff --git a/dir.c b/dir.c
index aa96030031fa801e2311b48e9f64055e92e779ae..8d6c59a9dcf2e8221d07c7ac7a4bf68e88f1e5e6 100644 (file)
--- a/dir.c
+++ b/dir.c
@@ -54,6 +54,11 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,
 static int resolve_dtype(int dtype, struct index_state *istate,
                         const char *path, int len);
 
+void dir_init(struct dir_struct *dir)
+{
+       memset(dir, 0, sizeof(*dir));
+}
+
 int count_slashes(const char *s)
 {
        int cnt = 0;
@@ -3012,7 +3017,7 @@ int remove_path(const char *name)
  * Frees memory within dir which was allocated, and resets fields for further
  * use.  Does not free dir itself.
  */
-void clear_directory(struct dir_struct *dir)
+void dir_clear(struct dir_struct *dir)
 {
        int i, j;
        struct exclude_list_group *group;
@@ -3045,7 +3050,7 @@ void clear_directory(struct dir_struct *dir)
        }
        strbuf_release(&dir->basebuf);
 
-       memset(&dir, 0, sizeof(*dir));
+       dir_init(dir);
 }
 
 struct ondisk_untracked_cache {

diff --git a/dir.h b/dir.h
index 7d76d0644f2ff811337df004ec8dca7358c896d1..a3c40dec5165425ec00181a152b1ddac37dba43d 100644 (file)
--- a/dir.h
+++ b/dir.h
@@ -19,24 +19,23 @@
  * CE_SKIP_WORKTREE marked. If you want to exclude files, make sure you have
  * loaded the index first.
  *
- * - Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,
- * sizeof(dir))`.
+ * - Prepare `struct dir_struct dir` using `dir_init()` function.
  *
  * - To add single exclude pattern, call `add_pattern_list()` and then
  *   `add_pattern()`.
  *
  * - To add patterns from a file (e.g. `.git/info/exclude`), call
- *   `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`.  A
- *   short-hand function `setup_standard_excludes()` can be used to set
- *   up the standard set of exclude settings.
+ *   `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`.
  *
- * - Set options described in the Data Structure section above.
+ * - A short-hand function `setup_standard_excludes()` can be used to set
+ *   up the standard set of exclude settings, instead of manually calling
+ *   the add_pattern*() family of functions.
  *
- * - Call `read_directory()`.
+ * - Call `fill_directory()`.
  *
- * - Use `dir.entries[]`.
+ * - Use `dir.entries[]` and `dir.ignored[]`.
  *
- * - Call `clear_directory()` when the contained elements are no longer in use.
+ * - Call `dir_clear()` when the contained elements are no longer in use.
  *
  */
 
@@ -362,6 +361,8 @@ int match_pathspec(const struct index_state *istate,
 int report_path_error(const char *ps_matched, const struct pathspec *pathspec);
 int within_depth(const char *name, int namelen, int depth, int max_depth);
 
+void dir_init(struct dir_struct *dir);
+
 int fill_directory(struct dir_struct *dir,
                   struct index_state *istate,
                   const struct pathspec *pathspec);
@@ -428,7 +429,7 @@ void parse_path_pattern(const char **string, int *patternlen, unsigned *flags, i
 void add_pattern(const char *string, const char *base,
                 int baselen, struct pattern_list *pl, int srcpos);
 void clear_pattern_list(struct pattern_list *pl);
-void clear_directory(struct dir_struct *dir);
+void dir_clear(struct dir_struct *dir);
 
 int repo_file_exists(struct repository *repo, const char *path);
 int file_exists(const char *);

diff --git a/merge.c b/merge.c
index 753e461659e62812e60fe863bf76269cc8784f82..5fb88af10254a75557697345fd25cfcf19df9748 100644 (file)
--- a/merge.c
+++ b/merge.c
@@ -80,8 +80,8 @@ int checkout_fast_forward(struct repository *r,
        }
 
        memset(&opts, 0, sizeof(opts));
+       dir_init(&dir);
        if (overwrite_ignore) {
-               memset(&dir, 0, sizeof(dir));
                dir.flags |= DIR_SHOW_IGNORED;
                setup_standard_excludes(&dir);
                opts.dir = &dir;
@@ -102,6 +102,7 @@ int checkout_fast_forward(struct repository *r,
                clear_unpack_trees_porcelain(&opts);
                return -1;
        }
+       dir_clear(&dir);
        clear_unpack_trees_porcelain(&opts);
 
        if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))

diff --git a/wt-status.c b/wt-status.c
index c00ea3e06a395a49a07068c46dbbac6c50db9f9d..7ce58b8aae06a94a3615f0acdd07e0e10ffbd32f 100644 (file)
--- a/wt-status.c
+++ b/wt-status.c
@@ -703,7 +703,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
        if (!s->show_untracked_files)
                return;
 
-       memset(&dir, 0, sizeof(dir));
+       dir_init(&dir);
        if (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)
                dir.flags |=
                        DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
@@ -732,7 +732,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
                        string_list_insert(&s->ignored, ent->name);
        }
 
-       clear_directory(&dir);
+       dir_clear(&dir);
 
        if (advice_status_u_option)
                s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;