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>
Elijah Newren committed
Aug 18, 2020 at 22:58 UTC
eceba5321410bbcc9e0b11e6aa479832f574eca8
10 files changed
+36
-28
builtin/add.c
+2
-2
@@ -534,11 +534,11 @@ int cmd_add(int argc, const char **argv, const char *prefix)
534
die_in_unpopulated_submodule(&the_index, prefix);
535
die_path_inside_submodule(&the_index, &pathspec);
536
537
+ dir_init(&dir);
538
if (add_new_files) {
539
int baselen;
540
541
/* Set up the default git porcelain excludes */
541
- memset(&dir, 0, sizeof(dir));
542
if (!ignored_too) {
543
dir.flags |= DIR_COLLECT_IGNORED;
544
setup_standard_excludes(&dir);
@@ -611,7 +611,7 @@ finish:
611
COMMIT_LOCK | SKIP_IF_UNCHANGED))
612
die(_("Unable to write new index file"));
613
614
+ dir_clear(&dir);
615
UNLEAK(pathspec);
615
- UNLEAK(dir);
616
return exit_status;
617
}
builtin/check-ignore.c
+2
-2
@@ -180,7 +180,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
180
if (!no_index && read_cache() < 0)
181
die(_("index file corrupt"));
182
183
- memset(&dir, 0, sizeof(dir));
183
+ dir_init(&dir);
184
setup_standard_excludes(&dir);
185
186
if (stdin_paths) {
@@ -190,7 +190,7 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
190
maybe_flush_or_die(stdout, "ignore to stdout");
191
}
192
193
- clear_directory(&dir);
193
+ dir_clear(&dir);
194
195
return !num_ignored;
196
}
builtin/clean.c
+4
-4
@@ -667,7 +667,7 @@ static int filter_by_patterns_cmd(void)
667
if (!confirm.len)
668
break;
669
670
- memset(&dir, 0, sizeof(dir));
670
+ dir_init(&dir);
671
pl = add_pattern_list(&dir, EXC_CMDL, "manual exclude");
672
ignore_list = strbuf_split_max(&confirm, ' ', 0);
673
@@ -698,7 +698,7 @@ static int filter_by_patterns_cmd(void)
698
}
699
700
strbuf_list_free(ignore_list);
701
- clear_directory(&dir);
701
+ dir_clear(&dir);
702
}
703
704
strbuf_release(&confirm);
@@ -923,7 +923,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
923
argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
924
0);
925
926
- memset(&dir, 0, sizeof(dir));
926
+ dir_init(&dir);
927
if (!interactive && !dry_run && !force) {
928
if (config_set)
929
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)
1021
string_list_append(&del_list, rel);
1022
}
1023
1024
- clear_directory(&dir);
1024
+ dir_clear(&dir);
1025
1026
if (interactive && del_list.nr > 0)
1027
interactive_main_loop();
builtin/grep.c
+2
-1
@@ -693,7 +693,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
693
struct dir_struct dir;
694
int i, hit = 0;
695
696
- memset(&dir, 0, sizeof(dir));
696
+ dir_init(&dir);
697
if (!use_index)
698
dir.flags |= DIR_NO_GITLINKS;
699
if (exc_std)
@@ -705,6 +705,7 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
705
if (hit && opt->status_only)
706
break;
707
}
708
+ dir_clear(&dir);
709
return hit;
710
}
711
builtin/ls-files.c
+2
-2
@@ -584,7 +584,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
584
if (argc == 2 && !strcmp(argv[1], "-h"))
585
usage_with_options(ls_files_usage, builtin_ls_files_options);
586
587
- memset(&dir, 0, sizeof(dir));
587
+ dir_init(&dir);
588
prefix = cmd_prefix;
589
if (prefix)
590
prefix_len = strlen(prefix);
@@ -688,6 +688,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
688
return bad ? 1 : 0;
689
}
690
691
- UNLEAK(dir);
691
+ dir_clear(&dir);
692
return 0;
693
}
builtin/stash.c
+2
-2
@@ -864,7 +864,7 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,
864
int found = 0;
865
struct dir_struct dir;
866
867
- memset(&dir, 0, sizeof(dir));
867
+ dir_init(&dir);
868
if (include_untracked != INCLUDE_ALL_FILES)
869
setup_standard_excludes(&dir);
870
@@ -877,7 +877,7 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,
877
strbuf_addch(untracked_files, '\0');
878
}
879
880
- clear_directory(&dir);
880
+ dir_clear(&dir);
881
return found;
882
}
883
dir.c
+7
-2
@@ -54,6 +54,11 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,
54
static int resolve_dtype(int dtype, struct index_state *istate,
55
const char *path, int len);
56
57
+void dir_init(struct dir_struct *dir)
58
+{
59
+ memset(dir, 0, sizeof(*dir));
60
+}
61
+
62
int count_slashes(const char *s)
63
{
64
int cnt = 0;
@@ -3012,7 +3017,7 @@ int remove_path(const char *name)
3017
* Frees memory within dir which was allocated, and resets fields for further
3018
* use. Does not free dir itself.
3019
*/
3015
-void clear_directory(struct dir_struct *dir)
3020
+void dir_clear(struct dir_struct *dir)
3021
{
3022
int i, j;
3023
struct exclude_list_group *group;
@@ -3045,7 +3050,7 @@ void clear_directory(struct dir_struct *dir)
3050
}
3051
strbuf_release(&dir->basebuf);
3052
3048
- memset(&dir, 0, sizeof(*dir));
3053
+ dir_init(dir);
3054
}
3055
3056
struct ondisk_untracked_cache {
dir.h
+11
-10
@@ -19,24 +19,23 @@
19
* CE_SKIP_WORKTREE marked. If you want to exclude files, make sure you have
20
* loaded the index first.
21
*
22
- * - Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,
23
- * sizeof(dir))`.
22
+ * - Prepare `struct dir_struct dir` using `dir_init()` function.
23
*
24
* - To add single exclude pattern, call `add_pattern_list()` and then
25
* `add_pattern()`.
26
*
27
* - To add patterns from a file (e.g. `.git/info/exclude`), call
29
- * `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`. A
30
- * short-hand function `setup_standard_excludes()` can be used to set
31
- * up the standard set of exclude settings.
28
+ * `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`.
29
*
33
- * - Set options described in the Data Structure section above.
30
+ * - A short-hand function `setup_standard_excludes()` can be used to set
31
+ * up the standard set of exclude settings, instead of manually calling
32
+ * the add_pattern*() family of functions.
33
*
35
- * - Call `read_directory()`.
34
+ * - Call `fill_directory()`.
35
*
37
- * - Use `dir.entries[]`.
36
+ * - Use `dir.entries[]` and `dir.ignored[]`.
37
*
39
- * - Call `clear_directory()` when the contained elements are no longer in use.
38
+ * - Call `dir_clear()` when the contained elements are no longer in use.
39
*
40
*/
41
@@ -362,6 +361,8 @@ int match_pathspec(const struct index_state *istate,
361
int report_path_error(const char *ps_matched, const struct pathspec *pathspec);
362
int within_depth(const char *name, int namelen, int depth, int max_depth);
363
364
+void dir_init(struct dir_struct *dir);
365
+
366
int fill_directory(struct dir_struct *dir,
367
struct index_state *istate,
368
const struct pathspec *pathspec);
@@ -428,7 +429,7 @@ void parse_path_pattern(const char **string, int *patternlen, unsigned *flags, i
429
void add_pattern(const char *string, const char *base,
430
int baselen, struct pattern_list *pl, int srcpos);
431
void clear_pattern_list(struct pattern_list *pl);
431
-void clear_directory(struct dir_struct *dir);
432
+void dir_clear(struct dir_struct *dir);
433
434
int repo_file_exists(struct repository *repo, const char *path);
435
int file_exists(const char *);
merge.c
+2
-1
@@ -80,8 +80,8 @@ int checkout_fast_forward(struct repository *r,
80
}
81
82
memset(&opts, 0, sizeof(opts));
83
+ dir_init(&dir);
84
if (overwrite_ignore) {
84
- memset(&dir, 0, sizeof(dir));
85
dir.flags |= DIR_SHOW_IGNORED;
86
setup_standard_excludes(&dir);
87
opts.dir = &dir;
@@ -102,6 +102,7 @@ int checkout_fast_forward(struct repository *r,
102
clear_unpack_trees_porcelain(&opts);
103
return -1;
104
}
105
+ dir_clear(&dir);
106
clear_unpack_trees_porcelain(&opts);
107
108
if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
wt-status.c
+2
-2
@@ -703,7 +703,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
703
if (!s->show_untracked_files)
704
return;
705
706
- memset(&dir, 0, sizeof(dir));
706
+ dir_init(&dir);
707
if (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)
708
dir.flags |=
709
DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
@@ -732,7 +732,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
732
string_list_insert(&s->ignored, ent->name);
733
}
734
735
- clear_directory(&dir);
735
+ dir_clear(&dir);
736
737
if (advice_status_u_option)
738
s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;