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@google.com>
+ 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()
Andrzej Hunt via GitGitGadget
