setup: fix memory leaks with `struct repository_format`

After we set up a `struct repository_format`, it owns various pieces of allocated memory. We then either use those members, because we decide we want to use the "candidate" repository format, or we discard the candidate / scratch space. In the first case, we transfer ownership of the memory to a few global variables. In the latter case, we just silently drop the struct and end up leaking memory. Introduce an initialization macro `REPOSITORY_FORMAT_INIT` and a function `clear_repository_format()`, to be used on each side of `read_repository_format()`. To have a clear and simple memory ownership, let all users of `struct repository_format` duplicate the strings that they take from it, rather than stealing the pointers. Call `clear_...()` at the start of `read_...()` instead of just zeroing the struct, since we sometimes enter the function multiple times. Thus, it is important to initialize the struct before calling `read_...()`, so document that. It's also important because we might not even call `read_...()` before we call `clear_...()`, see, e.g., builtin/init-db.c. Teach `read_...()` to clear the struct on error, so that it is reset to a safe state, and document this. (In `setup_git_directory_gently()`, we look at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we weren't actually supposed to do per the API. After this commit, that's ok.) We inherit the existing code's combining "error" and "no version found". Both are signalled through `version == -1` and now both cause us to clear any partial configuration we have picked up. For "extensions.*", that's fine, since they require a positive version number. For "core.bare" and "core.worktree", we're already verifying that we have a non-negative version number before using them. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Martin Ågren committed Feb 28, 2019 at 21:36 UTC e8805af1c33d79750a979014c021cd63d780c720
5 files changed +62 -18
builtin/init-db.c
+2 -1
@@ -96,7 +96,7 @@ static void copy_templates(const char *template_dir)
96 struct strbuf path = STRBUF_INIT;
97 struct strbuf template_path = STRBUF_INIT;
98 size_t template_len;
99 - struct repository_format template_format;
99 + struct repository_format template_format = REPOSITORY_FORMAT_INIT;
100 struct strbuf err = STRBUF_INIT;
101 DIR *dir;
102 char *to_free = NULL;
@@ -148,6 +148,7 @@ free_return:
148 free(to_free);
149 strbuf_release(&path);
150 strbuf_release(&template_path);
151 + clear_repository_format(&template_format);
152 }
153
154 static int git_init_db_config(const char *k, const char *v, void *cb)
cache.h
+28 -3
@@ -961,6 +961,10 @@ extern char *repository_format_partial_clone;
961 extern const char *core_partial_clone_filter_default;
962 extern int repository_format_worktree_config;
963
964 +/*
965 + * You _have_ to initialize a `struct repository_format` using
966 + * `= REPOSITORY_FORMAT_INIT` before calling `read_repository_format()`.
967 + */
968 struct repository_format {
969 int version;
970 int precious_objects;
@@ -972,14 +976,35 @@ struct repository_format {
976 struct string_list unknown_extensions;
977 };
978
979 +/*
980 + * Always use this to initialize a `struct repository_format`
981 + * to a well-defined, default state before calling
982 + * `read_repository()`.
983 + */
984 +#define REPOSITORY_FORMAT_INIT \
985 +{ \
986 + .version = -1, \
987 + .is_bare = -1, \
988 + .hash_algo = GIT_HASH_SHA1, \
989 + .unknown_extensions = STRING_LIST_INIT_DUP, \
990 +}
991 +
992 /*
993 * Read the repository format characteristics from the config file "path" into
977 - * "format" struct. Returns the numeric version. On error, -1 is returned,
978 - * format->version is set to -1, and all other fields in the struct are
979 - * undefined.
994 + * "format" struct. Returns the numeric version. On error, or if no version is
995 + * found in the configuration, -1 is returned, format->version is set to -1,
996 + * and all other fields in the struct are set to the default configuration
997 + * (REPOSITORY_FORMAT_INIT). Always initialize the struct using
998 + * REPOSITORY_FORMAT_INIT before calling this function.
999 */
1000 int read_repository_format(struct repository_format *format, const char *path);
1001
1002 +/*
1003 + * Free the memory held onto by `format`, but not the struct itself.
1004 + * (No need to use this after `read_repository_format()` fails.)
1005 + */
1006 +void clear_repository_format(struct repository_format *format);
1007 +
1008 /*
1009 * Verify that the repository described by repository_format is something we
1010 * can read. If it is, return 0. Otherwise, return -1, and "err" will describe
repository.c
+2 -1
@@ -148,7 +148,7 @@ int repo_init(struct repository *repo,
148 const char *gitdir,
149 const char *worktree)
150 {
151 - struct repository_format format;
151 + struct repository_format format = REPOSITORY_FORMAT_INIT;
152 memset(repo, 0, sizeof(*repo));
153
154 repo->objects = raw_object_store_new();
@@ -165,6 +165,7 @@ int repo_init(struct repository *repo,
165 if (worktree)
166 repo_set_worktree(repo, worktree);
167
168 + clear_repository_format(&format);
169 return 0;
170
171 error:
setup.c
+27 -12
@@ -477,7 +477,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
477 }
478
479 repository_format_precious_objects = candidate->precious_objects;
480 - repository_format_partial_clone = candidate->partial_clone;
480 + repository_format_partial_clone = xstrdup_or_null(candidate->partial_clone);
481 repository_format_worktree_config = candidate->worktree_config;
482 string_list_clear(&candidate->unknown_extensions, 0);
483
@@ -500,27 +500,38 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
500 }
501 if (candidate->work_tree) {
502 free(git_work_tree_cfg);
503 - git_work_tree_cfg = candidate->work_tree;
503 + git_work_tree_cfg = xstrdup(candidate->work_tree);
504 inside_work_tree = -1;
505 }
506 - } else {
507 - free(candidate->work_tree);
506 }
507
508 return 0;
509 }
510
511 +static void init_repository_format(struct repository_format *format)
512 +{
513 + const struct repository_format fresh = REPOSITORY_FORMAT_INIT;
514 +
515 + memcpy(format, &fresh, sizeof(fresh));
516 +}
517 +
518 int read_repository_format(struct repository_format *format, const char *path)
519 {
515 - memset(format, 0, sizeof(*format));
516 - format->version = -1;
517 - format->is_bare = -1;
518 - format->hash_algo = GIT_HASH_SHA1;
519 - string_list_init(&format->unknown_extensions, 1);
520 + clear_repository_format(format);
521 git_config_from_file(check_repo_format, path, format);
522 + if (format->version == -1)
523 + clear_repository_format(format);
524 return format->version;
525 }
526
527 +void clear_repository_format(struct repository_format *format)
528 +{
529 + string_list_clear(&format->unknown_extensions, 0);
530 + free(format->work_tree);
531 + free(format->partial_clone);
532 + init_repository_format(format);
533 +}
534 +
535 int verify_repository_format(const struct repository_format *format,
536 struct strbuf *err)
537 {
@@ -1008,7 +1019,7 @@ int discover_git_directory(struct strbuf *commondir,
1019 struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
1020 size_t gitdir_offset = gitdir->len, cwd_len;
1021 size_t commondir_offset = commondir->len;
1011 - struct repository_format candidate;
1022 + struct repository_format candidate = REPOSITORY_FORMAT_INIT;
1023
1024 if (strbuf_getcwd(&dir))
1025 return -1;
@@ -1045,9 +1056,11 @@ int discover_git_directory(struct strbuf *commondir,
1056 strbuf_release(&err);
1057 strbuf_setlen(commondir, commondir_offset);
1058 strbuf_setlen(gitdir, gitdir_offset);
1059 + clear_repository_format(&candidate);
1060 return -1;
1061 }
1062
1063 + clear_repository_format(&candidate);
1064 return 0;
1065 }
1066
@@ -1056,7 +1069,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
1069 static struct strbuf cwd = STRBUF_INIT;
1070 struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;
1071 const char *prefix;
1059 - struct repository_format repo_fmt;
1072 + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
1073
1074 /*
1075 * We may have read an incomplete configuration before
@@ -1146,6 +1159,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
1159
1160 strbuf_release(&dir);
1161 strbuf_release(&gitdir);
1162 + clear_repository_format(&repo_fmt);
1163
1164 return prefix;
1165 }
@@ -1203,9 +1217,10 @@ int git_config_perm(const char *var, const char *value)
1217
1218 void check_repository_format(void)
1219 {
1206 - struct repository_format repo_fmt;
1220 + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
1221 check_repository_format_gently(get_git_dir(), &repo_fmt, NULL);
1222 startup_info->have_repository = 1;
1223 + clear_repository_format(&repo_fmt);
1224 }
1225
1226 /*
worktree.c
+3 -1
@@ -444,7 +444,7 @@ int submodule_uses_worktrees(const char *path)
444 DIR *dir;
445 struct dirent *d;
446 int ret = 0;
447 - struct repository_format format;
447 + struct repository_format format = REPOSITORY_FORMAT_INIT;
448
449 submodule_gitdir = git_pathdup_submodule(path, "%s", "");
450 if (!submodule_gitdir)
@@ -462,8 +462,10 @@ int submodule_uses_worktrees(const char *path)
462 read_repository_format(&format, sb.buf);
463 if (format.version != 0) {
464 strbuf_release(&sb);
465 + clear_repository_format(&format);
466 return 1;
467 }
468 + clear_repository_format(&format);
469
470 /* Replace config by worktrees. */
471 strbuf_setlen(&sb, sb.len - strlen("config"));