Memory leak fixes (pt.3)
cover Memory leak fixes (pt.3)
Hi,
I originally wanted to hold off with sending out this series until v2.46 was out. But I saw that Junio sent out some patches which are plugging the same leaks as I did, so I dedcided to send it out now to avoid some duplicated work.
There isn't really any structure to this series, I just happened to pick some random test suites that fail with the leak checker enabled and then fixed those. Naturally, I've also got part 4 of this series of patch series in the pipeline already :) As mentioned elsewhere, I hope to get the number of failing test suites to zero this year. Let's see whether this is realistic.
Patrick
Patrick Steinhardt (23):
builtin/replay: plug leaking advance_name variable builtin/log: fix leaking branch name when creating cover letters builtin/describe: fix memory leak with --contains= builtin/describe: fix leaking array when running diff-index builtin/describe: fix trivial memory leak when describing blob builtin/name-rev: fix various trivial memory leaks builtin/submodule--helper: fix various trivial memory leaks builtin/ls-remote: fix leaking pattern strings builtin/remote: fix leaking strings in branch_list builtin/remote: fix various trivial memory leaks builtin/stash: fix various trivial memory leaks builtin/rev-parse: fix memory leak with --parseopt builtin/show-branch: fix several memory leaks builtin/credential-store: fix leaking credential builtin/rerere: fix various trivial memory leaks builtin/shortlog: fix various trivial memory leaks builtin/worktree: fix leaking derived branch names builtin/credential-cache: fix trivial leaks t/test-repository: fix leaking repository
object-name: fix leaking commit list items
entry: fix leaking pathnames during delayed checkout
convert: fix leaking config strings
commit-reach: fix trivial memory leak when computing reachability
builtin/credential-cache.c | 9 ++++- builtin/credential-store.c | 1 + builtin/describe.c | 25 ++++++++++-- builtin/log.c | 4 +- builtin/ls-remote.c | 11 ++++-- builtin/name-rev.c | 6 ++- builtin/remote.c | 44 ++++++++++++++++----- builtin/replay.c | 20 +++++++--- builtin/rerere.c | 8 +++- builtin/rev-parse.c | 5 ++- builtin/shortlog.c | 1 + builtin/show-branch.c | 52 +++++++++++++++++-------- builtin/stash.c | 18 ++++++++- builtin/submodule--helper.c | 13 +++++-- builtin/worktree.c | 7 ++-- commit-reach.c | 1 + convert.c | 14 +++++-- entry.c | 4 +- object-name.c | 26 ++++++++----- rerere.c | 9 ++++- t/helper/test-repository.c | 4 +- t/t0021-conversion.sh | 1 + t/t0301-credential-cache.sh | 2 + t/t0302-credential-store.sh | 2 + t/t0303-credential-external.sh | 1 + t/t1502-rev-parse-parseopt.sh | 2 + t/t1511-rev-parse-caret.sh | 1 + t/t2030-unresolve-info.sh | 1 + t/t2080-parallel-checkout-basics.sh | 1 + t/t2082-parallel-checkout-attributes.sh | 1 + t/t2400-worktree-add.sh | 1 + t/t2501-cwd-empty.sh | 1 + t/t3201-branch-contains.sh | 1 + t/t3202-show-branch.sh | 1 + t/t3206-range-diff.sh | 1 + t/t3650-replay-basics.sh | 1 + t/t3903-stash.sh | 1 + t/t3904-stash-patch.sh | 2 + t/t3905-stash-include-untracked.sh | 1 + t/t4200-rerere.sh | 1 + t/t4201-shortlog.sh | 1 + t/t5318-commit-graph.sh | 2 + t/t5512-ls-remote.sh | 1 + t/t5514-fetch-multiple.sh | 1 + t/t5520-pull.sh | 1 + t/t5528-push-default.sh | 1 + t/t5535-fetch-push-symref.sh | 1 + t/t5543-atomic-push.sh | 1 + t/t5570-git-daemon.sh | 1 + t/t6007-rev-list-cherry-pick-file.sh | 1 + t/t6010-merge-base.sh | 1 + t/t6120-describe.sh | 1 + t/t6133-pathspec-rev-dwim.sh | 2 + t/t7064-wtstatus-pv2.sh | 1 + t/t7400-submodule-basic.sh | 1 + t/t9902-completion.sh | 1 + t/t9903-bash-prompt.sh | 1 + 57 files changed, 251 insertions(+), 73 deletions(-)
Signature · 1 line
2.46.0.rc1.dirty
PATCH 1/23 builtin/replay: plug leaking `advance_name` variable
The advance_name variable can either contain a static string when parsed via the --advance command line option or it may be an allocated string when set via determine_replay_mode(). Because we cannot be sure whether it is allocated or not we just didn't free it at all, resulting in a memory leak.
Split up the variables such that we can track the static and allocated strings separately and then free the allocated one to fix the memory leak.
--- builtin/replay.c | 20 ++++++++++++++------ t/t3650-replay-basics.sh | 1 + 2 files changed, 15 insertions(+), 6 deletions(-)
builtin/replay.c +14 -6
@@ -151,7 +151,7 @@ static void get_ref_information(struct rev_cmdline_info *cmd_info,static void determine_replay_mode (struct rev_cmdline_info *cmd_info,const char *onto_name, -const char **advance_name, +char **advance_name,struct commit **onto,struct strset **update_refs) {
@@ -174,6 +174,7 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info, *onto =peel_committish (*advance_name);if (repo_dwim_ref (the_repository, *advance_name,strlen (*advance_name), &oid, &fullname,0 ) ==1 ) { +free (*advance_name); *advance_name = fullname; }else {die (_ ("argument to --advance must be a reference" ));
@@ -197,6 +198,7 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,if (negative_refs_complete) {struct hashmap_iter iter;struct strmap_entry *entry; +const char *last_key =NULL ;if (rinfo.negative_refexprs ==0 )die (_ ("all positive revisions given must be references" ));
@@ -208,8 +210,11 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,/* Only one entry, but we have to loop to get it */ strset_for_each_entry (&rinfo.negative_refs, &iter, entry) { - *advance_name = entry->key; + last_key = entry->key; } + +free (*advance_name); + *advance_name =xstrdup_or_null (last_key); }else {/* positive_refs_complete */ if (rinfo.negative_refexprs >1 )die (_ ("cannot implicitly determine correct base for --onto" ));
@@ -271,7 +276,8 @@ static struct commit *pick_regular_commit(struct commit *pickme,int cmd_replay (int argc,const char **argv,const char *prefix) { -const char *advance_name =NULL ; +const char *advance_name_opt =NULL ; +char *advance_name =NULL ;struct commit *onto =NULL ;const char *onto_name =NULL ;int contained =0 ;
@@ -292,7 +298,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)NULL };struct option replay_options[] = { -OPT_STRING (0 ,"advance" , &advance_name, +OPT_STRING (0 ,"advance" , &advance_name_opt,N_ ("branch" ),N_ ("make replay advance given branch" )), OPT_STRING(0, "onto", &onto_name,
@@ -306,14 +312,15 @@ int cmd_replay(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, replay_options, replay_usage,PARSE_OPT_KEEP_ARGV0 |PARSE_OPT_KEEP_UNKNOWN_OPT ); -if (!onto_name && !advance_name) { +if (!onto_name && !advance_name_opt) {error (_ ("option --onto or --advance is mandatory" ));usage_with_options (replay_usage, replay_options); } -if (advance_name && contained) +if (advance_name_opt && contained)die (_ ("options '%s' and '%s' cannot be used together" ),"--advance" ,"--contained" ); + advance_name =xstrdup_or_null (advance_name_opt);repo_init_revisions (the_repository, &revs, prefix);
@@ -441,6 +448,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix) cleanup:release_revisions (&revs); +free (advance_name);/* Return */ if (ret <0 )
t/t3650-replay-basics.sh +1 -0
@@ -5,6 +5,7 @@ test_description='basic git replay tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_AUTHOR_NAME=author@name
Signature · 1 line
2.46.0.rc1.dirty
PATCH 2/23 builtin/log: fix leaking branch name when creating cover letters
In reply to Patrick SteinhardtWhen calling make_cover_letter() without a branch name, then we try to derive the branch name by calling find_branch_name(). But while this function returns an allocated string, we never free the result and thus have a memory leak. Fix this.
--- builtin/log.c | 4 +++- t/t3206-range-diff.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
builtin/log.c +3 -1
@@ -1434,6 +1434,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,int need_8bit_cte =0 ;struct pretty_print_context pp = {0 };struct commit *head = list[0 ]; +char *to_free =NULL ;if (!cmit_fmt_is_mail (rev->commit_format))die (_ ("cover letter needs email format" ));
@@ -1455,7 +1456,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, }if (!branch_name) - branch_name =find_branch_name (rev); + branch_name = to_free =find_branch_name (rev); pp.fmt =CMIT_FMT_EMAIL ; pp.date_mode.type =DATE_RFC2822 ;
@@ -1466,6 +1467,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, encoding, need_8bit_cte, cfg);fprintf (rev->diffopt.file,"%s\n" , sb.buf); +free (to_free);free (pp.after_subject);strbuf_release (&sb);
t/t3206-range-diff.sh +1 -0
@@ -5,6 +5,7 @@ test_description='range-diff tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Note that because of the range-diff's heuristics, test_commit does more
Signature · 1 line
2.46.0.rc1.dirty
PATCH 3/23 builtin/describe: fix memory leak with `--contains=`
In reply to Patrick SteinhardtWhen calling git describe --contains=, we end up invoking cmd_name_rev() with some munged argv array. This array may contain allocated strings and furthermore will likely be modified by the called function. This results in two memory leaks:
- First, we leak the array that we use to assemble the arguments.
- Second, we leak the allocated strings that we may have put into the array.
Fix those leaks by creating a separate copy of the array that we can hand over to cmd_name_rev(). This allows us to free all strings contained in the strvec, as the original vector will not be modified anymore.
Furthermore, free both the strvec and the copied array to fix the first memory leak.
--- builtin/describe.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
builtin/describe.c +17 -1
@@ -619,6 +619,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)if (contains) {struct string_list_item *item;struct strvec args; +const char **argv_copy; +int ret;strvec_init (&args); strvec_pushl(&args, "name-rev",
@@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)strvec_pushv (&args, argv); else strvec_push(&args, "HEAD"); -return cmd_name_rev (args.nr, args.v, prefix); + +/* +* `cmd_name_rev()` modifies the array, so we'd link its +* contained strings if we didn't do a copy here. +*/ +ALLOC_ARRAY (argv_copy, args.nr +1 ); +for (size_t i =0 ; i < args.nr; i++) + argv_copy[i] = args.v[i]; + argv_copy[args.nr] =NULL ; + + ret =cmd_name_rev (args.nr, argv_copy, prefix); + +strvec_clear (&args); +free (argv_copy); +return ret; }hashmap_init (&names, commit_name_neq,NULL ,0 );
Signature · 1 line
2.46.0.rc1.dirty
PATCH 4/23 builtin/describe: fix leaking array when running diff-index
In reply to Patrick SteinhardtWhen running git-describe(1) with --dirty, we will set up a struct rev_info with arguments for git-diff-index(1). The way we assmble the arguments it causes two memory leaks though:
- We never release the struct strvec.
- setup_revisions() may end up removing some entries from the strvec, which we wouldn't free even if we released the struct.
While we could plug those leaks, this is ultimately unnecessary as the arguments we pass are part of a static array anyway. So instead, refactor the code to drop the struct strvec and just pass this static array directly.
--- builtin/describe.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
builtin/describe.c +3 -3
@@ -695,7 +695,6 @@ int cmd_describe(int argc, const char **argv, const char *prefix) }else if (dirty) {struct lock_file index_lock =LOCK_INIT ;struct rev_info revs; -struct strvec args =STRVEC_INIT ;int fd;setup_work_tree ();
@@ -710,8 +709,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)repo_update_index_if_able (the_repository, &index_lock);repo_init_revisions (the_repository, &revs, prefix); -strvec_pushv (&args, diff_index_args); -if (setup_revisions (args.nr, args.v, &revs,NULL ) !=1 ) + +if (setup_revisions (ARRAY_SIZE (diff_index_args) -1 , + diff_index_args, &revs,NULL ) !=1 )BUG ("malformed internal diff-index command line" );run_diff_index (&revs,0 );
Signature · 1 line
2.46.0.rc1.dirty
PATCH 5/23 builtin/describe: fix trivial memory leak when describing blob
In reply to Patrick SteinhardtWe never free the struct strvec args variable in describe_blob(), which thus causes a memory leak. Fix this.
--- builtin/describe.c | 1 + t/t9903-bash-prompt.sh | 1 + 2 files changed, 2 insertions(+)
builtin/describe.c +1 -0
@@ -529,6 +529,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)traverse_commit_list (&revs, process_commit, process_object, &pcd);reset_revision_walk ();release_revisions (&revs); +strvec_clear (&args); }static void describe (const char *arg,int last_one)
t/t9903-bash-prompt.sh +1 -0
@@ -8,6 +8,7 @@ test_description='test git-specific bash prompt functions' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-bash.sh . "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"
Signature · 1 line
2.46.0.rc1.dirty
PATCH 6/23 builtin/name-rev: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are several structures that we don't release after cmd_name_rev() is done. Plug those leaks.
--- builtin/name-rev.c | 6 ++++-- t/t6007-rev-list-cherry-pick-file.sh | 1 + t/t6120-describe.sh | 1 + 3 files changed, 6 insertions(+), 2 deletions(-)
builtin/name-rev.c +4 -2
@@ -677,7 +677,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) always, allow_undefined, data.name_only); } -UNLEAK (string_pool); -UNLEAK (revs); +string_list_clear (&data.ref_filters,0 ); +string_list_clear (&data.exclude_filters,0 ); +mem_pool_discard (&string_pool,0 ); +object_array_clear (&revs);return 0 ; }
t/t6007-rev-list-cherry-pick-file.sh +1 -0
@@ -5,6 +5,7 @@ test_description='test git rev-list --cherry-pick -- file' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # A---B---D---F
t/t6120-describe.sh +1 -0
@@ -14,6 +14,7 @@ test_description='test describe' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_describe () {
Signature · 1 line
2.46.0.rc1.dirty
PATCH 7/23 builtin/submodule--helper: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are multiple trivial memory leaks in the submodule helper. Fix those.
--- builtin/submodule--helper.c | 13 ++++++++++--- t/t7400-submodule-basic.sh | 1 + 2 files changed, 11 insertions(+), 3 deletions(-)
builtin/submodule--helper.c +10 -3
@@ -2269,6 +2269,7 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf rev =STRBUF_INIT ;char *hex =oid_to_hex (oid); +int reachable; cp.git_cmd =1 ; cp.dir = path;
@@ -2278,9 +2279,12 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)prepare_submodule_repo_env (&cp.env);if (capture_command (&cp, &rev,GIT_MAX_HEXSZ +1 ) || rev.len) -return 0 ; + reachable =0 ; +else + reachable =1 ; -return 1 ; +strbuf_release (&rev); +return reachable; } static int fetch_in_submodule(const char *module_path, int depth, int quiet,
@@ -3135,6 +3139,7 @@ static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)static int add_submodule (const struct add_data *add_data) {char *submod_gitdir_path; +char *depth_to_free =NULL ;struct module_clone_data clone_data =MODULE_CLONE_DATA_INIT ;struct string_list reference =STRING_LIST_INIT_NODUP ;int ret =-1 ;
@@ -3200,7 +3205,7 @@ static int add_submodule(const struct add_data *add_data) } clone_data.dissociate = add_data->dissociate;if (add_data->depth >=0 ) - clone_data.depth =xstrfmt ("%d" , add_data->depth); + clone_data.depth = depth_to_free =xstrfmt ("%d" , add_data->depth);if (clone_submodule (&clone_data, &reference)) goto cleanup;
@@ -3223,7 +3228,9 @@ static int add_submodule(const struct add_data *add_data)die (_ ("unable to checkout submodule '%s'" ), add_data->sm_path); } ret =0 ; + cleanup: +free (depth_to_free);string_list_clear (&reference,1 );return ret; }
t/t7400-submodule-basic.sh +1 -0
@@ -12,6 +12,7 @@ subcommands of git submodule. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup - enable local submodules' '
Signature · 1 line
2.46.0.rc1.dirty
PATCH 8/23 builtin/ls-remote: fix leaking `pattern` strings
In reply to Patrick SteinhardtUsers can pass patterns to git-ls-remote(1), which allows them to filter the list of printed references. We assemble those patterns into an array and prefix them with "*/", but never free either the array nor the allocated strings.
Fix those leaks.
--- builtin/ls-remote.c | 11 +++++++---- t/t5535-fetch-push-symref.sh | 1 + 2 files changed, 8 insertions(+), 4 deletions(-)
builtin/ls-remote.c +7 -4
@@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)int status =0 ;int show_symref_target =0 ;const char *uploadpack =NULL ; -const char **pattern =NULL ; +char **pattern =NULL ;struct transport_ls_refs_options transport_options =TRANSPORT_LS_REFS_OPTIONS_INIT ;int i;
@@ -96,9 +96,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)if (argc >1 ) {int i;CALLOC_ARRAY (pattern, argc); -for (i =1 ; i < argc; i++) { +for (i =1 ; i < argc; i++) pattern[i -1 ] =xstrfmt ("*/%s" , argv[i]); - } }if (flags &REF_TAGS )
@@ -136,7 +135,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)struct ref_array_item *item;if (!check_ref_type (ref, flags))continue ; -if (!tail_match (pattern, ref->name)) +if (!tail_match ((const char **) pattern, ref->name))continue ; item =ref_array_push (&ref_array, ref->name, &ref->old_oid); item->symref =xstrdup_or_null (ref->symref);
@@ -158,5 +157,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)if (transport_disconnect (transport)) status =1 ;transport_ls_refs_options_release (&transport_options); + +for (i =1 ; i < argc; i++) +free (pattern[i -1 ]); +free (pattern);return status; }
t/t5535-fetch-push-symref.sh +1 -0
@@ -2,6 +2,7 @@ test_description='avoiding conflicting update through symref aliasing' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.rc1.dirty
PATCH 9/23 builtin/remote: fix leaking strings in `branch_list`
In reply to Patrick SteinhardtThe struct string_list branch_list is declared as NODUP, which makes it not copy strings inserted into it. This causes memory leaks though, as this means it also won't be responsible for _freeing_ inserted strings. Thus, every branch we add to this will leak.
Fix this by marking the list as DUP instead and free the local copy we have of the variable.
--- builtin/remote.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
builtin/remote.c +3 -2
@@ -258,7 +258,7 @@ struct branch_info {char *push_remote_name; }; -static struct string_list branch_list =STRING_LIST_INIT_NODUP ; +static struct string_list branch_list =STRING_LIST_INIT_DUP ;static const char *abbrev_ref (const char *name,const char *prefix) {
@@ -292,8 +292,8 @@ static int config_read_branches(const char *key, const char *value, type =PUSH_REMOTE ; else return 0; - name =xmemdupz (key, key_len); + name =xmemdupz (key, key_len); item =string_list_insert (&branch_list, name);if (!item->util)
@@ -337,6 +337,7 @@ static int config_read_branches(const char *key, const char *value,BUG ("unexpected type=%d" , type); } +free (name);return 0 ; }
Signature · 1 line
2.46.0.rc1.dirty
PATCH 10/23 builtin/remote: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are multiple trivial memory leaks in git-remote(1). Fix those.
--- builtin/remote.c | 39 ++++++++++++++++++++++++++++++++------- t/t5512-ls-remote.sh | 1 + t/t5514-fetch-multiple.sh | 1 + t/t5520-pull.sh | 1 + t/t5528-push-default.sh | 1 + t/t5543-atomic-push.sh | 1 + t/t5570-git-daemon.sh | 1 + 7 files changed, 38 insertions(+), 7 deletions(-)
builtin/remote.c +32 -7
@@ -555,13 +555,16 @@ static int add_branch_for_removal(const char *refname, refspec.dst = (char *)refname;if (remote_find_tracking (branches->remote, &refspec))return 0 ; +free (refspec.src);/* don't delete a branch if another remote also uses it */ for (kr = branches->keep->list; kr; kr = kr->next) {memset (&refspec,0 ,sizeof (refspec)); refspec.dst = (char *)refname; -if (!remote_find_tracking (kr->remote, &refspec)) +if (!remote_find_tracking (kr->remote, &refspec)) { +free (refspec.src);return 0 ; + } }/* don't delete non-remote-tracking refs */
@@ -668,7 +671,11 @@ static int config_read_push_default(const char *key, const char *value,static void handle_push_default (const char * old_name,const char * new_name) {struct push_default_info push_default = { - old_name,CONFIG_SCOPE_UNKNOWN ,STRBUF_INIT ,-1 }; + .old_name = old_name, + .scope =CONFIG_SCOPE_UNKNOWN , + .origin =STRBUF_INIT , + .linenr =-1 , + };git_config (config_read_push_default, &push_default);if (push_default.scope >=CONFIG_SCOPE_COMMAND ) ;/* pass */
@@ -688,6 +695,8 @@ static void handle_push_default(const char* old_name, const char* new_name) push_default.origin.buf, push_default.linenr, old_name); } + +strbuf_release (&push_default.origin); }
@@ -785,7 +794,7 @@ static int mv(int argc, const char **argv, const char *prefix) }if (!refspec_updated) -return 0 ; + goto out;/* * First remove symrefs, then rename the rest, finally create
@@ -851,10 +860,15 @@ static int mv(int argc, const char **argv, const char *prefix)display_progress (progress, ++refs_renamed_nr); }stop_progress (&progress); -string_list_clear (&remote_branches,1 );handle_push_default (rename.old_name, rename.new_name); +out: +string_list_clear (&remote_branches,1 ); +strbuf_release (&old_remote_context); +strbuf_release (&buf); +strbuf_release (&buf2); +strbuf_release (&buf3);return 0 ; }
@@ -945,12 +959,21 @@ static int rm(int argc, const char **argv, const char *prefix)if (!result) {strbuf_addf (&buf,"remote.%s" , remote->name); -if (git_config_rename_section (buf.buf,NULL ) <1 ) -return error (_ ("Could not remove config section '%s'" ), buf.buf); +if (git_config_rename_section (buf.buf,NULL ) <1 ) { + result =error (_ ("Could not remove config section '%s'" ), buf.buf); + goto out; + }handle_push_default (remote->name,NULL ); } +out: +for (struct known_remote *r = known_remotes.list; r;) { +struct known_remote *next = r->next; +free (r); + r = next; + } +strbuf_release (&buf);return result; }
@@ -983,8 +1006,10 @@ static int append_ref_to_tracked_list(const char *refname,memset (&refspec,0 ,sizeof (refspec)); refspec.dst = (char *)refname; -if (!remote_find_tracking (states->remote, &refspec)) +if (!remote_find_tracking (states->remote, &refspec)) {string_list_append (&states->tracked,abbrev_branch (refspec.src)); +free (refspec.src); + }return 0 ; }
t/t5512-ls-remote.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git ls-remote' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh generate_references () {
t/t5514-fetch-multiple.sh +1 -0
@@ -5,6 +5,7 @@ test_description='fetch --all works correctly' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh setup_repository () {
t/t5520-pull.sh +1 -0
@@ -5,6 +5,7 @@ test_description='pulling into void' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh modify () {
t/t5528-push-default.sh +1 -0
@@ -4,6 +4,7 @@ test_description='check various push.default settings' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup bare remotes' '
t/t5543-atomic-push.sh +1 -0
@@ -5,6 +5,7 @@ test_description='pushing to a repository using the atomic push option' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh mk_repo_pair () {
t/t5570-git-daemon.sh +1 -0
@@ -4,6 +4,7 @@ test_description='test fetching over git protocol' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-git-daemon.sh
Signature · 1 line
2.46.0.rc1.dirty
PATCH 11/23 builtin/stash: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are multiple trivial memory leaks in git-stash(1). Fix those.
--- builtin/stash.c | 18 ++++++++++++++++-- t/t2501-cwd-empty.sh | 1 + t/t3903-stash.sh | 1 + t/t3904-stash-patch.sh | 2 ++ t/t3905-stash-include-untracked.sh | 1 + t/t7064-wtstatus-pv2.sh | 1 + 6 files changed, 22 insertions(+), 2 deletions(-)
builtin/stash.c +16 -2
@@ -1521,6 +1521,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int qstruct strbuf patch =STRBUF_INIT ;struct strbuf stash_msg_buf =STRBUF_INIT ;struct strbuf untracked_files =STRBUF_INIT ; +struct strbuf out =STRBUF_INIT ;if (patch_mode && keep_index ==-1 ) keep_index =1 ;
@@ -1626,7 +1627,6 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int qstruct child_process cp_add =CHILD_PROCESS_INIT ;struct child_process cp_diff =CHILD_PROCESS_INIT ;struct child_process cp_apply =CHILD_PROCESS_INIT ; -struct strbuf out =STRBUF_INIT ; cp_add.git_cmd =1 ;strvec_push (&cp_add.args,"add" );
@@ -1718,6 +1718,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q done:strbuf_release (&patch); +strbuf_release (&out);free_stash_info (&info);strbuf_release (&stash_msg_buf);strbuf_release (&untracked_files);
@@ -1869,6 +1870,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)OPT_SUBCOMMAND_F ("save" , &fn, save_stash,PARSE_OPT_NOCOMPLETE ), OPT_END() }; +const char **args_copy; +int ret;git_config (git_stash_config,NULL );
@@ -1892,5 +1895,16 @@ int cmd_stash(int argc, const char **argv, const char *prefix)/* Assume 'stash push' */ strvec_push (&args,"push" );strvec_pushv (&args, argv); -return !!push_stash (args.nr, args.v, prefix,1 ); + +/* +* `push_stash()` ends up modifying the array, which causes memory +* leaks if we didn't copy the array here. +*/ +DUP_ARRAY (args_copy, args.v, args.nr); + + ret = !!push_stash (args.nr, args_copy, prefix,1 ); + +strvec_clear (&args); +free (args_copy); +return ret; }
t/t2501-cwd-empty.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Test handling of the current working directory becoming empty' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup '
t/t3903-stash.sh +1 -0
@@ -8,6 +8,7 @@ test_description='Test git stash' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-unique-files.sh
t/t3904-stash-patch.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='stash -p' + +TEST_PASSES_SANITIZE_LEAK=true . ./lib-patch-mode.sh test_expect_success 'setup' '
t/t3905-stash-include-untracked.sh +1 -0
@@ -5,6 +5,7 @@ test_description='Test git stash --include-untracked' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'stash save --include-untracked some dirty working directory' '
t/t7064-wtstatus-pv2.sh +1 -0
@@ -4,6 +4,7 @@ test_description='git status --porcelain=v2 This test exercises porcelain V2 output for git status.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh
Signature · 1 line
2.46.0.rc1.dirty
PATCH 12/23 builtin/rev-parse: fix memory leak with `--parseopt`
In reply to Patrick SteinhardtThe --parseopt mode allows shell scripts to have the same option parsing mode as we have in C builtins. It soaks up a set of option descriptions via stdin and massages them into proper struct options that we can then use to parse a set of arguments.
We only partially free those options when done though, creating a memory leak. Interestingly, we only end up free'ing the first option's help, which is of course wrong.
Fix this by freeing all option's help fields as well as their argh fielids to plug this memory leak.
--- builtin/rev-parse.c | 5 ++++- t/t1502-rev-parse-parseopt.sh | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-)
builtin/rev-parse.c +4 -1
@@ -553,7 +553,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)strbuf_release (&sb);strvec_clear (&longnames);strvec_clear (&usage); -free ((char *) opts->help); +for (size_t i =0 ; i < opts_nr; i++) { +free ((char *) opts[i].help); +free ((char *) opts[i].argh); + }free (opts);return 0 ; }
t/t1502-rev-parse-parseopt.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='test git rev-parse --parseopt' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_invalid_long_option () {
Signature · 1 line
2.46.0.rc1.dirty
PATCH 13/23 builtin/show-branch: fix several memory leaks
In reply to Patrick SteinhardtThere are several memory leaks in git-show-branch(1). Fix them.
--- builtin/show-branch.c | 52 +++++++++++++++++++++++++++++------------- t/t3202-show-branch.sh | 1 + t/t6010-merge-base.sh | 1 + 3 files changed, 38 insertions(+), 16 deletions(-)
builtin/show-branch.c +36 -16
@@ -502,14 +502,14 @@ static int rev_is_head(const char *head, const char *name)return !strcmp (head, name); } -static int show_merge_base (struct commit_list *seen,int num_rev) +static int show_merge_base (const struct commit_list *seen,int num_rev) {int all_mask = ((1u << (REV_SHIFT + num_rev)) -1 );int all_revs = all_mask & ~((1u <<REV_SHIFT ) -1 );int exit_status =1 ; -while (seen) { -struct commit *commit =pop_commit (&seen); +for (const struct commit_list *s = seen; s; s = s->next) { +struct commit *commit = s->item;int flags = commit->object.flags & all_mask;if (!(flags &UNINTERESTING ) && ((flags & all_revs) == all_revs)) {
@@ -635,7 +635,7 @@ static int parse_reflog_param(const struct option *opt, const char *arg,int cmd_show_branch (int ac,const char **av,const char *prefix) {struct commit *rev[MAX_REVS ], *commit; -char *reflog_msg[MAX_REVS ]; +char *reflog_msg[MAX_REVS ] = {0 };struct commit_list *list =NULL , *seen =NULL ;unsigned int rev_mask[MAX_REVS ];int num_rev, i, extra =0 ;
@@ -692,6 +692,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) parse_reflog_param), OPT_END() }; +const char **args_copy =NULL ; +int ret;init_commit_name_slab (&name_slab);
@@ -699,8 +701,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)/* If nothing is specified, try the default first */ if (ac ==1 && default_args.nr) { +DUP_ARRAY (args_copy, default_args.v, default_args.nr); ac = default_args.nr; - av = default_args.v; + av = args_copy; } ac = parse_options(ac, av, prefix, builtin_show_branch_options,
@@ -780,7 +783,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) }for (i =0 ; i < reflog; i++) { -char *logmsg; +char *logmsg =NULL ;char *nth_desc;const char *msg;char *end;
@@ -790,6 +793,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)if (read_ref_at (get_main_ref_store (the_repository), ref, flags,0 , base + i, &oid, &logmsg, ×tamp, &tz,NULL )) { +free (logmsg); reflog = i;break ; }
@@ -842,7 +846,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)if (!ref_name_cnt) {fprintf (stderr,"No revs to be shown.\n" ); -exit (0 ); + ret =0 ; + goto out; }for (num_rev =0 ; ref_name[num_rev]; num_rev++) {
@@ -879,11 +884,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)commit_list_sort_by_date (&seen); -if (merge_base) -return show_merge_base (seen, num_rev); +if (merge_base) { + ret =show_merge_base (seen, num_rev); + goto out; + } -if (independent) -return show_independent (rev, num_rev, rev_mask); +if (independent) { + ret =show_independent (rev, num_rev, rev_mask); + goto out; + }/* Show list; --more=-1 means list-only */ if (1 < num_rev || extra <0 ) {
@@ -919,8 +928,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)putchar ('\n' ); } } -if (extra <0 ) -exit (0 ); +if (extra <0 ) { + ret =0 ; + goto out; + }/* Sort topologically */ sort_in_topological_order (&seen, sort_order);
@@ -932,8 +943,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) all_mask = ((1u << (REV_SHIFT + num_rev)) -1 ); all_revs = all_mask & ~((1u <<REV_SHIFT ) -1 ); -while (seen) { -struct commit *commit =pop_commit (&seen); +for (struct commit_list *l = seen; l; l = l->next) { +struct commit *commit = l->item;int this_flag = commit->object.flags;int is_merge_point = ((this_flag & all_revs) == all_revs);
@@ -973,6 +984,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)if (shown_merge_point && --extra <0 )break ; } + + ret =0 ; + +out: +for (size_t i =0 ; i <ARRAY_SIZE (reflog_msg); i++) +free (reflog_msg[i]); +free_commit_list (seen); +free_commit_list (list); +free (args_copy);free (head); -return 0 ; +return ret; }
t/t3202-show-branch.sh +1 -0
@@ -2,6 +2,7 @@ test_description='test show-branch' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'error descriptions on empty repository' '
t/t6010-merge-base.sh +1 -0
@@ -6,6 +6,7 @@ test_description='Merge base and parent list computation. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh M=1130000000
Signature · 1 line
2.46.0.rc1.dirty
PATCH 14/23 builtin/credential-store: fix leaking credential
In reply to Patrick SteinhardtWe never free credentials read by the credential store, leading to a memory leak. Plug it.
--- builtin/credential-store.c | 1 + t/t0302-credential-store.sh | 2 ++ t/t0303-credential-external.sh | 1 + 3 files changed, 4 insertions(+)
builtin/credential-store.c +1 -0
@@ -218,5 +218,6 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix) ;/* Ignore unknown operation. */ string_list_clear (&fns,0 ); +credential_clear (&c);return 0 ; }
t/t0302-credential-store.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='credential-store tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-credential.sh
t/t0303-credential-external.sh +1 -0
@@ -29,6 +29,7 @@ you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell commands. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-credential.sh
Signature · 1 line
2.46.0.rc1.dirty
PATCH 15/23 builtin/rerere: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are multiple trivial memory leaks in git-rerere(1). Fix those.
--- builtin/rerere.c | 8 +++++++- rerere.c | 9 +++++++-- t/t2030-unresolve-info.sh | 1 + t/t4200-rerere.sh | 1 + 4 files changed, 16 insertions(+), 3 deletions(-)
builtin/rerere.c +7 -1
@@ -73,11 +73,17 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)if (!strcmp (argv[0 ],"forget" )) {struct pathspec pathspec; +int ret; +if (argc <2 )warning (_ ("'git rerere forget' without paths is deprecated" ));parse_pathspec (&pathspec,0 ,PATHSPEC_PREFER_CWD , prefix, argv +1 ); -return rerere_forget (the_repository, &pathspec); + + ret =rerere_forget (the_repository, &pathspec); + +clear_pathspec (&pathspec); +return ret; }if (!strcmp (argv[0 ],"clear" )) {
rerere.c +7 -2
@@ -1107,7 +1107,7 @@ static int rerere_forget_one_path(struct index_state *istate,int rerere_forget (struct repository *r,struct pathspec *pathspec) { -int i, fd; +int i, fd, ret;struct string_list conflict =STRING_LIST_INIT_DUP ;struct string_list merge_rr =STRING_LIST_INIT_DUP ;
@@ -1132,7 +1132,12 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec)continue ;rerere_forget_one_path (r->index, it->string, &merge_rr); } -return write_rr (&merge_rr, fd); + + ret =write_rr (&merge_rr, fd); + +string_list_clear (&conflict,0 ); +string_list_clear (&merge_rr,1 ); +return ret; }/*
t/t2030-unresolve-info.sh +1 -0
@@ -5,6 +5,7 @@ test_description='undoing resolution' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_resolve_undo () {
t/t4200-rerere.sh +1 -0
@@ -25,6 +25,7 @@ test_description='git rerere GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.rc1.dirty
PATCH 16/23 builtin/shortlog: fix various trivial memory leaks
In reply to Patrick SteinhardtThere is a trivial memory leak in git-shortlog(1). Fix it.
--- builtin/shortlog.c | 1 + t/t4201-shortlog.sh | 1 + 2 files changed, 2 insertions(+)
builtin/shortlog.c +1 -0
@@ -514,4 +514,5 @@ void shortlog_output(struct shortlog *log)string_list_clear (&log->list,1 );clear_mailmap (&log->mailmap);string_list_clear (&log->format,0 ); +string_list_clear (&log->trailers,0 ); }
t/t4201-shortlog.sh +1 -0
@@ -9,6 +9,7 @@ test_description='git shortlog GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.rc1.dirty
PATCH 17/23 builtin/worktree: fix leaking derived branch names
In reply to Patrick SteinhardtThere are several heuristics that git-worktree(1) uses to derive the name of the newly created branch when not given explicitly. These heuristics all allocate a new string, but we only end up freeing that string in a subset of cases.
Fix the remaining cases where we didn't yet free the derived branch names. While at it, also free opt_track, which is being populated via an OPT_PASSTHRU().
--- builtin/worktree.c | 7 ++++--- t/t2400-worktree-add.sh | 1 + t/t9902-completion.sh | 1 + 3 files changed, 6 insertions(+), 3 deletions(-)
builtin/worktree.c +4 -3
@@ -769,7 +769,7 @@ static int add(int ac, const char **av, const char *prefix)char *branch_to_free =NULL ;char *new_branch_to_free =NULL ;const char *new_branch =NULL ; -const char *opt_track =NULL ; +char *opt_track =NULL ;const char *lock_reason =NULL ;int keep_locked =0 ;int used_new_branch_options;
@@ -846,7 +846,7 @@ static int add(int ac, const char **av, const char *prefix)if (opts.orphan && !new_branch) {int n;const char *s =worktree_basename (path, &n); - new_branch =xstrndup (s, n); + new_branch = new_branch_to_free =xstrndup (s, n); }else if (opts.orphan) { ;/* no-op */ }else if (opts.detach) {
@@ -875,7 +875,7 @@ static int add(int ac, const char **av, const char *prefix) remote =unique_tracking_name (branch, &oid,NULL );if (remote) { new_branch = branch; - branch = remote; + branch = new_branch_to_free = remote; } }
@@ -923,6 +923,7 @@ static int add(int ac, const char **av, const char *prefix) ret =add_worktree (path, branch, &opts);free (path); +free (opt_track);free (branch_to_free);free (new_branch_to_free);return ret;
t/t2400-worktree-add.sh +1 -0
@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh
t/t9902-completion.sh +1 -0
@@ -16,6 +16,7 @@ test_untraceable=UnfortunatelyYes GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-bash.sh complete ()
Signature · 1 line
2.46.0.rc1.dirty
PATCH 18/23 builtin/credential-cache: fix trivial leaks
In reply to Patrick SteinhardtThere are two trivial leaks in git-credential-cache(1):
- We leak the child process in spawn_daemon(). As we do not call finish_command() and instead let the created process daemonize, we have to clear the process manually.
- We do not free the computed socket path in case it wasn't given via
`--socket=`.
Plug both of these memory leaks.
--- builtin/credential-cache.c | 9 +++++++-- t/t0301-credential-cache.sh | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-)
builtin/credential-cache.c +7 -2
@@ -88,6 +88,8 @@ static void spawn_daemon(const char *socket)die_errno ("unable to read result code from cache daemon" );if (r !=3 ||memcmp (buf,"ok\n" ,3 ))die ("cache daemon did not start: %.*s" , r, buf); + +child_process_clear (&daemon);close (daemon.out); }
@@ -137,7 +139,8 @@ static void announce_capabilities(void)int cmd_credential_cache (int argc,const char **argv,const char *prefix) { -char *socket_path =NULL ; +const char *socket_path_arg =NULL ; +char *socket_path;int timeout =900 ;const char *op;const char *const usage[] = {
@@ -147,7 +150,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_INTEGER (0 ,"timeout" , &timeout,"number of seconds to cache credentials" ), -OPT_STRING (0 ,"socket" , &socket_path,"path" , +OPT_STRING (0 ,"socket" , &socket_path_arg,"path" ,"path of cache-daemon socket" ),OPT_END () };
@@ -160,6 +163,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)if (!have_unix_sockets ())die (_ ("credential-cache unavailable; no unix socket support" )); + socket_path =xstrdup_or_null (socket_path_arg);if (!socket_path) socket_path =get_socket_path ();if (!socket_path)
@@ -176,6 +180,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)else ;/* ignore unknown operation */ +free (socket_path);return 0 ; }
t/t0301-credential-cache.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='credential-cache tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-credential.sh
Signature · 1 line
2.46.0.rc1.dirty
PATCH 19/23 t/test-repository: fix leaking repository
In reply to Patrick SteinhardtThe test-repository test helper zeroes out the_repository such that it can be sure that our codebase only ends up using the supplied repository that we initialize in the respective helper functions. This does cause memory leaks though as the data that the_repository has been holding onto is not referenced anymore.
Fix this by calling repo_clear() instead.
--- t/helper/test-repository.c | 4 ++-- t/t5318-commit-graph.sh | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-)
t/helper/test-repository.c +2 -2
@@ -19,7 +19,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,setup_git_env (gitdir); -memset (the_repository,0 ,sizeof (*the_repository)); +repo_clear (the_repository);if (repo_init (&r, gitdir, worktree))die ("Couldn't init repo" );
@@ -49,7 +49,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,setup_git_env (gitdir); -memset (the_repository,0 ,sizeof (*the_repository)); +repo_clear (the_repository);if (repo_init (&r, gitdir, worktree))die ("Couldn't init repo" );
t/t5318-commit-graph.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='commit graph' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-chunk.sh
Signature · 1 line
2.46.0.rc1.dirty
PATCH 20/23 object-name: fix leaking commit list items
In reply to Patrick SteinhardtWhen calling get_oid_oneline(), we pass in a struct commit_list that gets modified by the function. This creates a weird situation where the commit list may sometimes be empty after returning, but sometimes it will continue to carry additional commits. In those cases the remainder of the list leaks.
Ultimately, the design where we only pass partial ownership to get_oid_oneline() feels shoddy. Refactor the code such that we only pass a constant pointer to the list, creating a local copy as needed. Callers are thus always responsible for freeing the commit list, which then allows us to plug a bunch of memory leaks.
--- object-name.c | 26 ++++++++++++++++---------- t/t1511-rev-parse-caret.sh | 1 + t/t6133-pathspec-rev-dwim.sh | 2 ++ 3 files changed, 19 insertions(+), 10 deletions(-)
object-name.c +16 -10
@@ -27,7 +27,8 @@#include "date.h" #include "object-file-convert.h" -static int get_oid_oneline (struct repository *r,const char *,struct object_id *,struct commit_list *); +static int get_oid_oneline (struct repository *r,const char *,struct object_id *, +const struct commit_list *);typedef int (*disambiguate_hint_fn )(struct repository *,const struct object_id *,void *);
@@ -1254,6 +1255,8 @@ static int peel_onion(struct repository *r, const char *name, int len, prefix =xstrndup (sp +1 , name + len -1 - (sp +1 ));commit_list_insert ((struct commit *)o, &list); ret =get_oid_oneline (r, prefix, oid, list); + +free_commit_list (list);free (prefix);return ret; }
@@ -1388,9 +1391,10 @@ static int handle_one_ref(const char *path, const struct object_id *oid,static int get_oid_oneline (struct repository *r,const char *prefix,struct object_id *oid, -struct commit_list *list) +const struct commit_list *list) { -struct commit_list *backup =NULL , *l; +struct commit_list *copy =NULL ; +const struct commit_list *l;int found =0 ;int negative =0 ;regex_t regex;
@@ -1411,14 +1415,14 @@ static int get_oid_oneline(struct repository *r,for (l = list; l; l = l->next) { l->item->object.flags |=ONELINE_SEEN ; -commit_list_insert (l->item, &backup); +commit_list_insert (l->item, ©); } -while (list) { +while (copy) {const char *p, *buf;struct commit *commit;int matches; - commit =pop_most_recent_commit (&list,ONELINE_SEEN ); + commit =pop_most_recent_commit (©,ONELINE_SEEN );if (!parse_object (r, &commit->object.oid))continue ; buf =repo_get_commit_buffer (r, commit,NULL );
@@ -1433,10 +1437,9 @@ static int get_oid_oneline(struct repository *r, } }regfree (®ex); -free_commit_list (list); -for (l = backup; l; l = l->next) +for (l = list; l; l = l->next)clear_commit_marks (l->item,ONELINE_SEEN ); -free_commit_list (backup); +free_commit_list (copy);return found ?0 :-1 ; }
@@ -2024,7 +2027,10 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,refs_for_each_ref (get_main_ref_store (repo), handle_one_ref, &cb);refs_head_ref (get_main_ref_store (repo), handle_one_ref, &cb);commit_list_sort_by_date (&list); -return get_oid_oneline (repo, name +2 , oid, list); + ret =get_oid_oneline (repo, name +2 , oid, list); + +free_commit_list (list); +return ret; } if (namelen < 3 || name[2] != ':' ||
t/t1511-rev-parse-caret.sh +1 -0
@@ -5,6 +5,7 @@ test_description='tests for ref^{stuff}' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
t/t6133-pathspec-rev-dwim.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='test dwim of revs versus pathspecs in revision parser' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.rc1.dirty
PATCH 21/23 entry: fix leaking pathnames during delayed checkout
In reply to Patrick SteinhardtWhen filtering files during delayed checkout, we pass a string list to async_query_available_blobs(). This list is initialized with NODUP, and thus inserted strings will not be owned by the list. In the latter function we then try to hand over ownership by passing an xstrup()'d value to string_list_insert(). But this is not how this works: a NODUP list does not take ownership of allocated strings and will never free them for the caller.
Fix this issue by initializing the list as DUP instead and dropping the explicit call to xstrdup(). This is okay to do given that this is the single callsite of async_query_available_blobs().
--- convert.c | 2 +- entry.c | 4 +++- t/t2080-parallel-checkout-basics.sh | 1 + t/t2082-parallel-checkout-attributes.sh | 1 + 4 files changed, 6 insertions(+), 2 deletions(-)
convert.c +1 -1
@@ -960,7 +960,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_pwhile ((line =packet_read_line (process->out,NULL ))) {const char *path;if (skip_prefix (line,"pathname=" , &path)) -string_list_insert (available_paths,xstrdup (path)); +string_list_insert (available_paths, path);else ;/* ignore unknown keys */ }
entry.c +3 -1
@@ -191,7 +191,7 @@ int finish_delayed_checkout(struct checkout *state, int show_progress) progress =start_delayed_progress (_ ("Filtering content" ), dco->paths.nr);while (dco->filters.nr >0 ) {for_each_string_list_item (filter, &dco->filters) { -struct string_list available_paths =STRING_LIST_INIT_NODUP ; +struct string_list available_paths =STRING_LIST_INIT_DUP ;if (!async_query_available_blobs (filter->string, &available_paths)) {/* Filter reported an error */
@@ -245,6 +245,8 @@ int finish_delayed_checkout(struct checkout *state, int show_progress) }else errs =1 ; } + +string_list_clear (&available_paths,0 ); }filter_string_list (&dco->filters,0 , string_is_not_null,NULL );
t/t2080-parallel-checkout-basics.sh +1 -0
@@ -8,6 +8,7 @@ working tree. ' TEST_NO_CREATE_REPO=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-parallel-checkout.sh"
t/t2082-parallel-checkout-attributes.sh +1 -0
@@ -10,6 +10,7 @@ properly (without access to the index or attribute stack). ' TEST_NO_CREATE_REPO=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-parallel-checkout.sh" . "$TEST_DIRECTORY/lib-encoding.sh"
Signature · 1 line
2.46.0.rc1.dirty
PATCH 22/23 convert: fix leaking config strings
In reply to Patrick SteinhardtIn read_convert_config(), we end up reading some string values into variables. We don't free any potentially-existing old values though, which will result in a memory leak in case the same key has been defined multiple times.
Fix those leaks.
--- convert.c | 12 +++++++++--- t/t0021-conversion.sh | 1 + 2 files changed, 10 insertions(+), 3 deletions(-)
convert.c +9 -3
@@ -1050,14 +1050,20 @@ static int read_convert_config(const char *var, const char *value,* The command-line will not be interpolated in any way. */ -if (!strcmp ("smudge" , key)) +if (!strcmp ("smudge" , key)) { +FREE_AND_NULL (drv->smudge);return git_config_string (&drv->smudge, var, value); + } -if (!strcmp ("clean" , key)) +if (!strcmp ("clean" , key)) { +FREE_AND_NULL (drv->clean);return git_config_string (&drv->clean, var, value); + } -if (!strcmp ("process" , key)) +if (!strcmp ("process" , key)) { +FREE_AND_NULL (drv->process);return git_config_string (&drv->process, var, value); + }if (!strcmp ("required" , key)) { drv->required =git_config_bool (var, value);
t/t0021-conversion.sh +1 -0
@@ -5,6 +5,7 @@ test_description='blob conversion via gitattributes' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh
Signature · 1 line
2.46.0.rc1.dirty
PATCH 23/23 commit-reach: fix trivial memory leak when computing reachability
In reply to Patrick SteinhardtWe don't free the local stack commit list that we use to compute reachability of multiple commits at once. Do so.
--- commit-reach.c | 1 + t/t3201-branch-contains.sh | 1 + 2 files changed, 2 insertions(+)
commit-reach.c +1 -0
@@ -1227,4 +1227,5 @@ void tips_reachable_from_bases(struct repository *r, done:free (commits);repo_clear_commit_marks (r,SEEN ); +free_commit_list (stack); }
t/t3201-branch-contains.sh +1 -0
@@ -2,6 +2,7 @@ test_description='branch --contains <commit>, --no-contains <commit> --merged, and --no-merged' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup '
Signature · 1 line
2.46.0.rc1.dirty
Re: [PATCH 02/23] builtin/log: fix leaking branch name when creating cover letters
In reply to Patrick SteinhardtQuoted 1 line · Patrick Steinhardt
When calling
make_cover_letter()without a branch name, then we try to
Nit: s/then//
Quoted 3 lines
derive the branch name by calling
find_branch_name(). But while this function returns an allocated string, we never free the result and thus have a memory leak. Fix this.
[snip]
Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`
In reply to Patrick SteinhardtPatrick Steinhardt <[email protected]> writes:
[snip]
Quoted 4 lines
builtin/describe.c +0
-0@@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)strvec_pushv (&args, argv); else strvec_push(&args, "HEAD");
builtin/describe.c int cmd_describe(int argc, const char **argv, const char *prefix) · line 650
Quoted 10 lines
builtin/describe.c +9
-1-return cmd_name_rev (args.nr, args.v, prefix); + +/* +* `cmd_name_rev()` modifies the array, so we'd link its +* contained strings if we didn't do a copy here. +*/ +ALLOC_ARRAY (argv_copy, args.nr +1 ); +for (size_t i =0 ; i < args.nr; i++) + argv_copy[i] = args.v[i]; + argv_copy[args.nr] =NULL ;
Eventhough we pass args.nr, we still set the last element to NULL. This replicates what strvec does and makes it more robust. Nice.
Quoted 10 lines
builtin/describe.c +5
-1+ ret =cmd_name_rev (args.nr, argv_copy, prefix); + +strvec_clear (&args); +free (argv_copy); +return ret; }hashmap_init (&names, commit_name_neq,NULL ,0 ); --2.46.0.rc1.dirty
Re: [PATCH 04/23] builtin/describe: fix leaking array when running diff-index
In reply to Patrick SteinhardtQuoted 2 lines · Patrick Steinhardt
When running git-describe(1) with
--dirty, we will set up astruct rev_infowith arguments for git-diff-index(1). The way we assmble the
s/assmble/assemble
[snip]
Re: [PATCH 12/23] builtin/rev-parse: fix memory leak with `--parseopt`
In reply to Patrick SteinhardtQuoted 5 lines · Patrick Steinhardt
The
--parseoptmode allows shell scripts to have the same option parsing mode as we have in C builtins. It soaks up a set of option descriptions via stdin and massages them into properstruct options that we can then use to parse a set of arguments.
Quoted 7 lines
We only partially free those options when done though, creating a memory leak. Interestingly, we only end up free'ing the first option's help, which is of course wrong.
Fix this by freeing all option's help fields as well as their
arghfielids to plug this memory leak.
s/fielids/fields
[snip]
Quoted 7 lines · Patrick Steinhardt
Hi,
I originally wanted to hold off with sending out this series until v2.46 was out. But I saw that Junio sent out some patches which are plugging the same leaks as I did, so I dedcided to send it out now to avoid some duplicated work.
Quoted 9 lines
There isn't really any structure to this series, I just happened to pick some random test suites that fail with the leak checker enabled and then fixed those. Naturally, I've also got part 4 of this series of patch series in the pipeline already :) As mentioned elsewhere, I hope to get the number of failing test suites to zero this year. Let's see whether this is realistic.
Patrick
This was quite easy to review since there wasn't much to add and it was clearly split into small commits. Apart from some nits, the series looks great to me.
Thanks,
Karthik
Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`
In reply to Patrick SteinhardtQuoted 7 lines · Patrick Steinhardt
When calling
git describe --contains=, we end up invokingcmd_name_rev()with some munged argv array. This array may contain allocated strings and furthermore will likely be modified by the called function. This results in two memory leaks:- First, we leak the array that we use to assemble the arguments.
Quoted 7 lines
- Second, we leak the allocated strings that we may have put into the array.
Fix those leaks by creating a separate copy of the array that we can hand over to
cmd_name_rev(). This allows us to free all strings contained in thestrvec, as the original vector will not be modified anymore.
OK, the separate copy has to be a shallow copy, as its purpose is not to lose pointers to the contained strings.
builtin/describe.c int cmd_describe(int argc, const char **argv, const char *prefix) · line 655
Quoted 5 lines
Furthermore, free both the
strvecand the copied array to fix the first memory leak.
...builtin/describe.c +2
-0+strvec_clear (&args); +free (argv_copy);
So, calling cmd_name_rev() may shuffle the argv_copy[] array but at least it will not free any element in it (as expected---it is typically the (argc, argv[]) the process receives from getting exec'ed) [*]. Freeing the argv_copy shell itself is sufficient to discard what we used to call cmd_name_rev(). And we discard args both its content strings and the array. OK.
builtin/describe.c int cmd_describe(int argc, const char **argv, const char *prefix) · line 659
Quoted 4 lines
builtin/describe.c +1
-0+return ret; }hashmap_init (&names, commit_name_neq,NULL ,0 );
[Footnote]
* The fact that cmd_foo() is called is not a hygiene thing to do in the first place, and in the longer term #leftoverbits we may need to refactor the thing further, into a proper library-ish reusable helper function that can be used to compute name_rev() any number of times, plus cmd_name_rev() and this caller that call it.
Re: [PATCH 06/23] builtin/name-rev: fix various trivial memory leaks
In reply to Patrick SteinhardtQuoted 16 lines · Patrick Steinhardt
There are several structures that we don't release after
cmd_name_rev()is done. Plug those leaks.Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/name-rev.c | 6 ++++-- t/t6007-rev-list-cherry-pick-file.sh | 1 + t/t6120-describe.sh | 1 + 3 files changed, 6 insertions(+), 2 deletions(-)builtin/name-rev.c +0
-0@@ -677,7 +677,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) always, allow_undefined, data.name_only);
builtin/name-rev.c int cmd_name_rev(int argc, const char **argv, const char *prefix) · line 685
Quoted 10 lines
builtin/name-rev.c +4
-2} -UNLEAK (string_pool); -UNLEAK (revs); +string_list_clear (&data.ref_filters,0 ); +string_list_clear (&data.exclude_filters,0 ); +mem_pool_discard (&string_pool,0 ); +object_array_clear (&revs);return 0 ; }
You, originally these "we know we are at the very end of the process, so _exit() will take care of releasing the resources" was a very much deliberate, and in a sense, cmd_describe() calling to this function as the last thing to do was still in the same spirit, but it was not a hygiene thing to do.
In the longer term, we would want to make a major part of the body of this "main" function into a reusable library-ish function, which will be called the desribe and name-rev command implementations, and when that happens, these fixes would move together to that library-ish function.
Looking good.
Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`
In reply to Junio C HamanoQuoted 8 lines · Junio C Hamano
Patrick Steinhardt <[email protected]> writes:
[Footnote]* The fact that
cmd_foo()is called is not a hygiene thing to do in the first place, and in the longer term #leftoverbits we may need to refactor the thing further, into a proper library-ish reusable helper function that can be used to computename_rev()any number of times, pluscmd_name_rev()and this caller that call it.
Agreed. There have been several instances of this scattered across the codebase. The fix is quite ugly in my opinion, but it would be a bigger topic to refactor those cases properly, so I refrained from doing so as part of this series.
Patrick
Quoted 16 lines · Karthik Nayak
Quoted 14 lines · Patrick Steinhardt
Hi,
I originally wanted to hold off with sending out this series until v2.46 was out. But I saw that Junio sent out some patches which are plugging the same leaks as I did, so I dedcided to send it out now to avoid some duplicated work.
There isn't really any structure to this series, I just happened to pick some random test suites that fail with the leak checker enabled and then fixed those. Naturally, I've also got part 4 of this series of patch series in the pipeline already :) As mentioned elsewhere, I hope to get the number of failing test suites to zero this year. Let's see whether this is realistic.
Quoted 6 lines
Quoted 2 lines
Patrick
This was quite easy to review since there wasn't much to add and it was clearly split into small commits. Apart from some nits, the series looks great to me.
Thanks for your review! Given that until now the only thing that needs fixing is smallish typoes I'll refrain from sending the whole thing out just to fix those. I've fixed them locally though and will send them out in case anything else needs fixing.
Patrick
Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`
In reply to Patrick SteinhardtQuoted 10 lines · Patrick Steinhardt
Quoted 8 lines · Junio C Hamano
Patrick Steinhardt <[email protected]> writes:
[Footnote]* The fact that
cmd_foo()is called is not a hygiene thing to do in the first place, and in the longer term #leftoverbits we may need to refactor the thing further, into a proper library-ish reusable helper function that can be used to computename_rev()any number of times, pluscmd_name_rev()and this caller that call it.
Quoted 4 lines
Agreed. There have been several instances of this scattered across the codebase. The fix is quite ugly in my opinion, but it would be a bigger topic to refactor those cases properly, so I refrained from doing so as part of this series.
Oh, I agree that it would be a "after all the dust settles" kind of clean-up.
Thanks.
Re: [PATCH 01/23] builtin/replay: plug leaking `advance_name` variable
In reply to Patrick SteinhardtQuoted 10 lines · Patrick Steinhardt
The
advance_namevariable can either contain a static string when parsed via the--advancecommand line option or it may be an allocated string when set viadetermine_replay_mode(). Because we cannot be sure whether it is allocated or not we just didn't free it at all, resulting in a memory leak.Split up the variables such that we can track the static and allocated strings separately and then free the allocated one to fix the memory leak.
Quoted 5 lines
Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/replay.c | 20 ++++++++++++++------ t/t3650-replay-basics.sh | 1 + 2 files changed, 15 insertions(+), 6 deletions(-)
I had to read this patch a couple of times to make sure that I understood the flow into and out of determine_replay_mode(). But after reading it a couple of times, I agree with the change that you made here.
Thanks,
Taylor
Re: [PATCH 02/23] builtin/log: fix leaking branch name when creating cover letters
In reply to Karthik NayakQuoted 5 lines · Karthik Nayak
Quoted 1 line · Patrick Steinhardt
When calling
make_cover_letter()without a branch name, then we try toNit: s/then//
Good spotting :-).
Thanks,
Taylor
Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`
In reply to Patrick Steinhardtbuiltin/describe.c int cmd_describe(int argc, const char **argv, const char *prefix) · line 644
Quoted 8 lines · Patrick Steinhardt
builtin/describe.c +3
-1@@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)strvec_pushv (&args, argv); else strvec_push(&args, "HEAD"); -return cmd_name_rev (args.nr, args.v, prefix); + +/* +* `cmd_name_rev()` modifies the array, so we'd link its
s/link/leak/ ?
Thanks,
Taylor
Re: [PATCH 08/23] builtin/ls-remote: fix leaking `pattern` strings
In reply to Patrick SteinhardtQuoted 27 lines · Patrick Steinhardt
Users can pass patterns to git-ls-remote(1), which allows them to filter the list of printed references. We assemble those patterns into an array and prefix them with "*/", but never free either the array nor the allocated strings.
Fix those leaks.
Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/ls-remote.c | 11 +++++++---- t/t5535-fetch-push-symref.sh | 1 + 2 files changed, 8 insertions(+), 4 deletions(-)builtin/ls-remote.c +1
-1@@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)int status =0 ;int show_symref_target =0 ;const char *uploadpack =NULL ; -const char **pattern =NULL ; +char **pattern =NULL ;struct transport_ls_refs_options transport_options =TRANSPORT_LS_REFS_OPTIONS_INIT ;int i;
builtin/ls-remote.c int cmd_ls_remote(int argc, const char **argv, const char *prefix) · line 101
Quoted 7 lines
builtin/ls-remote.c +1
-2@@ -96,9 +96,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)if (argc >1 ) {int i;CALLOC_ARRAY (pattern, argc); -for (i =1 ; i < argc; i++) { +for (i =1 ; i < argc; i++) pattern[i -1 ] =xstrfmt ("*/%s" , argv[i]); - }
Instead of xstrfmt()-ing these strings directly, it may be more ergonomic to use a strvec here and call strvec_pushf(). That would save you explicitly CALLOC'ing the array, and would also save you the explicit free() on each element of pattern below.
builtin/ls-remote.c int cmd_ls_remote(int argc, const char **argv, const char *prefix) · line 138
Quoted 9 lines
builtin/ls-remote.c +1
-1}if (flags &REF_TAGS )@@ -136,7 +135,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)struct ref_array_item *item;if (!check_ref_type (ref, flags))continue ; -if (!tail_match (pattern, ref->name)) +if (!tail_match ((const char **) pattern, ref->name))
You could also drop the const cast here as well. The resulting patch on top of this one simplifies things a bit:
--- 8< ---
builtin/ls-remote.c +5 -11
@@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)int status =0 ;int show_symref_target =0 ;const char *uploadpack =NULL ; -char **pattern =NULL ; +struct strvec pattern =STRVEC_INIT ;struct transport_ls_refs_options transport_options =TRANSPORT_LS_REFS_OPTIONS_INIT ;int i;
@@ -93,12 +93,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)packet_trace_identity ("ls-remote" ); -if (argc >1 ) { -int i; -CALLOC_ARRAY (pattern, argc); -for (i =1 ; i < argc; i++) - pattern[i -1 ] =xstrfmt ("*/%s" , argv[i]); - } +for (int i =1 ; i < argc; i++) +strvec_pushf (&pattern,"*/%s" , argv[i]);if (flags &REF_TAGS )strvec_push (&transport_options.ref_prefixes,"refs/tags/" );
@@ -135,7 +131,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)struct ref_array_item *item;if (!check_ref_type (ref, flags))continue ; -if (!tail_match ((const char **) pattern, ref->name)) +if (!tail_match (pattern.v, ref->name))continue ; item =ref_array_push (&ref_array, ref->name, &ref->old_oid); item->symref =xstrdup_or_null (ref->symref);
@@ -158,8 +154,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) status =1 ;transport_ls_refs_options_release (&transport_options); -for (i =1 ; i < argc; i++) -free (pattern[i -1 ]); -free (pattern); +strvec_clear (&pattern);return status; }
--- >8 ---
Thanks,
Taylor
Re: [PATCH 09/23] builtin/remote: fix leaking strings in `branch_list`
In reply to Patrick Steinhardtbuiltin/remote.c static int config_read_branches(const char *key, const char *value, · line 296
Quoted 7 lines · Patrick Steinhardt
builtin/remote.c +1
-1@@ -292,8 +292,8 @@ static int config_read_branches(const char *key, const char *value, type =PUSH_REMOTE ; else return 0; - name =xmemdupz (key, key_len); + name =xmemdupz (key, key_len);
I stared at this for longer than I'd like to admit wondering if there was a whitespace error in the pre-image or something. But I see you just moved initializing 'name' next to the initialization of 'item'.
I'd argue that might be a stray diff, but I don't think it matters much.
builtin/remote.c static int config_read_branches(const char *key, const char *value, · line 342
Quoted 10 lines
builtin/remote.c +1
-0item =string_list_insert (&branch_list, name);if (!item->util)@@ -337,6 +337,7 @@ static int config_read_branches(const char *key, const char *value,BUG ("unexpected type=%d" , type); } +free (name);return 0 ; }
The patch looks good to me otherwise.
Thanks,
Taylor
Re: [PATCH 11/23] builtin/stash: fix various trivial memory leaks
In reply to Patrick SteinhardtQuoted 8 lines · Patrick Steinhardt
builtin/stash.c +3
-1@@ -1892,5 +1895,16 @@ int cmd_stash(int argc, const char **argv, const char *prefix)/* Assume 'stash push' */ strvec_push (&args,"push" );strvec_pushv (&args, argv); -return !!push_stash (args.nr, args.v, prefix,1 ); + +/* +* `push_stash()` ends up modifying the array, which causes memory
builtin/stash.c int cmd_stash(int argc, const char **argv, const char *prefix) · line 1910
Quoted 10 lines
builtin/stash.c +9
-0+* leaks if we didn't copy the array here. +*/ +DUP_ARRAY (args_copy, args.v, args.nr); + + ret = !!push_stash (args.nr, args_copy, prefix,1 ); + +strvec_clear (&args); +free (args_copy); +return ret; }
OK, so this is the same pattern as we saw in the third patch of this series. I agree with Junio's comments in that sub-thread, but also that they are out-of-scope for the present series ;-).
Looking good.
Thanks,
Taylor
Quoted 1 line · Patrick Steinhardt
57 files changed, 251 insertions(+), 73 deletions(-)
I took a careful read through these patches, and found most of them easy to review. I was admittedly a little lost with the "fix various leak" patches, and having slightly more context in the commit descriptions there would have been helpful.
I think most of these patches look all good to me, but there is one where I think using a strvec would be a better fit than allocating our own array.
Otherwise, these are looking in good shape. Thanks for working on this!
Thanks,
Taylor
Re: [PATCH 07/23] builtin/submodule--helper: fix various trivial memory leaks
In reply to Patrick SteinhardtQuoted 4 lines · Patrick Steinhardt
There are multiple trivial memory leaks in the submodule helper. Fix those.
Signed-off-by Patrick Steinhardt <[email protected]>
Hi Patrick,
While working on another series, I fixed some leaks. But, I haven't sent any patches.
The series this message belongs to fixes all the leaks that my unsent series addresses. So, I won't bother sending my patches.
However, I think we can do better for the second leak solved in this patch. I leave my patch at the end of this message in case you want to take a look.
In any case, thanks for working on this. I have the habit of having "SANITIZE=leak" in my config.mak, so this series will make me have fewer distractions :)
Quoted 74 lines
--- builtin/submodule--helper.c | 13 ++++++++++--- t/t7400-submodule-basic.sh | 1 + 2 files changed, 11 insertions(+), 3 deletions(-)builtin/submodule--helper.c +10
-3@@ -2269,6 +2269,7 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf rev =STRBUF_INIT ;char *hex =oid_to_hex (oid); +int reachable; cp.git_cmd =1 ; cp.dir = path;@@ -2278,9 +2279,12 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)prepare_submodule_repo_env (&cp.env);if (capture_command (&cp, &rev,GIT_MAX_HEXSZ +1 ) || rev.len) -return 0 ; + reachable =0 ; +else + reachable =1 ; -return 1 ; +strbuf_release (&rev); +return reachable; } static int fetch_in_submodule(const char *module_path, int depth, int quiet,@@ -3135,6 +3139,7 @@ static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)static int add_submodule (const struct add_data *add_data) {char *submod_gitdir_path; +char *depth_to_free =NULL ;struct module_clone_data clone_data =MODULE_CLONE_DATA_INIT ;struct string_list reference =STRING_LIST_INIT_NODUP ;int ret =-1 ;@@ -3200,7 +3205,7 @@ static int add_submodule(const struct add_data *add_data) } clone_data.dissociate = add_data->dissociate;if (add_data->depth >=0 ) - clone_data.depth =xstrfmt ("%d" , add_data->depth); + clone_data.depth = depth_to_free =xstrfmt ("%d" , add_data->depth);if (clone_submodule (&clone_data, &reference)) goto cleanup;@@ -3223,7 +3228,9 @@ static int add_submodule(const struct add_data *add_data)die (_ ("unable to checkout submodule '%s'" ), add_data->sm_path); } ret =0 ; + cleanup: +free (depth_to_free);string_list_clear (&reference,1 );return ret; }t/t7400-submodule-basic.sh +1
-0@@ -12,6 +12,7 @@ subcommands of git submodule. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup - enable local submodules' 'Signature · 1 line
2.46.0.rc1.dirty
----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] submodule--helper: depth clone parameter
The clone subcommand has an OPT_STRING parameter: --depth, that eventually is used when invoking git-clone.
As git-clone only support positive integer values for --depth since 5594bcad21 (clone,fetch: catch non positive --depth option value, 2013-12-05), seems safe to change the --depth parameter to OPT_INTEGER in module_clone.
Doing so we avoid a leak in add_submodule.
Do it.
--- builtin/submodule--helper.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
builtin/submodule--helper.c +5 -7
@@ -1530,7 +1530,7 @@ struct module_clone_data {const char *path;const char *name;const char *url; -const char *depth; +int depth;struct list_objects_filter_options *filter_options; unsigned int quiet: 1; unsigned int progress: 1;
@@ -1729,8 +1729,8 @@ static int clone_submodule(const struct module_clone_data *clone_data,strvec_push (&cp.args,"--quiet" );if (clone_data->progress)strvec_push (&cp.args,"--progress" ); -if (clone_data->depth && *(clone_data->depth)) -strvec_pushl (&cp.args,"--depth" , clone_data->depth,NULL ); +if (clone_data->depth >0 ) +strvec_pushf (&cp.args,"--depth=%d" , clone_data->depth);if (reference->nr) {struct string_list_item *item;
@@ -1851,8 +1851,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) N_("reference repository")),OPT_BOOL (0 ,"dissociate" , &dissociate,N_ ("use --reference only while cloning" )), -OPT_STRING (0 ,"depth" , &clone_data.depth, -N_ ("string" ), +OPT_INTEGER (0 ,"depth" , &clone_data.depth,N_ ("depth for shallow clones" )),OPT__QUIET (&quiet,"suppress output for cloning a submodule" ), OPT_BOOL(0, "progress", &progress,
@@ -3199,8 +3198,7 @@ static int add_submodule(const struct add_data *add_data)string_list_append (&reference, p)->util = p; } clone_data.dissociate = add_data->dissociate; -if (add_data->depth >=0 ) - clone_data.depth =xstrfmt ("%d" , add_data->depth); + clone_data.depth = add_data->depth;if (clone_submodule (&clone_data, &reference)) goto cleanup;
Signature · 1 line
2.45.1
Re: [PATCH 08/23] builtin/ls-remote: fix leaking `pattern` strings
In reply to Taylor BlauQuoted 32 lines · Taylor Blau
Quoted 31 lines · Patrick Steinhardt
Users can pass patterns to git-ls-remote(1), which allows them to filter the list of printed references. We assemble those patterns into an array and prefix them with "*/", but never free either the array nor the allocated strings.
Fix those leaks.
Signed-off-by Patrick Steinhardt <[email protected]>--- builtin/ls-remote.c | 11 +++++++---- t/t5535-fetch-push-symref.sh | 1 + 2 files changed, 8 insertions(+), 4 deletions(-)builtin/ls-remote.c +1
-2@@ -47,7 +47,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)int status =0 ;int show_symref_target =0 ;const char *uploadpack =NULL ; -const char **pattern =NULL ; +char **pattern =NULL ;struct transport_ls_refs_options transport_options =TRANSPORT_LS_REFS_OPTIONS_INIT ;int i;@@ -96,9 +96,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)if (argc >1 ) {int i;CALLOC_ARRAY (pattern, argc); -for (i =1 ; i < argc; i++) {
Quoted 8 lines
builtin/ls-remote.c int cmd_ls_remote(int argc, const char **argv, const char *prefix) · line 101
Quoted 3 lines
builtin/ls-remote.c +1
-1+for (i =1 ; i < argc; i++) pattern[i -1 ] =xstrfmt ("*/%s" , argv[i]); - }Instead of xstrfmt()-ing these strings directly, it may be more ergonomic to use a strvec here and call
strvec_pushf(). That would save you explicitly CALLOC'ing the array, and would also save you the explicitfree()on each element of pattern below.
Agreed, that reads way better. Thanks!
Patrick
Quoted 7 lines · Taylor Blau
Quoted 1 line · Patrick Steinhardt
57 files changed, 251 insertions(+), 73 deletions(-)I took a careful read through these patches, and found most of them easy to review. I was admittedly a little lost with the "fix various leak" patches, and having slightly more context in the commit descriptions there would have been helpful.
Yeah, I was a bit torn here on whether to expand the commit messages or not. I just didn't think it's all that useful to always say "Variable x is allocated, but never freed" for trivial cases where allocation and freeing need to happen in the same function, much less so if we repeat such commits for every single such variable in the same subsystem. So I just threw those fixes into a single bag.
Patrick
Re: [PATCH 07/23] builtin/submodule--helper: fix various trivial memory leaks
In reply to Rubén JustoQuoted 12 lines · Rubén Justo
Quoted 4 lines · Patrick Steinhardt
There are multiple trivial memory leaks in the submodule helper. Fix those.
Signed-off-by Patrick Steinhardt <[email protected]>Hi Patrick,
While working on another series, I fixed some leaks. But, I haven't sent any patches.
Quoted 6 lines
The series this message belongs to fixes all the leaks that my unsent series addresses. So, I won't bother sending my patches.
However, I think we can do better for the second leak solved in this patch. I leave my patch at the end of this message in case you want to take a look.
Agreed, your patch looks way neater. I'll massage the patch a bit and then add it to the series with an "Original-patch-by" trailer.
Patrick
v2 cover Memory leak fixes (pt.3)
In reply to Patrick SteinhardtHi,
this is the second version of my patch series that plugs another round of memory leaks in Git. Changes compared to v2:
- Various typo fixes.
- Adopted a patch by Rubén that plugs a git-submodule--helper(1) leak in a neater way by using an integer to track the clone depth instead of using a (possibly allocated) string.
- Refactored git-ls-remote(1) to use a struct strvec to track patterns instead of manually populating an array, which makes the code easier to reason about while still plugging the memory leak. This was suggested by Taylor.
Thanks for all the reviews so far!
Patrick
Patrick Steinhardt (23):
builtin/replay: plug leaking advance_name variable builtin/log: fix leaking branch name when creating cover letters builtin/describe: fix memory leak with --contains= builtin/describe: fix leaking array when running diff-index builtin/describe: fix trivial memory leak when describing blob builtin/name-rev: fix various trivial memory leaks builtin/submodule--helper: fix leaking buffer in is_tip_reachable builtin/ls-remote: fix leaking pattern strings builtin/remote: fix leaking strings in branch_list builtin/remote: fix various trivial memory leaks builtin/stash: fix various trivial memory leaks builtin/rev-parse: fix memory leak with --parseopt builtin/show-branch: fix several memory leaks builtin/credential-store: fix leaking credential builtin/rerere: fix various trivial memory leaks builtin/shortlog: fix various trivial memory leaks builtin/worktree: fix leaking derived branch names builtin/credential-cache: fix trivial leaks t/test-repository: fix leaking repository
object-name: fix leaking commit list items
entry: fix leaking pathnames during delayed checkout
convert: fix leaking config strings
commit-reach: fix trivial memory leak when computing reachability
Rubén Justico (1):
builtin/submodule--helper: fix leaking clone depth parameter
builtin/credential-cache.c | 9 ++++- builtin/credential-store.c | 1 + builtin/describe.c | 25 ++++++++++-- builtin/log.c | 4 +- builtin/ls-remote.c | 24 +++++------- builtin/name-rev.c | 6 ++- builtin/remote.c | 44 ++++++++++++++++----- builtin/replay.c | 20 +++++++--- builtin/rerere.c | 8 +++- builtin/rev-parse.c | 5 ++- builtin/shortlog.c | 1 + builtin/show-branch.c | 52 +++++++++++++++++-------- builtin/stash.c | 18 ++++++++- builtin/submodule--helper.c | 20 ++++++---- builtin/worktree.c | 7 ++-- commit-reach.c | 1 + convert.c | 14 +++++-- entry.c | 4 +- object-name.c | 26 ++++++++----- rerere.c | 9 ++++- t/helper/test-repository.c | 4 +- t/t0021-conversion.sh | 1 + t/t0301-credential-cache.sh | 2 + t/t0302-credential-store.sh | 2 + t/t0303-credential-external.sh | 1 + t/t1502-rev-parse-parseopt.sh | 2 + t/t1511-rev-parse-caret.sh | 1 + t/t2030-unresolve-info.sh | 1 + t/t2080-parallel-checkout-basics.sh | 1 + t/t2082-parallel-checkout-attributes.sh | 1 + t/t2400-worktree-add.sh | 1 + t/t2501-cwd-empty.sh | 1 + t/t3201-branch-contains.sh | 1 + t/t3202-show-branch.sh | 1 + t/t3206-range-diff.sh | 1 + t/t3650-replay-basics.sh | 1 + t/t3903-stash.sh | 1 + t/t3904-stash-patch.sh | 2 + t/t3905-stash-include-untracked.sh | 1 + t/t4200-rerere.sh | 1 + t/t4201-shortlog.sh | 1 + t/t5318-commit-graph.sh | 2 + t/t5512-ls-remote.sh | 1 + t/t5514-fetch-multiple.sh | 1 + t/t5520-pull.sh | 1 + t/t5528-push-default.sh | 1 + t/t5535-fetch-push-symref.sh | 1 + t/t5543-atomic-push.sh | 1 + t/t5570-git-daemon.sh | 1 + t/t6007-rev-list-cherry-pick-file.sh | 1 + t/t6010-merge-base.sh | 1 + t/t6120-describe.sh | 1 + t/t6133-pathspec-rev-dwim.sh | 2 + t/t7064-wtstatus-pv2.sh | 1 + t/t7400-submodule-basic.sh | 1 + t/t9902-completion.sh | 1 + t/t9903-bash-prompt.sh | 1 + 57 files changed, 256 insertions(+), 88 deletions(-)
Range-diff against v1:
1: dd044eacc2 = 1: b5c8a9495a builtin/replay: plug leaking `advance_name` variable 2: 4daae88877 ! 2: 73a16fff43 builtin/log: fix leaking branch name when creating cover letters
@@ Metadata
## Commit message ##
builtin/log: fix leaking branch name when creating cover letters
- When calling make_cover_letter() without a branch name, then we try to
+ When calling make_cover_letter() without a branch name, we try to
derive the branch name by calling find_branch_name(). But while this function returns an allocated string, we never free the result and thus have a memory leak. Fix this.
-: ---------- > 3: 65027d13b7 builtin/describe: fix memory leak with `--contains=` 4: e8d86c01cf ! 4: 19ca97e33a builtin/describe: fix leaking array when running diff-index
@@ Commit message
builtin/describe: fix leaking array when running diff-index
When running git-describe(1) with --dirty, we will set up a `struct
- rev_info` with arguments for git-diff-index(1). The way we assmble the
+ rev_info` with arguments for git-diff-index(1). The way we assemble the
arguments it causes two memory leaks though:
- We never release the struct strvec.
5: 8aba7434bd = 5: 6deacb8667 builtin/describe: fix trivial memory leak when describing blob 6: 088f730572 = 6: e38e9d1b62 builtin/name-rev: fix various trivial memory leaks 3: 08a12be13c ! 7: d3eb0372bd builtin/describe: fix memory leak with `--contains=`
@@
## Metadata ##
-Author: Patrick Steinhardt <[email protected]>
+Author: Rubén Justico <[email protected]>
## Commit message ##
- builtin/describe: fix memory leak with --contains=
+ builtin/submodule--helper: fix leaking clone depth parameter
- When calling git describe --contains=, we end up invoking
- cmd_name_rev() with some munged argv array. This array may contain
- allocated strings and furthermore will likely be modified by the called
- function. This results in two memory leaks:
+ The submodule helper supports a --depth parameter for both its "add"
+ and "clone" subcommands, which in both cases end up being forwarded to
+ git-clone(1). But while the former subcommand uses an OPT_INTEGER() to
+ parse the depth, the latter uses OPT_STRING(). Consequently, it is
+ possible to pass non-integer input to "--depth" when calling the "clone"
+ subcommand, where the value will then ultimately cause git-clone(1) to
+ bail out.
- - First, we leak the array that we use to assemble the arguments.
+ Besides the fact that the parameter verification should happen earlier,
+ the submodule helper infrastructure also internally tracks the depth via
+ a string. This requires us to convert the integer in the "add"
+ subcommand into an allocated string, and this string ultimately leaks.
- - Second, we leak the allocated strings that we may have put into the
- array.
-
- Fix those leaks by creating a separate copy of the array that we can
- hand over to cmd_name_rev(). This allows us to free all strings
- contained in the strvec, as the original vector will not be modified
- anymore.
-
- Furthermore, free both the strvec and the copied array to fix the
- first memory leak.
+ Refactor the code to consistently track the clone depth as an integer.
+ This plugs the memory leak, simplifies the code and allows us to use
+ OPT_INTEGER() instead of OPT_STRING(), validating the input before
+ we shell out to git--clone(1).
+ Original-patch-by: Rubén Justo <[email protected]>
- ## builtin/describe.c ##
-@@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *prefix)
- if (contains) {
- struct string_list_item *item;
- struct strvec args;
-+ const char **argv_copy;
-+ int ret;
+ ## builtin/submodule--helper.c ##
+@@ builtin/submodule--helper.c: struct module_clone_data {
+ const char *path;
+ const char *name;
+ const char *url;
+- const char *depth;
++ int depth;
+ struct list_objects_filter_options *filter_options;
+ unsigned int quiet: 1;
+ unsigned int progress: 1;
+@@ builtin/submodule--helper.c: static int clone_submodule(const struct module_clone_data *clone_data,
+ strvec_push(&cp.args, "--quiet");
+ if (clone_data->progress)
+ strvec_push(&cp.args, "--progress");
+- if (clone_data->depth && *(clone_data->depth))
+- strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL);
++ if (clone_data->depth > 0)
++ strvec_pushf(&cp.args, "--depth=%d", clone_data->depth);
+ if (reference->nr) {
+ struct string_list_item *item;
- strvec_init(&args);
- strvec_pushl(&args, "name-rev",
-@@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *prefix)
- strvec_pushv(&args, argv);
- else
- strvec_push(&args, "HEAD");
-- return cmd_name_rev(args.nr, args.v, prefix);
-+
-+ /*
-+ * cmd_name_rev() modifies the array, so we'd link its
-+ * contained strings if we didn't do a copy here.
-+ */
-+ ALLOC_ARRAY(argv_copy, args.nr + 1);
-+ for (size_t i = 0; i < args.nr; i++)
-+ argv_copy[i] = args.v[i];
-+ argv_copy[args.nr] = NULL;
-+
-+ ret = cmd_name_rev(args.nr, argv_copy, prefix);
-+
-+ strvec_clear(&args);
-+ free(argv_copy);
-+ return ret;
- }
+@@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv, const char *prefix)
+ N_("reference repository")),
+ OPT_BOOL(0, "dissociate", &dissociate,
+ N_("use --reference only while cloning")),
+- OPT_STRING(0, "depth", &clone_data.depth,
+- N_("string"),
++ OPT_INTEGER(0, "depth", &clone_data.depth,
+ N_("depth for shallow clones")),
+ OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
+ OPT_BOOL(0, "progress", &progress,
+@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)
+ }
+ clone_data.dissociate = add_data->dissociate;
+ if (add_data->depth >= 0)
+- clone_data.depth = xstrfmt("%d", add_data->depth);
++ clone_data.depth = add_data->depth;
- hashmap_init(&names, commit_name_neq, NULL, 0);
+ if (clone_submodule(&clone_data, &reference))
+ goto cleanup;
7: 5220c91bda ! 8: 49c156cebb builtin/submodule--helper: fix various trivial memory leaks
@@ Metadata
Author: Patrick Steinhardt <[email protected]>
## Commit message ##
- builtin/submodule--helper: fix various trivial memory leaks
+ builtin/submodule--helper: fix leaking buffer in is_tip_reachable
- There are multiple trivial memory leaks in the submodule helper. Fix
- those.
+ The rev buffer in is_tip_reachable() is being populated with the
+ output of git-rev-list(1) -- if either the command fails or the buffer
+ contains any data, then the input commit is not reachable.
+
+ The buffer isn't used for anything else, but neither do we free it,
+ causing a memory leak. Fix this.
@@ builtin/submodule--helper.c: static int is_tip_reachable(const char *path, const
}
static int fetch_in_submodule(const char *module_path, int depth, int quiet,
-@@ builtin/submodule--helper.c: static void append_fetch_remotes(struct strbuf *msg, const char *git_dir_path)
- static int add_submodule(const struct add_data *add_data)
- {
- char *submod_gitdir_path;
-+ char *depth_to_free = NULL;
- struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
- struct string_list reference = STRING_LIST_INIT_NODUP;
- int ret = -1;
-@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)
- }
- clone_data.dissociate = add_data->dissociate;
- if (add_data->depth >= 0)
-- clone_data.depth = xstrfmt("%d", add_data->depth);
-+ clone_data.depth = depth_to_free = xstrfmt("%d", add_data->depth);
-
- if (clone_submodule(&clone_data, &reference))
- goto cleanup;
@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)
die(_("unable to checkout submodule '%s'"), add_data->sm_path);
}
ret = 0;
+
cleanup:
-+ free(depth_to_free);
string_list_clear(&reference, 1);
return ret;
- }
## t/t7400-submodule-basic.sh ##
@@ t/t7400-submodule-basic.sh: subcommands of git submodule.
8: d42152654b ! 9: 4330c80905 builtin/ls-remote: fix leaking pattern strings
@@ Commit message
and prefix them with "*/", but never free either the array nor the
allocated strings.
- Fix those leaks.
+ Refactor the code to use a `struct strvec` instead of manually tracking
+ the strings in an array. Like this, we can easily use `strvec_clear()`
+ to release both the vector and the contained string for us, plugging the
+ leak.
+ Helped-by: Taylor Blau <[email protected]>
Signed-off-by: Patrick Steinhardt <[email protected]>
## builtin/ls-remote.c ##
+@@ builtin/ls-remote.c: static const char * const ls_remote_usage[] = {
+ * Is there one among the list of patterns that match the tail part
+ * of the path?
+ */
+-static int tail_match(const char **pattern, const char *path)
++static int tail_match(const struct strvec *pattern, const char *path)
+ {
+- const char *p;
+ char *pathbuf;
+
+- if (!pattern)
++ if (!pattern->nr)
+ return 1; /* no restriction */
+
+ pathbuf = xstrfmt("/%s", path);
+- while ((p = *(pattern++)) != NULL) {
+- if (!wildmatch(p, pathbuf, 0)) {
++ for (size_t i = 0; i < pattern->nr; i++) {
++ if (!wildmatch(pattern->v[i], pathbuf, 0)) {
+ free(pathbuf);
+ return 1;
+ }
@@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *prefix)
int status = 0;
int show_symref_target = 0;
const char *uploadpack = NULL;
- const char **pattern = NULL;
-+ char **pattern = NULL;
++ struct strvec pattern = STRVEC_INIT;
struct transport_ls_refs_options transport_options =
TRANSPORT_LS_REFS_OPTIONS_INIT;
int i;
@@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *prefix)
- if (argc > 1) {
- int i;
- CALLOC_ARRAY(pattern, argc);
+
+ packet_trace_identity("ls-remote");
+
+- if (argc > 1) {
+- int i;
+- CALLOC_ARRAY(pattern, argc);
- for (i = 1; i < argc; i++) {
-+ for (i = 1; i < argc; i++)
- pattern[i - 1] = xstrfmt("*/%s", argv[i]);
+- pattern[i - 1] = xstrfmt("*/%s", argv[i]);
- }
- }
+- }
++ for (int i = 1; i < argc; i++)
++ strvec_pushf(&pattern, "*/%s", argv[i]);
if (flags & REF_TAGS)
+ strvec_push(&transport_options.ref_prefixes, "refs/tags/");
@@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *prefix)
struct ref_array_item *item;
if (!check_ref_type(ref, flags))
continue;
- if (!tail_match(pattern, ref->name))
-+ if (!tail_match((const char **) pattern, ref->name))
++ if (!tail_match(&pattern, ref->name))
continue;
item = ref_array_push(&ref_array, ref->name, &ref->old_oid);
item->symref = xstrdup_or_null(ref->symref);
@@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *
status = 1;
transport_ls_refs_options_release(&transport_options);
+
-+ for (i = 1; i < argc; i++)
-+ free(pattern[i - 1]);
-+ free(pattern);
++ strvec_clear(&pattern);
return status;
}9: 6952fb2ff2 = 10: 2e3f76b428 builtin/remote: fix leaking strings in `branch_list` 10: 8bedcfdad8 = 11: 384203c34b builtin/remote: fix various trivial memory leaks 11: a4b3ca49c9 = 12: b7c0671797 builtin/stash: fix various trivial memory leaks 12: e277222bd2 ! 13: 034c416d46 builtin/rev-parse: fix memory leak with `--parseopt`
@@ Commit message
which is of course wrong.
Fix this by freeing all option's help fields as well as their argh
- fielids to plug this memory leak.
+ fields to plug this memory leak.
13: fd857efa9f = 14: 20a40e2fd4 builtin/show-branch: fix several memory leaks 14: ba4a883ae1 = 15: b553f1ffb9 builtin/credential-store: fix leaking credential 15: 6d49645c0f = 16: 76d81c8ae1 builtin/rerere: fix various trivial memory leaks 16: 778e87221a = 17: d52ac1a550 builtin/shortlog: fix various trivial memory leaks 17: 8a649bf7ed = 18: dccaf695a9 builtin/worktree: fix leaking derived branch names 18: 2c7a369490 = 19: 788f0bee7b builtin/credential-cache: fix trivial leaks 19: 7564911d2a = 20: eb58826b14 t/test-repository: fix leaking repository 20: e3dbf9116b = 21: fa95a67ccd object-name: fix leaking commit list items 21: e426fd7ec5 = 22: 8cae4c44f4 entry: fix leaking pathnames during delayed checkout 22: 8c2a19994f = 23: 5d2bf0f0f3 convert: fix leaking config strings 23: 60aef16aef = 24: 6db1829c79 commit-reach: fix trivial memory leak when computing reachability
base-commit: d19b6cd2dd72dc811f19df4b32c7ed223256c3ee
Signature · 1 line
2.46.0.dirty
PATCH v2 1/24 builtin/replay: plug leaking `advance_name` variable
The advance_name variable can either contain a static string when parsed via the --advance command line option or it may be an allocated string when set via determine_replay_mode(). Because we cannot be sure whether it is allocated or not we just didn't free it at all, resulting in a memory leak.
Split up the variables such that we can track the static and allocated strings separately and then free the allocated one to fix the memory leak.
--- builtin/replay.c | 20 ++++++++++++++------ t/t3650-replay-basics.sh | 1 + 2 files changed, 15 insertions(+), 6 deletions(-)
builtin/replay.c +14 -6
@@ -151,7 +151,7 @@ static void get_ref_information(struct rev_cmdline_info *cmd_info,static void determine_replay_mode (struct rev_cmdline_info *cmd_info,const char *onto_name, -const char **advance_name, +char **advance_name,struct commit **onto,struct strset **update_refs) {
@@ -174,6 +174,7 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info, *onto =peel_committish (*advance_name);if (repo_dwim_ref (the_repository, *advance_name,strlen (*advance_name), &oid, &fullname,0 ) ==1 ) { +free (*advance_name); *advance_name = fullname; }else {die (_ ("argument to --advance must be a reference" ));
@@ -197,6 +198,7 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,if (negative_refs_complete) {struct hashmap_iter iter;struct strmap_entry *entry; +const char *last_key =NULL ;if (rinfo.negative_refexprs ==0 )die (_ ("all positive revisions given must be references" ));
@@ -208,8 +210,11 @@ static void determine_replay_mode(struct rev_cmdline_info *cmd_info,/* Only one entry, but we have to loop to get it */ strset_for_each_entry (&rinfo.negative_refs, &iter, entry) { - *advance_name = entry->key; + last_key = entry->key; } + +free (*advance_name); + *advance_name =xstrdup_or_null (last_key); }else {/* positive_refs_complete */ if (rinfo.negative_refexprs >1 )die (_ ("cannot implicitly determine correct base for --onto" ));
@@ -271,7 +276,8 @@ static struct commit *pick_regular_commit(struct commit *pickme,int cmd_replay (int argc,const char **argv,const char *prefix) { -const char *advance_name =NULL ; +const char *advance_name_opt =NULL ; +char *advance_name =NULL ;struct commit *onto =NULL ;const char *onto_name =NULL ;int contained =0 ;
@@ -292,7 +298,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix)NULL };struct option replay_options[] = { -OPT_STRING (0 ,"advance" , &advance_name, +OPT_STRING (0 ,"advance" , &advance_name_opt,N_ ("branch" ),N_ ("make replay advance given branch" )), OPT_STRING(0, "onto", &onto_name,
@@ -306,14 +312,15 @@ int cmd_replay(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, replay_options, replay_usage,PARSE_OPT_KEEP_ARGV0 |PARSE_OPT_KEEP_UNKNOWN_OPT ); -if (!onto_name && !advance_name) { +if (!onto_name && !advance_name_opt) {error (_ ("option --onto or --advance is mandatory" ));usage_with_options (replay_usage, replay_options); } -if (advance_name && contained) +if (advance_name_opt && contained)die (_ ("options '%s' and '%s' cannot be used together" ),"--advance" ,"--contained" ); + advance_name =xstrdup_or_null (advance_name_opt);repo_init_revisions (the_repository, &revs, prefix);
@@ -441,6 +448,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix) cleanup:release_revisions (&revs); +free (advance_name);/* Return */ if (ret <0 )
t/t3650-replay-basics.sh +1 -0
@@ -5,6 +5,7 @@ test_description='basic git replay tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GIT_AUTHOR_NAME=author@name
Signature · 1 line
2.46.0.dirty
PATCH v2 2/24 builtin/log: fix leaking branch name when creating cover letters
In reply to Patrick SteinhardtWhen calling make_cover_letter() without a branch name, we try to derive the branch name by calling find_branch_name(). But while this function returns an allocated string, we never free the result and thus have a memory leak. Fix this.
--- builtin/log.c | 4 +++- t/t3206-range-diff.sh | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)
builtin/log.c +3 -1
@@ -1434,6 +1434,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,int need_8bit_cte =0 ;struct pretty_print_context pp = {0 };struct commit *head = list[0 ]; +char *to_free =NULL ;if (!cmit_fmt_is_mail (rev->commit_format))die (_ ("cover letter needs email format" ));
@@ -1455,7 +1456,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, }if (!branch_name) - branch_name =find_branch_name (rev); + branch_name = to_free =find_branch_name (rev); pp.fmt =CMIT_FMT_EMAIL ; pp.date_mode.type =DATE_RFC2822 ;
@@ -1466,6 +1467,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file, encoding, need_8bit_cte, cfg);fprintf (rev->diffopt.file,"%s\n" , sb.buf); +free (to_free);free (pp.after_subject);strbuf_release (&sb);
t/t3206-range-diff.sh +1 -0
@@ -5,6 +5,7 @@ test_description='range-diff tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # Note that because of the range-diff's heuristics, test_commit does more
Signature · 1 line
2.46.0.dirty
PATCH v2 3/24 builtin/describe: fix memory leak with `--contains=`
In reply to Patrick SteinhardtWhen calling git describe --contains=, we end up invoking cmd_name_rev() with some munged argv array. This array may contain allocated strings and furthermore will likely be modified by the called function. This results in two memory leaks:
- First, we leak the array that we use to assemble the arguments.
- Second, we leak the allocated strings that we may have put into the array.
Fix those leaks by creating a separate copy of the array that we can hand over to cmd_name_rev(). This allows us to free all strings contained in the strvec, as the original vector will not be modified anymore.
Furthermore, free both the strvec and the copied array to fix the first memory leak.
--- builtin/describe.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
builtin/describe.c +17 -1
@@ -619,6 +619,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)if (contains) {struct string_list_item *item;struct strvec args; +const char **argv_copy; +int ret;strvec_init (&args); strvec_pushl(&args, "name-rev",
@@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)strvec_pushv (&args, argv); else strvec_push(&args, "HEAD"); -return cmd_name_rev (args.nr, args.v, prefix); + +/* +* `cmd_name_rev()` modifies the array, so we'd leak its +* contained strings if we didn't do a copy here. +*/ +ALLOC_ARRAY (argv_copy, args.nr +1 ); +for (size_t i =0 ; i < args.nr; i++) + argv_copy[i] = args.v[i]; + argv_copy[args.nr] =NULL ; + + ret =cmd_name_rev (args.nr, argv_copy, prefix); + +strvec_clear (&args); +free (argv_copy); +return ret; }hashmap_init (&names, commit_name_neq,NULL ,0 );
Signature · 1 line
2.46.0.dirty
PATCH v2 4/24 builtin/describe: fix leaking array when running diff-index
In reply to Patrick SteinhardtWhen running git-describe(1) with --dirty, we will set up a struct rev_info with arguments for git-diff-index(1). The way we assemble the arguments it causes two memory leaks though:
- We never release the struct strvec.
- setup_revisions() may end up removing some entries from the strvec, which we wouldn't free even if we released the struct.
While we could plug those leaks, this is ultimately unnecessary as the arguments we pass are part of a static array anyway. So instead, refactor the code to drop the struct strvec and just pass this static array directly.
--- builtin/describe.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
builtin/describe.c +3 -3
@@ -695,7 +695,6 @@ int cmd_describe(int argc, const char **argv, const char *prefix) }else if (dirty) {struct lock_file index_lock =LOCK_INIT ;struct rev_info revs; -struct strvec args =STRVEC_INIT ;int fd;setup_work_tree ();
@@ -710,8 +709,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)repo_update_index_if_able (the_repository, &index_lock);repo_init_revisions (the_repository, &revs, prefix); -strvec_pushv (&args, diff_index_args); -if (setup_revisions (args.nr, args.v, &revs,NULL ) !=1 ) + +if (setup_revisions (ARRAY_SIZE (diff_index_args) -1 , + diff_index_args, &revs,NULL ) !=1 )BUG ("malformed internal diff-index command line" );run_diff_index (&revs,0 );
Signature · 1 line
2.46.0.dirty
PATCH v2 5/24 builtin/describe: fix trivial memory leak when describing blob
In reply to Patrick SteinhardtWe never free the struct strvec args variable in describe_blob(), which thus causes a memory leak. Fix this.
--- builtin/describe.c | 1 + t/t9903-bash-prompt.sh | 1 + 2 files changed, 2 insertions(+)
builtin/describe.c +1 -0
@@ -529,6 +529,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)traverse_commit_list (&revs, process_commit, process_object, &pcd);reset_revision_walk ();release_revisions (&revs); +strvec_clear (&args); }static void describe (const char *arg,int last_one)
t/t9903-bash-prompt.sh +1 -0
@@ -8,6 +8,7 @@ test_description='test git-specific bash prompt functions' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-bash.sh . "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"
Signature · 1 line
2.46.0.dirty
PATCH v2 6/24 builtin/name-rev: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are several structures that we don't release after cmd_name_rev() is done. Plug those leaks.
--- builtin/name-rev.c | 6 ++++-- t/t6007-rev-list-cherry-pick-file.sh | 1 + t/t6120-describe.sh | 1 + 3 files changed, 6 insertions(+), 2 deletions(-)
builtin/name-rev.c +4 -2
@@ -677,7 +677,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) always, allow_undefined, data.name_only); } -UNLEAK (string_pool); -UNLEAK (revs); +string_list_clear (&data.ref_filters,0 ); +string_list_clear (&data.exclude_filters,0 ); +mem_pool_discard (&string_pool,0 ); +object_array_clear (&revs);return 0 ; }
t/t6007-rev-list-cherry-pick-file.sh +1 -0
@@ -5,6 +5,7 @@ test_description='test git rev-list --cherry-pick -- file' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # A---B---D---F
t/t6120-describe.sh +1 -0
@@ -14,6 +14,7 @@ test_description='test describe' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_describe () {
Signature · 1 line
2.46.0.dirty
PATCH v2 8/24 builtin/submodule--helper: fix leaking buffer in `is_tip_reachable`
In reply to Patrick SteinhardtThe rev buffer in is_tip_reachable() is being populated with the output of git-rev-list(1) -- if either the command fails or the buffer contains any data, then the input commit is not reachable.
The buffer isn't used for anything else, but neither do we free it, causing a memory leak. Fix this.
--- builtin/submodule--helper.c | 9 +++++++-- t/t7400-submodule-basic.sh | 1 + 2 files changed, 8 insertions(+), 2 deletions(-)
builtin/submodule--helper.c +7 -2
@@ -2268,6 +2268,7 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)struct child_process cp =CHILD_PROCESS_INIT ;struct strbuf rev =STRBUF_INIT ;char *hex =oid_to_hex (oid); +int reachable; cp.git_cmd =1 ; cp.dir = path;
@@ -2277,9 +2278,12 @@ static int is_tip_reachable(const char *path, const struct object_id *oid)prepare_submodule_repo_env (&cp.env);if (capture_command (&cp, &rev,GIT_MAX_HEXSZ +1 ) || rev.len) -return 0 ; + reachable =0 ; +else + reachable =1 ; -return 1 ; +strbuf_release (&rev); +return reachable; } static int fetch_in_submodule(const char *module_path, int depth, int quiet,
@@ -3222,6 +3226,7 @@ static int add_submodule(const struct add_data *add_data)die (_ ("unable to checkout submodule '%s'" ), add_data->sm_path); } ret =0 ; + cleanup:string_list_clear (&reference,1 );return ret;
t/t7400-submodule-basic.sh +1 -0
@@ -12,6 +12,7 @@ subcommands of git submodule. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup - enable local submodules' '
Signature · 1 line
2.46.0.dirty
PATCH v2 9/24 builtin/ls-remote: fix leaking `pattern` strings
In reply to Patrick SteinhardtUsers can pass patterns to git-ls-remote(1), which allows them to filter the list of printed references. We assemble those patterns into an array and prefix them with "*/", but never free either the array nor the allocated strings.
Refactor the code to use a struct strvec instead of manually tracking the strings in an array. Like this, we can easily use strvec_clear() to release both the vector and the contained string for us, plugging the leak.
--- builtin/ls-remote.c | 24 ++++++++++-------------- t/t5535-fetch-push-symref.sh | 1 + 2 files changed, 11 insertions(+), 14 deletions(-)
builtin/ls-remote.c +10 -14
@@ -19,17 +19,16 @@ static const char * const ls_remote_usage[] = {* Is there one among the list of patterns that match the tail part * of the path? */ -static int tail_match (const char **pattern,const char *path) +static int tail_match (const struct strvec *pattern,const char *path) { -const char *p;char *pathbuf; -if (!pattern) +if (!pattern->nr)return 1 ;/* no restriction */ pathbuf =xstrfmt ("/%s" , path); -while ((p = *(pattern++)) !=NULL ) { -if (!wildmatch (p, pathbuf,0 )) { +for (size_t i =0 ; i < pattern->nr; i++) { +if (!wildmatch (pattern->v[i], pathbuf,0 )) {free (pathbuf);return 1 ; }
@@ -47,7 +46,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)int status =0 ;int show_symref_target =0 ;const char *uploadpack =NULL ; -const char **pattern =NULL ; +struct strvec pattern =STRVEC_INIT ;struct transport_ls_refs_options transport_options =TRANSPORT_LS_REFS_OPTIONS_INIT ;int i;
@@ -93,13 +92,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)packet_trace_identity ("ls-remote" ); -if (argc >1 ) { -int i; -CALLOC_ARRAY (pattern, argc); -for (i =1 ; i < argc; i++) { - pattern[i -1 ] =xstrfmt ("*/%s" , argv[i]); - } - } +for (int i =1 ; i < argc; i++) +strvec_pushf (&pattern,"*/%s" , argv[i]);if (flags &REF_TAGS )strvec_push (&transport_options.ref_prefixes,"refs/tags/" );
@@ -136,7 +130,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)struct ref_array_item *item;if (!check_ref_type (ref, flags))continue ; -if (!tail_match (pattern, ref->name)) +if (!tail_match (&pattern, ref->name))continue ; item =ref_array_push (&ref_array, ref->name, &ref->old_oid); item->symref =xstrdup_or_null (ref->symref);
@@ -158,5 +152,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)if (transport_disconnect (transport)) status =1 ;transport_ls_refs_options_release (&transport_options); + +strvec_clear (&pattern);return status; }
t/t5535-fetch-push-symref.sh +1 -0
@@ -2,6 +2,7 @@ test_description='avoiding conflicting update through symref aliasing' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.dirty
PATCH v2 10/24 builtin/remote: fix leaking strings in `branch_list`
In reply to Patrick SteinhardtThe struct string_list branch_list is declared as NODUP, which makes it not copy strings inserted into it. This causes memory leaks though, as this means it also won't be responsible for _freeing_ inserted strings. Thus, every branch we add to this will leak.
Fix this by marking the list as DUP instead and free the local copy we have of the variable.
--- builtin/remote.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
builtin/remote.c +3 -2
@@ -258,7 +258,7 @@ struct branch_info {char *push_remote_name; }; -static struct string_list branch_list =STRING_LIST_INIT_NODUP ; +static struct string_list branch_list =STRING_LIST_INIT_DUP ;static const char *abbrev_ref (const char *name,const char *prefix) {
@@ -292,8 +292,8 @@ static int config_read_branches(const char *key, const char *value, type =PUSH_REMOTE ; else return 0; - name =xmemdupz (key, key_len); + name =xmemdupz (key, key_len); item =string_list_insert (&branch_list, name);if (!item->util)
@@ -337,6 +337,7 @@ static int config_read_branches(const char *key, const char *value,BUG ("unexpected type=%d" , type); } +free (name);return 0 ; }
Signature · 1 line
2.46.0.dirty
PATCH v2 11/24 builtin/remote: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are multiple trivial memory leaks in git-remote(1). Fix those.
--- builtin/remote.c | 39 ++++++++++++++++++++++++++++++++------- t/t5512-ls-remote.sh | 1 + t/t5514-fetch-multiple.sh | 1 + t/t5520-pull.sh | 1 + t/t5528-push-default.sh | 1 + t/t5543-atomic-push.sh | 1 + t/t5570-git-daemon.sh | 1 + 7 files changed, 38 insertions(+), 7 deletions(-)
builtin/remote.c +32 -7
@@ -555,13 +555,16 @@ static int add_branch_for_removal(const char *refname, refspec.dst = (char *)refname;if (remote_find_tracking (branches->remote, &refspec))return 0 ; +free (refspec.src);/* don't delete a branch if another remote also uses it */ for (kr = branches->keep->list; kr; kr = kr->next) {memset (&refspec,0 ,sizeof (refspec)); refspec.dst = (char *)refname; -if (!remote_find_tracking (kr->remote, &refspec)) +if (!remote_find_tracking (kr->remote, &refspec)) { +free (refspec.src);return 0 ; + } }/* don't delete non-remote-tracking refs */
@@ -668,7 +671,11 @@ static int config_read_push_default(const char *key, const char *value,static void handle_push_default (const char * old_name,const char * new_name) {struct push_default_info push_default = { - old_name,CONFIG_SCOPE_UNKNOWN ,STRBUF_INIT ,-1 }; + .old_name = old_name, + .scope =CONFIG_SCOPE_UNKNOWN , + .origin =STRBUF_INIT , + .linenr =-1 , + };git_config (config_read_push_default, &push_default);if (push_default.scope >=CONFIG_SCOPE_COMMAND ) ;/* pass */
@@ -688,6 +695,8 @@ static void handle_push_default(const char* old_name, const char* new_name) push_default.origin.buf, push_default.linenr, old_name); } + +strbuf_release (&push_default.origin); }
@@ -785,7 +794,7 @@ static int mv(int argc, const char **argv, const char *prefix) }if (!refspec_updated) -return 0 ; + goto out;/* * First remove symrefs, then rename the rest, finally create
@@ -851,10 +860,15 @@ static int mv(int argc, const char **argv, const char *prefix)display_progress (progress, ++refs_renamed_nr); }stop_progress (&progress); -string_list_clear (&remote_branches,1 );handle_push_default (rename.old_name, rename.new_name); +out: +string_list_clear (&remote_branches,1 ); +strbuf_release (&old_remote_context); +strbuf_release (&buf); +strbuf_release (&buf2); +strbuf_release (&buf3);return 0 ; }
@@ -945,12 +959,21 @@ static int rm(int argc, const char **argv, const char *prefix)if (!result) {strbuf_addf (&buf,"remote.%s" , remote->name); -if (git_config_rename_section (buf.buf,NULL ) <1 ) -return error (_ ("Could not remove config section '%s'" ), buf.buf); +if (git_config_rename_section (buf.buf,NULL ) <1 ) { + result =error (_ ("Could not remove config section '%s'" ), buf.buf); + goto out; + }handle_push_default (remote->name,NULL ); } +out: +for (struct known_remote *r = known_remotes.list; r;) { +struct known_remote *next = r->next; +free (r); + r = next; + } +strbuf_release (&buf);return result; }
@@ -983,8 +1006,10 @@ static int append_ref_to_tracked_list(const char *refname,memset (&refspec,0 ,sizeof (refspec)); refspec.dst = (char *)refname; -if (!remote_find_tracking (states->remote, &refspec)) +if (!remote_find_tracking (states->remote, &refspec)) {string_list_append (&states->tracked,abbrev_branch (refspec.src)); +free (refspec.src); + }return 0 ; }
t/t5512-ls-remote.sh +1 -0
@@ -5,6 +5,7 @@ test_description='git ls-remote' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh generate_references () {
t/t5514-fetch-multiple.sh +1 -0
@@ -5,6 +5,7 @@ test_description='fetch --all works correctly' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh setup_repository () {
t/t5520-pull.sh +1 -0
@@ -5,6 +5,7 @@ test_description='pulling into void' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh modify () {
t/t5528-push-default.sh +1 -0
@@ -4,6 +4,7 @@ test_description='check various push.default settings' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup bare remotes' '
t/t5543-atomic-push.sh +1 -0
@@ -5,6 +5,7 @@ test_description='pushing to a repository using the atomic push option' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh mk_repo_pair () {
t/t5570-git-daemon.sh +1 -0
@@ -4,6 +4,7 @@ test_description='test fetching over git protocol' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-git-daemon.sh
Signature · 1 line
2.46.0.dirty
PATCH v2 12/24 builtin/stash: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are multiple trivial memory leaks in git-stash(1). Fix those.
--- builtin/stash.c | 18 ++++++++++++++++-- t/t2501-cwd-empty.sh | 1 + t/t3903-stash.sh | 1 + t/t3904-stash-patch.sh | 2 ++ t/t3905-stash-include-untracked.sh | 1 + t/t7064-wtstatus-pv2.sh | 1 + 6 files changed, 22 insertions(+), 2 deletions(-)
builtin/stash.c +16 -2
@@ -1521,6 +1521,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int qstruct strbuf patch =STRBUF_INIT ;struct strbuf stash_msg_buf =STRBUF_INIT ;struct strbuf untracked_files =STRBUF_INIT ; +struct strbuf out =STRBUF_INIT ;if (patch_mode && keep_index ==-1 ) keep_index =1 ;
@@ -1626,7 +1627,6 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int qstruct child_process cp_add =CHILD_PROCESS_INIT ;struct child_process cp_diff =CHILD_PROCESS_INIT ;struct child_process cp_apply =CHILD_PROCESS_INIT ; -struct strbuf out =STRBUF_INIT ; cp_add.git_cmd =1 ;strvec_push (&cp_add.args,"add" );
@@ -1718,6 +1718,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q done:strbuf_release (&patch); +strbuf_release (&out);free_stash_info (&info);strbuf_release (&stash_msg_buf);strbuf_release (&untracked_files);
@@ -1869,6 +1870,8 @@ int cmd_stash(int argc, const char **argv, const char *prefix)OPT_SUBCOMMAND_F ("save" , &fn, save_stash,PARSE_OPT_NOCOMPLETE ), OPT_END() }; +const char **args_copy; +int ret;git_config (git_stash_config,NULL );
@@ -1892,5 +1895,16 @@ int cmd_stash(int argc, const char **argv, const char *prefix)/* Assume 'stash push' */ strvec_push (&args,"push" );strvec_pushv (&args, argv); -return !!push_stash (args.nr, args.v, prefix,1 ); + +/* +* `push_stash()` ends up modifying the array, which causes memory +* leaks if we didn't copy the array here. +*/ +DUP_ARRAY (args_copy, args.v, args.nr); + + ret = !!push_stash (args.nr, args_copy, prefix,1 ); + +strvec_clear (&args); +free (args_copy); +return ret; }
t/t2501-cwd-empty.sh +1 -0
@@ -2,6 +2,7 @@ test_description='Test handling of the current working directory becoming empty' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup '
t/t3903-stash.sh +1 -0
@@ -8,6 +8,7 @@ test_description='Test git stash' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-unique-files.sh
t/t3904-stash-patch.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='stash -p' + +TEST_PASSES_SANITIZE_LEAK=true . ./lib-patch-mode.sh test_expect_success 'setup' '
t/t3905-stash-include-untracked.sh +1 -0
@@ -5,6 +5,7 @@ test_description='Test git stash --include-untracked' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'stash save --include-untracked some dirty working directory' '
t/t7064-wtstatus-pv2.sh +1 -0
@@ -4,6 +4,7 @@ test_description='git status --porcelain=v2 This test exercises porcelain V2 output for git status.' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh
Signature · 1 line
2.46.0.dirty
PATCH v2 13/24 builtin/rev-parse: fix memory leak with `--parseopt`
In reply to Patrick SteinhardtThe --parseopt mode allows shell scripts to have the same option parsing mode as we have in C builtins. It soaks up a set of option descriptions via stdin and massages them into proper struct options that we can then use to parse a set of arguments.
We only partially free those options when done though, creating a memory leak. Interestingly, we only end up free'ing the first option's help, which is of course wrong.
Fix this by freeing all option's help fields as well as their argh fields to plug this memory leak.
--- builtin/rev-parse.c | 5 ++++- t/t1502-rev-parse-parseopt.sh | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-)
builtin/rev-parse.c +4 -1
@@ -553,7 +553,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)strbuf_release (&sb);strvec_clear (&longnames);strvec_clear (&usage); -free ((char *) opts->help); +for (size_t i =0 ; i < opts_nr; i++) { +free ((char *) opts[i].help); +free ((char *) opts[i].argh); + }free (opts);return 0 ; }
t/t1502-rev-parse-parseopt.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='test git rev-parse --parseopt' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_invalid_long_option () {
Signature · 1 line
2.46.0.dirty
PATCH v2 14/24 builtin/show-branch: fix several memory leaks
In reply to Patrick SteinhardtThere are several memory leaks in git-show-branch(1). Fix them.
--- builtin/show-branch.c | 52 +++++++++++++++++++++++++++++------------- t/t3202-show-branch.sh | 1 + t/t6010-merge-base.sh | 1 + 3 files changed, 38 insertions(+), 16 deletions(-)
builtin/show-branch.c +36 -16
@@ -502,14 +502,14 @@ static int rev_is_head(const char *head, const char *name)return !strcmp (head, name); } -static int show_merge_base (struct commit_list *seen,int num_rev) +static int show_merge_base (const struct commit_list *seen,int num_rev) {int all_mask = ((1u << (REV_SHIFT + num_rev)) -1 );int all_revs = all_mask & ~((1u <<REV_SHIFT ) -1 );int exit_status =1 ; -while (seen) { -struct commit *commit =pop_commit (&seen); +for (const struct commit_list *s = seen; s; s = s->next) { +struct commit *commit = s->item;int flags = commit->object.flags & all_mask;if (!(flags &UNINTERESTING ) && ((flags & all_revs) == all_revs)) {
@@ -635,7 +635,7 @@ static int parse_reflog_param(const struct option *opt, const char *arg,int cmd_show_branch (int ac,const char **av,const char *prefix) {struct commit *rev[MAX_REVS ], *commit; -char *reflog_msg[MAX_REVS ]; +char *reflog_msg[MAX_REVS ] = {0 };struct commit_list *list =NULL , *seen =NULL ;unsigned int rev_mask[MAX_REVS ];int num_rev, i, extra =0 ;
@@ -692,6 +692,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) parse_reflog_param), OPT_END() }; +const char **args_copy =NULL ; +int ret;init_commit_name_slab (&name_slab);
@@ -699,8 +701,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)/* If nothing is specified, try the default first */ if (ac ==1 && default_args.nr) { +DUP_ARRAY (args_copy, default_args.v, default_args.nr); ac = default_args.nr; - av = default_args.v; + av = args_copy; } ac = parse_options(ac, av, prefix, builtin_show_branch_options,
@@ -780,7 +783,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) }for (i =0 ; i < reflog; i++) { -char *logmsg; +char *logmsg =NULL ;char *nth_desc;const char *msg;char *end;
@@ -790,6 +793,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)if (read_ref_at (get_main_ref_store (the_repository), ref, flags,0 , base + i, &oid, &logmsg, ×tamp, &tz,NULL )) { +free (logmsg); reflog = i;break ; }
@@ -842,7 +846,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)if (!ref_name_cnt) {fprintf (stderr,"No revs to be shown.\n" ); -exit (0 ); + ret =0 ; + goto out; }for (num_rev =0 ; ref_name[num_rev]; num_rev++) {
@@ -879,11 +884,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)commit_list_sort_by_date (&seen); -if (merge_base) -return show_merge_base (seen, num_rev); +if (merge_base) { + ret =show_merge_base (seen, num_rev); + goto out; + } -if (independent) -return show_independent (rev, num_rev, rev_mask); +if (independent) { + ret =show_independent (rev, num_rev, rev_mask); + goto out; + }/* Show list; --more=-1 means list-only */ if (1 < num_rev || extra <0 ) {
@@ -919,8 +928,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)putchar ('\n' ); } } -if (extra <0 ) -exit (0 ); +if (extra <0 ) { + ret =0 ; + goto out; + }/* Sort topologically */ sort_in_topological_order (&seen, sort_order);
@@ -932,8 +943,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) all_mask = ((1u << (REV_SHIFT + num_rev)) -1 ); all_revs = all_mask & ~((1u <<REV_SHIFT ) -1 ); -while (seen) { -struct commit *commit =pop_commit (&seen); +for (struct commit_list *l = seen; l; l = l->next) { +struct commit *commit = l->item;int this_flag = commit->object.flags;int is_merge_point = ((this_flag & all_revs) == all_revs);
@@ -973,6 +984,15 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)if (shown_merge_point && --extra <0 )break ; } + + ret =0 ; + +out: +for (size_t i =0 ; i <ARRAY_SIZE (reflog_msg); i++) +free (reflog_msg[i]); +free_commit_list (seen); +free_commit_list (list); +free (args_copy);free (head); -return 0 ; +return ret; }
t/t3202-show-branch.sh +1 -0
@@ -2,6 +2,7 @@ test_description='test show-branch' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'error descriptions on empty repository' '
t/t6010-merge-base.sh +1 -0
@@ -6,6 +6,7 @@ test_description='Merge base and parent list computation. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh M=1130000000
Signature · 1 line
2.46.0.dirty
PATCH v2 15/24 builtin/credential-store: fix leaking credential
In reply to Patrick SteinhardtWe never free credentials read by the credential store, leading to a memory leak. Plug it.
--- builtin/credential-store.c | 1 + t/t0302-credential-store.sh | 2 ++ t/t0303-credential-external.sh | 1 + 3 files changed, 4 insertions(+)
builtin/credential-store.c +1 -0
@@ -218,5 +218,6 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix) ;/* Ignore unknown operation. */ string_list_clear (&fns,0 ); +credential_clear (&c);return 0 ; }
t/t0302-credential-store.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='credential-store tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-credential.sh
t/t0303-credential-external.sh +1 -0
@@ -29,6 +29,7 @@ you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell commands. ' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-credential.sh
Signature · 1 line
2.46.0.dirty
PATCH v2 16/24 builtin/rerere: fix various trivial memory leaks
In reply to Patrick SteinhardtThere are multiple trivial memory leaks in git-rerere(1). Fix those.
--- builtin/rerere.c | 8 +++++++- rerere.c | 9 +++++++-- t/t2030-unresolve-info.sh | 1 + t/t4200-rerere.sh | 1 + 4 files changed, 16 insertions(+), 3 deletions(-)
builtin/rerere.c +7 -1
@@ -73,11 +73,17 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)if (!strcmp (argv[0 ],"forget" )) {struct pathspec pathspec; +int ret; +if (argc <2 )warning (_ ("'git rerere forget' without paths is deprecated" ));parse_pathspec (&pathspec,0 ,PATHSPEC_PREFER_CWD , prefix, argv +1 ); -return rerere_forget (the_repository, &pathspec); + + ret =rerere_forget (the_repository, &pathspec); + +clear_pathspec (&pathspec); +return ret; }if (!strcmp (argv[0 ],"clear" )) {
rerere.c +7 -2
@@ -1107,7 +1107,7 @@ static int rerere_forget_one_path(struct index_state *istate,int rerere_forget (struct repository *r,struct pathspec *pathspec) { -int i, fd; +int i, fd, ret;struct string_list conflict =STRING_LIST_INIT_DUP ;struct string_list merge_rr =STRING_LIST_INIT_DUP ;
@@ -1132,7 +1132,12 @@ int rerere_forget(struct repository *r, struct pathspec *pathspec)continue ;rerere_forget_one_path (r->index, it->string, &merge_rr); } -return write_rr (&merge_rr, fd); + + ret =write_rr (&merge_rr, fd); + +string_list_clear (&conflict,0 ); +string_list_clear (&merge_rr,1 ); +return ret; }/*
t/t2030-unresolve-info.sh +1 -0
@@ -5,6 +5,7 @@ test_description='undoing resolution' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh check_resolve_undo () {
t/t4200-rerere.sh +1 -0
@@ -25,6 +25,7 @@ test_description='git rerere GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.dirty
PATCH v2 17/24 builtin/shortlog: fix various trivial memory leaks
In reply to Patrick SteinhardtThere is a trivial memory leak in git-shortlog(1). Fix it.
--- builtin/shortlog.c | 1 + t/t4201-shortlog.sh | 1 + 2 files changed, 2 insertions(+)
builtin/shortlog.c +1 -0
@@ -514,4 +514,5 @@ void shortlog_output(struct shortlog *log)string_list_clear (&log->list,1 );clear_mailmap (&log->mailmap);string_list_clear (&log->format,0 ); +string_list_clear (&log->trailers,0 ); }
t/t4201-shortlog.sh +1 -0
@@ -9,6 +9,7 @@ test_description='git shortlog GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.dirty
PATCH v2 18/24 builtin/worktree: fix leaking derived branch names
In reply to Patrick SteinhardtThere are several heuristics that git-worktree(1) uses to derive the name of the newly created branch when not given explicitly. These heuristics all allocate a new string, but we only end up freeing that string in a subset of cases.
Fix the remaining cases where we didn't yet free the derived branch names. While at it, also free opt_track, which is being populated via an OPT_PASSTHRU().
--- builtin/worktree.c | 7 ++++--- t/t2400-worktree-add.sh | 1 + t/t9902-completion.sh | 1 + 3 files changed, 6 insertions(+), 3 deletions(-)
builtin/worktree.c +4 -3
@@ -769,7 +769,7 @@ static int add(int ac, const char **av, const char *prefix)char *branch_to_free =NULL ;char *new_branch_to_free =NULL ;const char *new_branch =NULL ; -const char *opt_track =NULL ; +char *opt_track =NULL ;const char *lock_reason =NULL ;int keep_locked =0 ;int used_new_branch_options;
@@ -846,7 +846,7 @@ static int add(int ac, const char **av, const char *prefix)if (opts.orphan && !new_branch) {int n;const char *s =worktree_basename (path, &n); - new_branch =xstrndup (s, n); + new_branch = new_branch_to_free =xstrndup (s, n); }else if (opts.orphan) { ;/* no-op */ }else if (opts.detach) {
@@ -875,7 +875,7 @@ static int add(int ac, const char **av, const char *prefix) remote =unique_tracking_name (branch, &oid,NULL );if (remote) { new_branch = branch; - branch = remote; + branch = new_branch_to_free = remote; } }
@@ -923,6 +923,7 @@ static int add(int ac, const char **av, const char *prefix) ret =add_worktree (path, branch, &opts);free (path); +free (opt_track);free (branch_to_free);free (new_branch_to_free);return ret;
t/t2400-worktree-add.sh +1 -0
@@ -6,6 +6,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_CREATE_REPO_NO_TEMPLATE=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-rebase.sh
t/t9902-completion.sh +1 -0
@@ -16,6 +16,7 @@ test_untraceable=UnfortunatelyYes GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./lib-bash.sh complete ()
Signature · 1 line
2.46.0.dirty
PATCH v2 19/24 builtin/credential-cache: fix trivial leaks
In reply to Patrick SteinhardtThere are two trivial leaks in git-credential-cache(1):
- We leak the child process in spawn_daemon(). As we do not call finish_command() and instead let the created process daemonize, we have to clear the process manually.
- We do not free the computed socket path in case it wasn't given via
`--socket=`.
Plug both of these memory leaks.
--- builtin/credential-cache.c | 9 +++++++-- t/t0301-credential-cache.sh | 2 ++ 2 files changed, 9 insertions(+), 2 deletions(-)
builtin/credential-cache.c +7 -2
@@ -88,6 +88,8 @@ static void spawn_daemon(const char *socket)die_errno ("unable to read result code from cache daemon" );if (r !=3 ||memcmp (buf,"ok\n" ,3 ))die ("cache daemon did not start: %.*s" , r, buf); + +child_process_clear (&daemon);close (daemon.out); }
@@ -137,7 +139,8 @@ static void announce_capabilities(void)int cmd_credential_cache (int argc,const char **argv,const char *prefix) { -char *socket_path =NULL ; +const char *socket_path_arg =NULL ; +char *socket_path;int timeout =900 ;const char *op;const char *const usage[] = {
@@ -147,7 +150,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_INTEGER (0 ,"timeout" , &timeout,"number of seconds to cache credentials" ), -OPT_STRING (0 ,"socket" , &socket_path,"path" , +OPT_STRING (0 ,"socket" , &socket_path_arg,"path" ,"path of cache-daemon socket" ),OPT_END () };
@@ -160,6 +163,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)if (!have_unix_sockets ())die (_ ("credential-cache unavailable; no unix socket support" )); + socket_path =xstrdup_or_null (socket_path_arg);if (!socket_path) socket_path =get_socket_path ();if (!socket_path)
@@ -176,6 +180,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)else ;/* ignore unknown operation */ +free (socket_path);return 0 ; }
t/t0301-credential-cache.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='credential-cache tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-credential.sh
Signature · 1 line
2.46.0.dirty
PATCH v2 20/24 t/test-repository: fix leaking repository
In reply to Patrick SteinhardtThe test-repository test helper zeroes out the_repository such that it can be sure that our codebase only ends up using the supplied repository that we initialize in the respective helper functions. This does cause memory leaks though as the data that the_repository has been holding onto is not referenced anymore.
Fix this by calling repo_clear() instead.
--- t/helper/test-repository.c | 4 ++-- t/t5318-commit-graph.sh | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-)
t/helper/test-repository.c +2 -2
@@ -19,7 +19,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,setup_git_env (gitdir); -memset (the_repository,0 ,sizeof (*the_repository)); +repo_clear (the_repository);if (repo_init (&r, gitdir, worktree))die ("Couldn't init repo" );
@@ -49,7 +49,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,setup_git_env (gitdir); -memset (the_repository,0 ,sizeof (*the_repository)); +repo_clear (the_repository);if (repo_init (&r, gitdir, worktree))die ("Couldn't init repo" );
t/t5318-commit-graph.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='commit graph' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-chunk.sh
Signature · 1 line
2.46.0.dirty
PATCH v2 21/24 object-name: fix leaking commit list items
In reply to Patrick SteinhardtWhen calling get_oid_oneline(), we pass in a struct commit_list that gets modified by the function. This creates a weird situation where the commit list may sometimes be empty after returning, but sometimes it will continue to carry additional commits. In those cases the remainder of the list leaks.
Ultimately, the design where we only pass partial ownership to get_oid_oneline() feels shoddy. Refactor the code such that we only pass a constant pointer to the list, creating a local copy as needed. Callers are thus always responsible for freeing the commit list, which then allows us to plug a bunch of memory leaks.
--- object-name.c | 26 ++++++++++++++++---------- t/t1511-rev-parse-caret.sh | 1 + t/t6133-pathspec-rev-dwim.sh | 2 ++ 3 files changed, 19 insertions(+), 10 deletions(-)
object-name.c +16 -10
@@ -27,7 +27,8 @@#include "date.h" #include "object-file-convert.h" -static int get_oid_oneline (struct repository *r,const char *,struct object_id *,struct commit_list *); +static int get_oid_oneline (struct repository *r,const char *,struct object_id *, +const struct commit_list *);typedef int (*disambiguate_hint_fn )(struct repository *,const struct object_id *,void *);
@@ -1254,6 +1255,8 @@ static int peel_onion(struct repository *r, const char *name, int len, prefix =xstrndup (sp +1 , name + len -1 - (sp +1 ));commit_list_insert ((struct commit *)o, &list); ret =get_oid_oneline (r, prefix, oid, list); + +free_commit_list (list);free (prefix);return ret; }
@@ -1388,9 +1391,10 @@ static int handle_one_ref(const char *path, const struct object_id *oid,static int get_oid_oneline (struct repository *r,const char *prefix,struct object_id *oid, -struct commit_list *list) +const struct commit_list *list) { -struct commit_list *backup =NULL , *l; +struct commit_list *copy =NULL ; +const struct commit_list *l;int found =0 ;int negative =0 ;regex_t regex;
@@ -1411,14 +1415,14 @@ static int get_oid_oneline(struct repository *r,for (l = list; l; l = l->next) { l->item->object.flags |=ONELINE_SEEN ; -commit_list_insert (l->item, &backup); +commit_list_insert (l->item, ©); } -while (list) { +while (copy) {const char *p, *buf;struct commit *commit;int matches; - commit =pop_most_recent_commit (&list,ONELINE_SEEN ); + commit =pop_most_recent_commit (©,ONELINE_SEEN );if (!parse_object (r, &commit->object.oid))continue ; buf =repo_get_commit_buffer (r, commit,NULL );
@@ -1433,10 +1437,9 @@ static int get_oid_oneline(struct repository *r, } }regfree (®ex); -free_commit_list (list); -for (l = backup; l; l = l->next) +for (l = list; l; l = l->next)clear_commit_marks (l->item,ONELINE_SEEN ); -free_commit_list (backup); +free_commit_list (copy);return found ?0 :-1 ; }
@@ -2024,7 +2027,10 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,refs_for_each_ref (get_main_ref_store (repo), handle_one_ref, &cb);refs_head_ref (get_main_ref_store (repo), handle_one_ref, &cb);commit_list_sort_by_date (&list); -return get_oid_oneline (repo, name +2 , oid, list); + ret =get_oid_oneline (repo, name +2 , oid, list); + +free_commit_list (list); +return ret; } if (namelen < 3 || name[2] != ':' ||
t/t1511-rev-parse-caret.sh +1 -0
@@ -5,6 +5,7 @@ test_description='tests for ref^{stuff}' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
t/t6133-pathspec-rev-dwim.sh +2 -0
@@ -1,6 +1,8 @@ #!/bin/sh test_description='test dwim of revs versus pathspecs in revision parser' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success 'setup' '
Signature · 1 line
2.46.0.dirty
PATCH v2 22/24 entry: fix leaking pathnames during delayed checkout
In reply to Patrick SteinhardtWhen filtering files during delayed checkout, we pass a string list to async_query_available_blobs(). This list is initialized with NODUP, and thus inserted strings will not be owned by the list. In the latter function we then try to hand over ownership by passing an xstrup()'d value to string_list_insert(). But this is not how this works: a NODUP list does not take ownership of allocated strings and will never free them for the caller.
Fix this issue by initializing the list as DUP instead and dropping the explicit call to xstrdup(). This is okay to do given that this is the single callsite of async_query_available_blobs().
--- convert.c | 2 +- entry.c | 4 +++- t/t2080-parallel-checkout-basics.sh | 1 + t/t2082-parallel-checkout-attributes.sh | 1 + 4 files changed, 6 insertions(+), 2 deletions(-)
convert.c +1 -1
@@ -960,7 +960,7 @@ int async_query_available_blobs(const char *cmd, struct string_list *available_pwhile ((line =packet_read_line (process->out,NULL ))) {const char *path;if (skip_prefix (line,"pathname=" , &path)) -string_list_insert (available_paths,xstrdup (path)); +string_list_insert (available_paths, path);else ;/* ignore unknown keys */ }
entry.c +3 -1
@@ -191,7 +191,7 @@ int finish_delayed_checkout(struct checkout *state, int show_progress) progress =start_delayed_progress (_ ("Filtering content" ), dco->paths.nr);while (dco->filters.nr >0 ) {for_each_string_list_item (filter, &dco->filters) { -struct string_list available_paths =STRING_LIST_INIT_NODUP ; +struct string_list available_paths =STRING_LIST_INIT_DUP ;if (!async_query_available_blobs (filter->string, &available_paths)) {/* Filter reported an error */
@@ -245,6 +245,8 @@ int finish_delayed_checkout(struct checkout *state, int show_progress) }else errs =1 ; } + +string_list_clear (&available_paths,0 ); }filter_string_list (&dco->filters,0 , string_is_not_null,NULL );
t/t2080-parallel-checkout-basics.sh +1 -0
@@ -8,6 +8,7 @@ working tree. ' TEST_NO_CREATE_REPO=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-parallel-checkout.sh"
t/t2082-parallel-checkout-attributes.sh +1 -0
@@ -10,6 +10,7 @@ properly (without access to the index or attribute stack). ' TEST_NO_CREATE_REPO=1 +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY/lib-parallel-checkout.sh" . "$TEST_DIRECTORY/lib-encoding.sh"
Signature · 1 line
2.46.0.dirty
PATCH v2 23/24 convert: fix leaking config strings
In reply to Patrick SteinhardtIn read_convert_config(), we end up reading some string values into variables. We don't free any potentially-existing old values though, which will result in a memory leak in case the same key has been defined multiple times.
Fix those leaks.
--- convert.c | 12 +++++++++--- t/t0021-conversion.sh | 1 + 2 files changed, 10 insertions(+), 3 deletions(-)
convert.c +9 -3
@@ -1050,14 +1050,20 @@ static int read_convert_config(const char *var, const char *value,* The command-line will not be interpolated in any way. */ -if (!strcmp ("smudge" , key)) +if (!strcmp ("smudge" , key)) { +FREE_AND_NULL (drv->smudge);return git_config_string (&drv->smudge, var, value); + } -if (!strcmp ("clean" , key)) +if (!strcmp ("clean" , key)) { +FREE_AND_NULL (drv->clean);return git_config_string (&drv->clean, var, value); + } -if (!strcmp ("process" , key)) +if (!strcmp ("process" , key)) { +FREE_AND_NULL (drv->process);return git_config_string (&drv->process, var, value); + }if (!strcmp ("required" , key)) { drv->required =git_config_bool (var, value);
t/t0021-conversion.sh +1 -0
@@ -5,6 +5,7 @@ test_description='blob conversion via gitattributes' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-terminal.sh
Signature · 1 line
2.46.0.dirty
PATCH v2 24/24 commit-reach: fix trivial memory leak when computing reachability
In reply to Patrick SteinhardtWe don't free the local stack commit list that we use to compute reachability of multiple commits at once. Do so.
--- commit-reach.c | 1 + t/t3201-branch-contains.sh | 1 + 2 files changed, 2 insertions(+)
commit-reach.c +1 -0
@@ -1227,4 +1227,5 @@ void tips_reachable_from_bases(struct repository *r, done:free (commits);repo_clear_commit_marks (r,SEEN ); +free_commit_list (stack); }
t/t3201-branch-contains.sh +1 -0
@@ -2,6 +2,7 @@ test_description='branch --contains <commit>, --no-contains <commit> --merged, and --no-merged' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh test_expect_success setup '
Signature · 1 line
2.46.0.dirty
PATCH v2 7/24 builtin/submodule--helper: fix leaking clone depth parameter
In reply to Patrick SteinhardtThe submodule helper supports a --depth parameter for both its "add" and "clone" subcommands, which in both cases end up being forwarded to git-clone(1). But while the former subcommand uses an OPT_INTEGER() to parse the depth, the latter uses OPT_STRING(). Consequently, it is possible to pass non-integer input to "--depth" when calling the "clone" subcommand, where the value will then ultimately cause git-clone(1) to bail out.
Besides the fact that the parameter verification should happen earlier, the submodule helper infrastructure also internally tracks the depth via a string. This requires us to convert the integer in the "add" subcommand into an allocated string, and this string ultimately leaks.
Refactor the code to consistently track the clone depth as an integer. This plugs the memory leak, simplifies the code and allows us to use OPT_INTEGER() instead of OPT_STRING(), validating the input before we shell out to git--clone(1).
--- builtin/submodule--helper.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
builtin/submodule--helper.c +5 -6
@@ -1530,7 +1530,7 @@ struct module_clone_data {const char *path;const char *name;const char *url; -const char *depth; +int depth;struct list_objects_filter_options *filter_options; unsigned int quiet: 1; unsigned int progress: 1;
@@ -1729,8 +1729,8 @@ static int clone_submodule(const struct module_clone_data *clone_data,strvec_push (&cp.args,"--quiet" );if (clone_data->progress)strvec_push (&cp.args,"--progress" ); -if (clone_data->depth && *(clone_data->depth)) -strvec_pushl (&cp.args,"--depth" , clone_data->depth,NULL ); +if (clone_data->depth >0 ) +strvec_pushf (&cp.args,"--depth=%d" , clone_data->depth);if (reference->nr) {struct string_list_item *item;
@@ -1851,8 +1851,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) N_("reference repository")),OPT_BOOL (0 ,"dissociate" , &dissociate,N_ ("use --reference only while cloning" )), -OPT_STRING (0 ,"depth" , &clone_data.depth, -N_ ("string" ), +OPT_INTEGER (0 ,"depth" , &clone_data.depth,N_ ("depth for shallow clones" )),OPT__QUIET (&quiet,"suppress output for cloning a submodule" ), OPT_BOOL(0, "progress", &progress,
@@ -3200,7 +3199,7 @@ static int add_submodule(const struct add_data *add_data) } clone_data.dissociate = add_data->dissociate;if (add_data->depth >=0 ) - clone_data.depth =xstrfmt ("%d" , add_data->depth); + clone_data.depth = add_data->depth;if (clone_submodule (&clone_data, &reference)) goto cleanup;
Signature · 1 line
2.46.0.dirty
Quoted 9 lines · Patrick Steinhardt
Quoted 7 lines · Taylor Blau
Quoted 1 line · Patrick Steinhardt
57 files changed, 251 insertions(+), 73 deletions(-)I took a careful read through these patches, and found most of them easy to review. I was admittedly a little lost with the "fix various leak" patches, and having slightly more context in the commit descriptions there would have been helpful.
Quoted 6 lines
Yeah, I was a bit torn here on whether to expand the commit messages or not. I just didn't think it's all that useful to always say "Variable x is allocated, but never freed" for trivial cases where allocation and freeing need to happen in the same function, much less so if we repeat such commits for every single such variable in the same subsystem. So I just threw those fixes into a single bag.
I think sometimes that level of detail is useful, e.g., for tracking where the leak came from, if it's always existed, etc.
But I agree enumerating each leak in an otherwise straightforward commit is not a good use of author or reviewer time. I think more useful would be enumerating the kinds of leaks that you clean up.
Thanks,
Taylor
Quoted 1 line · Patrick Steinhardt
Range-diff against v1:
All looks reasonable to me. Thanks for incorporating mine and others' suggestions, and again for working on this!
Thanks,
Taylor