V2 of this series: * Addresses all of Peff's comments - many thanks for the review. * Adds a patch converting enum bitfields in parse-options.h to use binary shifts instead of numerical constants. * Adds a patch to free remote_refs in transport_disconnect() instead of freeing those refs in the callers of transport_get_remote_refs(). Andrzej Hunt (9): symbolic-ref: don't leak shortened refname in check_symref() reset: free instead of leaking unneeded ref clone: free or UNLEAK further pointers when finished worktree: fix leak in dwim_branch() init: remove git_init_db_config() while fixing leaks init-db: silence template_dir leak when converting to absolute path parse-options: convert bitfield values to use binary shift parse-options: don't leak alias help messages transport: also free remote_refs in transport_disconnect() builtin/clone.c | 14 ++++++++++---- builtin/init-db.c | 32 ++++++++++---------------------- builtin/ls-remote.c | 4 ++-- builtin/remote.c | 8 ++++---- builtin/reset.c | 2 +- builtin/symbolic-ref.c | 4 +++- builtin/worktree.c | 10 ++++++---- parse-options.c | 20 +++++++++++++++++++- parse-options.h | 35 ++++++++++++++++++----------------- transport.c | 2 ++ 10 files changed, 75 insertions(+), 56 deletions(-) base-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-899%2Fahunt%2Fleaksan-t0001-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-899/ahunt/leaksan-t0001-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/899 Range-diff vs v1: 1: ff0f7c167fa5 ! 1: c7bb403ae381 symbolic-ref: don't leak shortened refname in check_symref() @@ Metadata ## Commit message ## symbolic-ref: don't leak shortened refname in check_symref() + shorten_unambiguous_ref() returns an allocated string. We have to + track it separately from the const refname. + This leak has existed since: 9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21) @@ builtin/symbolic-ref.c: static int check_symref(const char *HEAD, int quiet, int return 1; } if (print) { -- if (shorten) ++ char *to_free = NULL; + if (shorten) - refname = shorten_unambiguous_ref(refname, 0); -- puts(refname); -+ if (shorten) { -+ const char *shortened_refname; -+ -+ shortened_refname = shorten_unambiguous_ref(refname, 0); -+ puts(shortened_refname); -+ free((void *)shortened_refname); -+ } else { -+ puts(refname); -+ } ++ refname = to_free = shorten_unambiguous_ref(refname, 0); + puts(refname); ++ free(to_free); } return 0; } 2: a7b6b873460f ! 2: da05fc72b77a reset: free instead of leaking unneeded ref @@ Metadata ## Commit message ## reset: free instead of leaking unneeded ref - dwim_ref() allocs a new string into ref. Instead of setting to NULL to discard - it, we can FREE_AND_NULL. + dwim_ref() allocs a new string into ref. Instead of setting to NULL to + discard it, we can FREE_AND_NULL. This leak appears to have been introduced in: 4cf76f6bbf (builtin/reset: compute checkout metadata for reset, 2020-03-16) 3: 107e98d00e16 ! 3: a74bbcae7363 clone: free or UNLEAK further pointers when finished @@ Commit message #4 0x51e9bd in wanted_peer_refs /home/ahunt/oss-fuzz/git/builtin/clone.c:574:21 #5 0x51cfe1 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1284:17 #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 - vv #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 + #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 #10 0x69c42e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 #11 0x7f8fef0c2349 in __libc_start_main (/lib64/libc.so.6+0x24349) - Direct leak of 165 byte(s) in 1 object(s) allocated from: - #0 0x49a6b2 in calloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3 - #1 0x9a72f2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8 - #2 0x8ce203 in alloc_ref_with_prefix /home/ahunt/oss-fuzz/git/remote.c:867:20 - #3 0x8ce1a2 in alloc_ref /home/ahunt/oss-fuzz/git/remote.c:875:9 - #4 0x72f63e in process_ref_v2 /home/ahunt/oss-fuzz/git/connect.c:426:8 - #5 0x72f21a in get_remote_refs /home/ahunt/oss-fuzz/git/connect.c:525:8 - #6 0x979ab7 in handshake /home/ahunt/oss-fuzz/git/transport.c:305:4 - #7 0x97872d in get_refs_via_connect /home/ahunt/oss-fuzz/git/transport.c:339:9 - #8 0x9774b5 in transport_get_remote_refs /home/ahunt/oss-fuzz/git/transport.c:1388:4 - #9 0x51cf80 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1271:9 - #10 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11 - #11 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3 - #12 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4 - #13 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19 - #14 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11 - #15 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349) - Direct leak of 178 byte(s) in 1 object(s) allocated from: #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8 @@ Commit message ## builtin/clone.c ## @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix) - char *path, *dir, *display_repo = NULL; + { + int is_bundle = 0, is_local; + const char *repo_name, *repo, *work_tree, *git_dir; +- char *path, *dir, *display_repo = NULL; ++ char *path = NULL, *dir, *display_repo = NULL; int dest_exists, real_dest_exists = 0; const struct ref *refs, *remote_head; - const struct ref *remote_head_points_at; -+ const struct ref *remote_head_points_at = NULL; ++ struct ref *remote_head_points_at = NULL; const struct ref *our_head_points_at; struct ref *mapped_refs; const struct ref *ref; @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix) path = get_repo_path(repo_name, &is_bundle); - if (path) + if (path) { -+ free(path); ++ FREE_AND_NULL(path); repo = absolute_pathdup(repo_name); - else if (strchr(repo_name, ':')) { + } else if (strchr(repo_name, ':')) { @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_release(&branch_top); strbuf_release(&key); + free_refs(mapped_refs); -+ free_refs((void *)remote_head_points_at); -+ free_refs((void *)refs); ++ free_refs(remote_head_points_at); + free(dir); + free(path); + UNLEAK(repo); 4: d46a4e701620 ! 4: a10ab9e68809 worktree: fix leak in dwim_branch() @@ Commit message ## builtin/worktree.c ## @@ builtin/worktree.c: static void print_preparing_worktree_line(int detach, - static const char *dwim_branch(const char *path, const char **new_branch) { -- int n; -+ int n, branch_exists; + int n; ++ int branch_exists; const char *s = worktree_basename(path, &n); const char *branchname = xstrndup(s, n); struct strbuf ref = STRBUF_INIT; -- + UNLEAK(branchname); - if (!strbuf_check_branch_ref(&ref, branchname) && - ref_exists(ref.buf)) { - strbuf_release(&ref); + -+ branch_exists = (!strbuf_check_branch_ref(&ref, branchname) && -+ ref_exists(ref.buf)); ++ branch_exists = !strbuf_check_branch_ref(&ref, branchname) && ++ ref_exists(ref.buf); + strbuf_release(&ref); + if (branch_exists) return branchname; 5: d30365d96765 = 5: 206a82200ca1 init: remove git_init_db_config() while fixing leaks 6: 6f81f3b2ab28 = 6: aa345e50782f init-db: silence template_dir leak when converting to absolute path -: ------------ > 7: 2b03785bd4cb parse-options: convert bitfield values to use binary shift 7: fb456bee0f69 ! 8: 4397c1fd8020 parse-options: don't leak alias help messages @@ Commit message 7c280589cf (parse-options: teach "git cmd -h" to show alias as alias, 2020-03-16) The preprocessed options themselves no longer contain any indication - that a given option is/was an alias: the easiest and fastest way to - figure it out is to look back at the original options. Alternatively we - could iterate over the alias_groups list - but that would require nested - looping and is likely to be a (little) less efficient. + that a given option is/was an alias - therefore we add a new flag to + indicate former aliases. (An alternative approach would be to look back + at the original options to determine which options are aliases - but + that seems like a fragile approach. Or we could even look at the + alias_groups list - which might be less fragile, but would be slower + as it requires nested looping.) As far as I can tell, parse_options() is only ever used once per command, and the help messages are small - hence this leak has very @@ Commit message Signed-off-by: Andrzej Hunt <ajrhunt@xxxxxxxxxx> + fold + ## parse-options.c ## @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all) * @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all */ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx, const struct option *options) +@@ parse-options.c: static struct option *preprocess_options(struct parse_opt_ctx_t *ctx, + newopt[i].short_name = short_name; + newopt[i].long_name = long_name; + newopt[i].help = strbuf_detach(&help, NULL); ++ newopt[i].flags |= PARSE_OPT_FROM_ALIAS; + break; + } + @@ parse-options.c: static struct option *preprocess_options(struct parse_opt_ctx_t *ctx, return newopt; } -+static void free_preprocessed_options(const struct option ** preprocessed_options, const struct option *original_options) ++static void free_preprocessed_options(struct option *options) +{ + int i; + -+ if (!*preprocessed_options) { ++ if (!options) + return; -+ } -+ for (i = 0; original_options[i].type != OPTION_END; i++) { -+ if (original_options[i].type == OPTION_ALIAS) { -+ free((void *)(*preprocessed_options)[i].help); ++ ++ for (i = 0; options[i].type != OPTION_END; i++) { ++ if (options[i].flags & PARSE_OPT_FROM_ALIAS) { ++ free((void *)options[i].help); + } + } -+ free((void *)*preprocessed_options); ++ free(options); +} + static int usage_with_options_internal(struct parse_opt_ctx_t *, const char * const *, const struct option *, int, int); -@@ parse-options.c: int parse_options(int argc, const char **argv, const char *prefix, - int flags) - { - struct parse_opt_ctx_t ctx; -- struct option *real_options; -+ const struct option *preprocessed_options, *original_options = NULL; - - disallow_abbreviated_options = - git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0); - - memset(&ctx, 0, sizeof(ctx)); -- real_options = preprocess_options(&ctx, options); -- if (real_options) -- options = real_options; -+ preprocessed_options = preprocess_options(&ctx, options); -+ if (preprocessed_options) { -+ original_options = options; -+ options = preprocessed_options; -+ } - parse_options_start_1(&ctx, argc, argv, prefix, options, flags); - switch (parse_options_step(&ctx, options, usagestr)) { - case PARSE_OPT_HELP: @@ parse-options.c: int parse_options(int argc, const char **argv, const char *prefix, } precompose_argv_prefix(argc, argv, NULL); - free(real_options); -+ free_preprocessed_options(&preprocessed_options, original_options); ++ free_preprocessed_options(real_options); free(ctx.alias_groups); return parse_options_end(&ctx); } + + ## parse-options.h ## +@@ parse-options.h: enum parse_opt_option_flags { + PARSE_OPT_NOCOMPLETE = 1 << 8, + PARSE_OPT_COMP_ARG = 1 << 9, + PARSE_OPT_CMDMODE = 1 << 10, ++ PARSE_OPT_FROM_ALIAS = 1 << 11, + }; + + enum parse_opt_result { -: ------------ > 9: a907f2460d42 transport: also free remote_refs in transport_disconnect() -- gitgitgadget
