# [PATCH 0/7] Fix some sequencer leaks 23 messages from 2012-05-21 to 2012-05-22. Participants: Christian Couder, Jonathan Nieder, Ramkumar Ramachandra. Thread: https://gitlist.dev/t/30579 ## Christian Couder, 2012-05-21 14:56 Subject: [PATCH 0/7] Fix some sequencer leaks Message-ID: <20120521143309.1911.94302.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120521143309.1911.94302.chriscool%40tuxfamily.org ``` Here is another iteration of a series to fix some leaks in the cherry-pick, revert and sequencer code. The previous iteration only had patch 1 and 2. They haven't changed. I just added the last 5 patches. Maybe the last 2 patches are a bit overkill. But with this series, the valgrind output with --leak-check=full is much cleaner. There most important leaks are still there though. One of them is related to ce_entries, like this: ==2015== 522 bytes in 6 blocks are definitely lost in loss record 51 of 54 ==2015== at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==2015== by 0x53113B: xcalloc (wrapper.c:119) ==2015== by 0x52BBE9: create_ce_entry (unpack-trees.c:559) ==2015== by 0x52BDC6: unpack_nondirectories (unpack-trees.c:609) ==2015== by 0x52C45B: unpack_callback (unpack-trees.c:799) ==2015== by 0x529B9B: traverse_trees (tree-walk.c:407) ==2015== by 0x52CE53: unpack_trees (unpack-trees.c:1091) ==2015== by 0x4D1B89: git_merge_trees (merge-recursive.c:241) ==2015== by 0x4D673A: merge_trees (merge-recursive.c:1819) ==2015== by 0x509521: do_recursive_merge (sequencer.c:225) ==2015== by 0x509F5B: do_pick_commit (sequencer.c:457) ==2015== by 0x50B3B4: pick_commits (sequencer.c:878) The other one is related to the index: ==2015== 574 (400 direct, 174 indirect) bytes in 2 blocks are definitely lost in loss record 52 of 54 ==2015== at 0x4C29DB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==2015== by 0x53113B: xcalloc (wrapper.c:119) ==2015== by 0x4EF8CF: read_index_from (read-cache.c:1441) ==2015== by 0x4EF316: read_index (read-cache.c:1282) ==2015== by 0x50940B: do_recursive_merge (sequencer.c:211) ==2015== by 0x509F5B: do_pick_commit (sequencer.c:457) ==2015== by 0x50B3B4: pick_commits (sequencer.c:878) ==2015== by 0x50B7C6: sequencer_pick_revisions (sequencer.c:994) ==2015== by 0x47A6D5: cmd_cherry_pick (revert.c:237) ==2015== by 0x404FEE: run_builtin (git.c:308) ==2015== by 0x405181: handle_internal_command (git.c:468) ==2015== by 0x40529B: run_argv (git.c:514) Christian Couder (7): sequencer: fix leaked todo_list memory sequencer: release a strbuf used in save_head() merge-recursive: free some string lists revert: free opts.revs to do a bit of cleanup revert: free revs->cmdline.rev unpack-trees: record which unpack error messages should be freed Properly free unpack trees error messages builtin/checkout.c | 1 + builtin/merge.c | 6 ++++-- builtin/revert.c | 6 ++++++ merge-recursive.c | 5 ++++- sequencer.c | 16 +++++++++++++--- unpack-trees.c | 50 +++++++++++++++++++++++++++++++++++--------------- unpack-trees.h | 3 +++ 7 files changed, 66 insertions(+), 21 deletions(-) -- 1.7.10.2.555.g6528037 ``` ## Christian Couder, 2012-05-21 14:56 Subject: [PATCH 1/7] sequencer: fix leaked todo_list memory Message-ID: <20120521145610.1911.87598.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120521145610.1911.87598.chriscool%40tuxfamily.org In-Reply-To: <20120521143309.1911.94302.chriscool@tuxfamily.org> ``` Signed-off-by: Christian Couder --- sequencer.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 3c384b9..762c527 100644 --- a/sequencer.c +++ b/sequencer.c @@ -900,6 +900,7 @@ static int continue_single_pick(void) static int sequencer_continue(struct replay_opts *opts) { struct commit_list *todo_list = NULL; + int res; if (!file_exists(git_path(SEQ_TODO_FILE))) return continue_single_pick(); @@ -915,8 +916,11 @@ static int sequencer_continue(struct replay_opts *opts) } if (index_differs_from("HEAD", 0)) return error_dirty_index(opts); - todo_list = todo_list->next; - return pick_commits(todo_list, opts); + res = pick_commits(todo_list->next, opts); + + free_commit_list(todo_list); + + return res; } static int single_pick(struct commit *cmit, struct replay_opts *opts) @@ -929,6 +933,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) { struct commit_list *todo_list = NULL; unsigned char sha1[20]; + int res; if (opts->subcommand == REPLAY_NONE) assert(opts->revs); @@ -985,5 +990,9 @@ int sequencer_pick_revisions(struct replay_opts *opts) } save_head(sha1_to_hex(sha1)); save_opts(opts); - return pick_commits(todo_list, opts); + res = pick_commits(todo_list, opts); + + free_commit_list(todo_list); + + return res; } -- 1.7.10.2.555.g6528037 ``` ## Christian Couder, 2012-05-21 14:56 Subject: [PATCH 2/7] sequencer: release a strbuf used in save_head() Message-ID: <20120521145610.1911.61154.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120521145610.1911.61154.chriscool%40tuxfamily.org In-Reply-To: <20120521143309.1911.94302.chriscool@tuxfamily.org> ``` Signed-off-by: Christian Couder --- sequencer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sequencer.c b/sequencer.c index 762c527..ad1bbea 100644 --- a/sequencer.c +++ b/sequencer.c @@ -741,6 +741,7 @@ static void save_head(const char *head) die_errno(_("Could not write to %s"), head_file); if (commit_lock_file(&head_lock) < 0) die(_("Error wrapping up %s."), head_file); + strbuf_release(&buf); } static int reset_for_rollback(const unsigned char *sha1) -- 1.7.10.2.555.g6528037 ``` ## Christian Couder, 2012-05-21 14:56 Subject: [PATCH 3/7] merge-recursive: free some string lists Message-ID: <20120521145610.1911.82257.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120521145610.1911.82257.chriscool%40tuxfamily.org In-Reply-To: <20120521143309.1911.94302.chriscool@tuxfamily.org> ``` Signed-off-by: Christian Couder --- merge-recursive.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/merge-recursive.c b/merge-recursive.c index 680937c..9c3570a 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1856,7 +1856,9 @@ int merge_trees(struct merge_options *o, string_list_clear(re_merge, 0); string_list_clear(re_head, 0); string_list_clear(entries, 1); - + free(re_merge); + free(re_head); + free(entries); } else clean = 1; -- 1.7.10.2.555.g6528037 ``` ## Christian Couder, 2012-05-21 14:56 Subject: [PATCH 4/7] revert: free opts.revs to do a bit of cleanup Message-ID: <20120521145610.1911.57879.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120521145610.1911.57879.chriscool%40tuxfamily.org In-Reply-To: <20120521143309.1911.94302.chriscool@tuxfamily.org> ``` Signed-off-by: Christian Couder --- builtin/revert.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/revert.c b/builtin/revert.c index 82d1bf8..5f82a84 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); parse_args(argc, argv, &opts); res = sequencer_pick_revisions(&opts); + free(opts.revs); if (res < 0) die(_("revert failed")); return res; @@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); parse_args(argc, argv, &opts); res = sequencer_pick_revisions(&opts); + free(opts.revs); if (res < 0) die(_("cherry-pick failed")); return res; -- 1.7.10.2.555.g6528037 ``` ## Christian Couder, 2012-05-21 14:56 Subject: [PATCH 5/7] revert: free revs->cmdline.rev Message-ID: <20120521145610.1911.46356.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120521145610.1911.46356.chriscool%40tuxfamily.org In-Reply-To: <20120521143309.1911.94302.chriscool@tuxfamily.org> ``` add_rev_cmdline() in revision.c is (re)allocating an array of struct rev_cmdline_entry. This patch releases it. Signed-off-by: Christian Couder --- builtin/revert.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/builtin/revert.c b/builtin/revert.c index 5f82a84..1d32ed5 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); parse_args(argc, argv, &opts); res = sequencer_pick_revisions(&opts); + if (opts.revs) + free(opts.revs->cmdline.rev); free(opts.revs); if (res < 0) die(_("revert failed")); @@ -233,6 +235,8 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); parse_args(argc, argv, &opts); res = sequencer_pick_revisions(&opts); + if (opts.revs) + free(opts.revs->cmdline.rev); free(opts.revs); if (res < 0) die(_("cherry-pick failed")); -- 1.7.10.2.555.g6528037 ``` ## Christian Couder, 2012-05-21 14:56 Subject: [PATCH 6/7] unpack-trees: record which unpack error messages should be freed Message-ID: <20120521145610.1911.60207.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120521145610.1911.60207.chriscool%40tuxfamily.org In-Reply-To: <20120521143309.1911.94302.chriscool@tuxfamily.org> ``` "struct unpack_trees_options" has a "const char *msgs[]" field that is setup with string values in setup_unpack_trees_porcelain(). Unfortunately this field is setup with strings that are sometimes allocated on the stack, sometimes not. The goal of this patch is to add some infrastructure to record which of the string have been allocated on the stack so they can be properly freed when not needed anymore. We use a bitfield in struct unpack_trees_options to record that. We add a function to both setup the error messages and record wether they should be freed. And we add another function to properly free all the messages that should be freed. Signed-off-by: Christian Couder --- unpack-trees.c | 50 +++++++++++++++++++++++++++++++++++--------------- unpack-trees.h | 3 +++ 2 files changed, 38 insertions(+), 15 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index bcee99c..3537fea 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -50,11 +50,29 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = { ? ((o)->msgs[(type)]) \ : (unpack_plumbing_errors[(type)]) ) +static void setup_unpack_error_msg(struct unpack_trees_options *opts, + enum unpack_trees_error_types err_type, + char *message, int to_free) +{ + /* Check that we can record which messages should be freed */ + assert(NB_UNPACK_TREES_ERROR_TYPES <= 8 * sizeof(opts->msgs_to_be_freed)); + + opts->msgs[err_type] = message; + opts->msgs_to_be_freed |= (to_free ? 1u : 0) << err_type; +} + +void free_unpack_error_msgs(struct unpack_trees_options *opts) +{ + int i; + for (i = 0; i < NB_UNPACK_TREES_ERROR_TYPES; i++) + if (opts->msgs_to_be_freed & 1u << i) + free((char *)opts->msgs[i]); +} + void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, const char *cmd) { int i; - const char **msgs = opts->msgs; const char *msg; char *tmp; const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches"; @@ -65,11 +83,11 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, msg = "Your local changes to the following files would be overwritten by %s:\n%%s"; tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2); sprintf(tmp, msg, cmd, cmd2); - msgs[ERROR_WOULD_OVERWRITE] = tmp; - msgs[ERROR_NOT_UPTODATE_FILE] = tmp; + setup_unpack_error_msg(opts, ERROR_WOULD_OVERWRITE, tmp, 1); + setup_unpack_error_msg(opts, ERROR_NOT_UPTODATE_FILE, tmp, 0); - msgs[ERROR_NOT_UPTODATE_DIR] = - "Updating the following directories would lose untracked files in it:\n%s"; + tmp = "Updating the following directories would lose untracked files in it:\n%s"; + setup_unpack_error_msg(opts, ERROR_NOT_UPTODATE_DIR, tmp, 0); if (advice_commit_before_merge) msg = "The following untracked working tree files would be %s by %s:\n%%s" @@ -78,23 +96,25 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, msg = "The following untracked working tree files would be %s by %s:\n%%s"; tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") + strlen(cmd2) - 4); sprintf(tmp, msg, "removed", cmd, cmd2); - msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp; + setup_unpack_error_msg(opts, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, tmp, 1); tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") + strlen(cmd2) - 4); sprintf(tmp, msg, "overwritten", cmd, cmd2); - msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp; + setup_unpack_error_msg(opts, ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, tmp, 1); /* * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we * cannot easily display it as a list. */ - msgs[ERROR_BIND_OVERLAP] = "Entry '%s' overlaps with '%s'. Cannot bind."; - - msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] = - "Cannot update sparse checkout: the following entries are not up-to-date:\n%s"; - msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] = - "The following Working tree files would be overwritten by sparse checkout update:\n%s"; - msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] = - "The following Working tree files would be removed by sparse checkout update:\n%s"; + tmp = "Entry '%s' overlaps with '%s'. Cannot bind."; + setup_unpack_error_msg(opts, ERROR_BIND_OVERLAP, tmp, 0); + + tmp = "Cannot update sparse checkout: the following entries are not up-to-date:\n%s"; + setup_unpack_error_msg(opts, ERROR_SPARSE_NOT_UPTODATE_FILE, tmp, 0); + + tmp = "The following Working tree files would be overwritten by sparse checkout update:\n%s"; + setup_unpack_error_msg(opts, ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN, tmp, 0); + tmp = "The following Working tree files would be removed by sparse checkout update:\n%s"; + setup_unpack_error_msg(opts, ERROR_WOULD_LOSE_ORPHANED_REMOVED, tmp, 0); opts->show_all_errors = 1; /* rejected paths may not have a static buffer */ diff --git a/unpack-trees.h b/unpack-trees.h index 5e432f5..9b9d6ab 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -30,6 +30,7 @@ enum unpack_trees_error_types { */ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, const char *cmd); +void free_unpack_error_msgs(struct unpack_trees_options *opts); struct unpack_trees_options { unsigned int reset, @@ -55,6 +56,8 @@ struct unpack_trees_options { struct pathspec *pathspec; merge_fn_t fn; const char *msgs[NB_UNPACK_TREES_ERROR_TYPES]; + /* Bit array to know which msgs must be freed */ + unsigned int msgs_to_be_freed; /* * Store error messages in an array, each case * corresponding to a error message type -- 1.7.10.2.555.g6528037 ``` ## Christian Couder, 2012-05-21 14:56 Subject: [PATCH 7/7] Properly free unpack trees error messages Message-ID: <20120521145610.1911.31963.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120521145610.1911.31963.chriscool%40tuxfamily.org In-Reply-To: <20120521143309.1911.94302.chriscool@tuxfamily.org> ``` Signed-off-by: Christian Couder --- builtin/checkout.c | 1 + builtin/merge.c | 6 ++++-- merge-recursive.c | 1 + 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 3ddda34..df8fef0 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -435,6 +435,7 @@ static int merge_working_tree(struct checkout_opts *opts, init_tree_desc(&trees[1], tree->buffer, tree->size); ret = unpack_trees(2, trees, &topts); + free_unpack_error_msgs(&topts); if (ret == -1) { /* * Unpack couldn't do a trivial merge; either diff --git a/builtin/merge.c b/builtin/merge.c index 470fc57..b96a9ae 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -768,7 +768,7 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote struct tree *trees[MAX_UNPACK_TREES]; struct unpack_trees_options opts; struct tree_desc t[MAX_UNPACK_TREES]; - int i, fd, nr_trees = 0; + int i, fd, res, nr_trees = 0; struct dir_struct dir; struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file)); @@ -805,7 +805,9 @@ int checkout_fast_forward(const unsigned char *head, const unsigned char *remote parse_tree(trees[i]); init_tree_desc(t+i, trees[i]->buffer, trees[i]->size); } - if (unpack_trees(nr_trees, t, &opts)) + res = unpack_trees(nr_trees, t, &opts); + free_unpack_error_msgs(&opts); + if (res) return -1; if (write_cache(fd, active_cache, active_nr) || commit_locked_index(lock_file)) diff --git a/merge-recursive.c b/merge-recursive.c index 9c3570a..e196c45 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -239,6 +239,7 @@ static int git_merge_trees(int index_only, init_tree_desc_from_tree(t+2, merge); rc = unpack_trees(3, t, &opts); + free_unpack_error_msgs(&opts); cache_tree_free(&active_cache_tree); return rc; } -- 1.7.10.2.555.g6528037 ``` ## Jonathan Nieder, 2012-05-21 20:39 Subject: Re: [PATCH 5/7] revert: free revs->cmdline.rev Message-ID: <20120521203941.GB28331@burratino> URL: https://gitlist.dev/e/20120521203941.GB28331%40burratino In-Reply-To: <20120521145610.1911.46356.chriscool@tuxfamily.org> ``` Hi, Christian Couder wrote: > add_rev_cmdline() in revision.c is (re)allocating an array of > struct rev_cmdline_entry. This patch releases it. [...] > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix) > git_config(git_default_config, NULL); > parse_args(argc, argv, &opts); > res = sequencer_pick_revisions(&opts); > + if (opts.revs) > + free(opts.revs->cmdline.rev); > free(opts.revs); > if (res < 0) > die(_("revert failed")); Quick thoughts: This feels like a layering violation. Avoidable? Maybe revision.c could gain a helper to allow this to be written like so: free_rev_info(opts.revs); free(opts.revs); Since this is a one-time allocation it is probably worth mentioning in the log message that this is a futureproofing/valgrind-cleanliness measure and is not actually fixing a leak. Micronit: it would feel slightly more comfortable if the free() were after the die(), even though the die() should probably be changed to exit(). That way someone wanting to add code after the die() that continues to assume opts.revs is valid would be able to. Of course I'd imagine the largest leak in cherry-pick is the deliberately constantly growing object hash table. It would be very interesting to fix that --- do you know how libgit2 handles it? Thanks much and hope that helps, Jonathan ``` ## Jonathan Nieder, 2012-05-21 20:43 Subject: Re: [PATCH 6/7] unpack-trees: record which unpack error messages should be freed Message-ID: <20120521204350.GC28331@burratino> URL: https://gitlist.dev/e/20120521204350.GC28331%40burratino In-Reply-To: <20120521145610.1911.60207.chriscool@tuxfamily.org> ``` Christian Couder wrote: > "struct unpack_trees_options" has a "const char *msgs[]" field > that is setup with string values in setup_unpack_trees_porcelain(). Hmm. Incidentally, should callers (e.g., the 100 single-picks involved in a large multi-pick) be reusing these strings instead of allocating them again and again? ``` ## Jonathan Nieder, 2012-05-21 20:57 Subject: Re: [PATCH 1/7] sequencer: fix leaked todo_list memory Message-ID: <20120521205701.GD28331@burratino> URL: https://gitlist.dev/e/20120521205701.GD28331%40burratino In-Reply-To: <20120521145610.1911.87598.chriscool@tuxfamily.org> ``` Christian Couder wrote: > --- a/sequencer.c > +++ b/sequencer.c > @@ -900,6 +900,7 @@ static int continue_single_pick(void) > static int sequencer_continue(struct replay_opts *opts) > { > struct commit_list *todo_list = NULL; > + int res; > > if (!file_exists(git_path(SEQ_TODO_FILE))) > return continue_single_pick(); > @@ -915,8 +916,11 @@ static int sequencer_continue(struct replay_opts *opts) > } > if (index_differs_from("HEAD", 0)) > return error_dirty_index(opts); > - todo_list = todo_list->next; > - return pick_commits(todo_list, opts); > + res = pick_commits(todo_list->next, opts); > + > + free_commit_list(todo_list); > + return res; [...] > @@ -929,6 +933,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) [same change there] Makes sense. This is a one-time allocation, but it is in a somewhat public function and freeing saves human readers and automatic debugging tools like valgrind some distraction. Shouldn't the error paths do this too? walk_revs_populate_todo(&todo_list, opts); if (create_seq_dir()) goto fail; if (get_sha1("HEAD", sha1)) { error(opts->action == REPLAY_REVERT ? _("Can't revert as initial commit") : _("Can't cherry-pick into empty head")); goto fail; } save_head(sha1_to_hex(sha1)); save_opts(opts); result = pick_commits(todo_list, opts); free_commit_list(todo_list); return result; fail: free_commit_list(todo_list); return -1; It does add another O(n) to the running time. Some impatient person watching a long post-multipick pause might want to add an optional SEQUENCER_FORGET_THE_MEMORY_WE_WILL_BE_DYING_SOON_ANYWAY flag argument, but probably that should happen as a separate change. So for what it's worth, I like this change as long as it is done consistently (meaning "in all code paths that return from the function, not just the success case"). Thanks. Jonathan ``` ## Ramkumar Ramachandra, 2012-05-22 04:12 Subject: Re: [PATCH 2/7] sequencer: release a strbuf used in save_head() Message-ID: URL: https://gitlist.dev/e/CALkWK0m9F6EU43v0HbJxWUVtHTiw%2BZvjCjwqbSVdQfomb6f4Aw%40mail.gmail.com In-Reply-To: <20120521145610.1911.61154.chriscool@tuxfamily.org> ``` Hi Christian, Christian Couder wrote: > diff --git a/sequencer.c b/sequencer.c > index 762c527..ad1bbea 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -741,6 +741,7 @@ static void save_head(const char *head) >                die_errno(_("Could not write to %s"), head_file); >        if (commit_lock_file(&head_lock) < 0) >                die(_("Error wrapping up %s."), head_file); > +       strbuf_release(&buf); >  } > >  static int reset_for_rollback(const unsigned char *sha1) Thanks. Shouldn't we be polluting the die() pathways with this strbuf_release(&buf) the way save_todo() does it consistently? if (write_in_full(fd, buf.buf, buf.len) < 0) { strbuf_release(&buf); die_errno(_("Could not write to %s"), todo_file); } if (commit_lock_file(&todo_lock) < 0) { strbuf_release(&buf); die(_("Error wrapping up %s."), todo_file); } strbuf_release(&buf); Ram ``` ## Jonathan Nieder, 2012-05-22 04:23 Subject: Re: [PATCH 2/7] sequencer: release a strbuf used in save_head() Message-ID: <20120522042316.GA3080@burratino> URL: https://gitlist.dev/e/20120522042316.GA3080%40burratino In-Reply-To: ``` Ramkumar Ramachandra wrote: > Christian Couder wrote: >> diff --git a/sequencer.c b/sequencer.c >> index 762c527..ad1bbea 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -741,6 +741,7 @@ static void save_head(const char *head) >>                die_errno(_("Could not write to %s"), head_file); >>        if (commit_lock_file(&head_lock) < 0) >>                die(_("Error wrapping up %s."), head_file); >> +       strbuf_release(&buf); >>  } >> >>  static int reset_for_rollback(const unsigned char *sha1) > > Thanks. Shouldn't we be polluting the die() pathways with this > strbuf_release(&buf) the way save_todo() does it consistently? > > if (write_in_full(fd, buf.buf, buf.len) < 0) { > strbuf_release(&buf); > die_errno(_("Could not write to %s"), todo_file); I don't see why. Doesn't exit take care of freeing everything? And looking at it from the other side, doesn't using exit mean that you cannot be valgrind-clean anyway, since allocations by functions higher in the call chain do not get a chance to be freed? Jonathan ``` ## Ramkumar Ramachandra, 2012-05-22 05:07 Subject: Re: [PATCH 2/7] sequencer: release a strbuf used in save_head() Message-ID: URL: https://gitlist.dev/e/CALkWK0kvjr3NSx6-8svz%3DPKb5ta_UwOUiF4uqh7GriwuJYncUA%40mail.gmail.com In-Reply-To: <20120522042316.GA3080@burratino> ``` Hi Jonathan, Jonathan Nieder wrote: > Ramkumar Ramachandra wrote: >> Christian Couder wrote: >>> [...] >>       if (write_in_full(fd, buf.buf, buf.len) < 0) { >>               strbuf_release(&buf); >>               die_errno(_("Could not write to %s"), todo_file); > > [...] > And > looking at it from the other side, doesn't using exit mean that you > cannot be valgrind-clean anyway, since allocations by functions higher > in the call chain do not get a chance to be freed? Good point; save_todo() sets a bad example. For symmetry, should these two instances of strbuf_release() before die() be removed in a separate patch? Ram ``` ## Ramkumar Ramachandra, 2012-05-22 05:14 Subject: Re: [PATCH 4/7] revert: free opts.revs to do a bit of cleanup Message-ID: URL: https://gitlist.dev/e/CALkWK0%3D1a_LvQ%3DzhdAEt91j1sMsgp6J%3D_CyeNFOkB95dw15pnQ%40mail.gmail.com In-Reply-To: <20120521145610.1911.57879.chriscool@tuxfamily.org> ``` Hi Christian, Christian Couder wrote: > diff --git a/builtin/revert.c b/builtin/revert.c > index 82d1bf8..5f82a84 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix) >        git_config(git_default_config, NULL); >        parse_args(argc, argv, &opts); >        res = sequencer_pick_revisions(&opts); > +       free(opts.revs); >        if (res < 0) >                die(_("revert failed")); >        return res; > @@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) >        git_config(git_default_config, NULL); >        parse_args(argc, argv, &opts); >        res = sequencer_pick_revisions(&opts); > +       free(opts.revs); >        if (res < 0) >                die(_("cherry-pick failed")); >        return res; Technically, memory is allocated to both opts->revs and opts->xopts in parse_args(). Why not free both? Ram ``` ## Jonathan Nieder, 2012-05-22 05:18 Subject: Re: [PATCH 2/7] sequencer: release a strbuf used in save_head() Message-ID: <20120522051827.GA4486@burratino> URL: https://gitlist.dev/e/20120522051827.GA4486%40burratino In-Reply-To: ``` Ramkumar Ramachandra wrote: > Jonathan Nieder wrote: >> And >> looking at it from the other side, doesn't using exit mean that you >> cannot be valgrind-clean anyway, since allocations by functions higher >> in the call chain do not get a chance to be freed? > > Good point; save_todo() sets a bad example. For symmetry, should > these two instances of strbuf_release() before die() be removed in a > separate patch? I can't find myself caring much either way. :) A single free() doesn't hurt performance much, so my hunch would be to leave it alone unless some other practical reason to keep or remove the free()s comes up. ``` ## Jonathan Nieder, 2012-05-22 14:17 Subject: Re: [PATCH 2/7] sequencer: release a strbuf used in save_head() Message-ID: <20120522141735.GA6660@burratino> URL: https://gitlist.dev/e/20120522141735.GA6660%40burratino In-Reply-To: <20120521145610.1911.61154.chriscool@tuxfamily.org> ``` Hi, Christian Couder wrote: > Signed-off-by: Christian Couder > --- > sequencer.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/sequencer.c b/sequencer.c > index 762c527..ad1bbea 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -741,6 +741,7 @@ static void save_head(const char *head) > die_errno(_("Could not write to %s"), head_file); > if (commit_lock_file(&head_lock) < 0) > die(_("Error wrapping up %s."), head_file); > + strbuf_release(&buf); Makes good sense. Actually, I'm not sure why this allocation is needed in the first place. Would something like the following work? -- >8 -- Subject: sequencer: avoid a one-time leak in save_head() This function uses the lockfile API to make its change to the .git/sequencer/head file effectively atomic, so there is no need to gather output intended for that file in a new buffer to write it in one go. Noticed with valgrind. Reported-by: Christian Couder Signed-off-by: Jonathan Nieder --- By the way, the lockfile API usage looks fishy. What kind of races is the locking meant to prevent? What happens if someone tries "git cherry-pick --abort" in another window after my multipick has started and before it finishes? sequencer.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git i/sequencer.c w/sequencer.c index 3c384b94..75b6a995 100644 --- i/sequencer.c +++ w/sequencer.c @@ -732,12 +732,11 @@ static void save_head(const char *head) { const char *head_file = git_path(SEQ_HEAD_FILE); static struct lock_file head_lock; - struct strbuf buf = STRBUF_INIT; int fd; fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR); - strbuf_addf(&buf, "%s\n", head); - if (write_in_full(fd, buf.buf, buf.len) < 0) + if (write_in_full(fd, head, strlen(head)) < 0 || + write_in_full(fd, "\n", 1) < 0) die_errno(_("Could not write to %s"), head_file); if (commit_lock_file(&head_lock) < 0) die(_("Error wrapping up %s."), head_file); ``` ## Christian Couder, 2012-05-22 20:01 Subject: Re: [PATCH 5/7] revert: free revs->cmdline.rev Message-ID: <20120522.220142.31626389365969573.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120522.220142.31626389365969573.chriscool%40tuxfamily.org In-Reply-To: <20120521203941.GB28331@burratino> ``` Hi, From: Jonathan Nieder > Hi, > > Christian Couder wrote: > >> add_rev_cmdline() in revision.c is (re)allocating an array of >> struct rev_cmdline_entry. This patch releases it. > [...] >> --- a/builtin/revert.c >> +++ b/builtin/revert.c >> @@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix) >> git_config(git_default_config, NULL); >> parse_args(argc, argv, &opts); >> res = sequencer_pick_revisions(&opts); >> + if (opts.revs) >> + free(opts.revs->cmdline.rev); >> free(opts.revs); >> if (res < 0) >> die(_("revert failed")); > > Quick thoughts: > > This feels like a layering violation. Avoidable? Maybe revision.c > could gain a helper to allow this to be written like so: > > free_rev_info(opts.revs); > free(opts.revs); > > Since this is a one-time allocation it is probably worth mentioning in > the log message that this is a futureproofing/valgrind-cleanliness > measure and is not actually fixing a leak. Ok, I will create the free_rev_info() helper and improve the commit message. > Micronit: it would feel slightly more comfortable if the free() were > after the die(), even though the die() should probably be changed to > exit(). That way someone wanting to add code after the die() that > continues to assume opts.revs is valid would be able to. > > Of course I'd imagine the largest leak in cherry-pick is the > deliberately constantly growing object hash table. It would be very > interesting to fix that --- do you know how libgit2 handles it? About the constantly growing hash table, perhaps it should be taken care of in a try_to_free_routine() used by xmalloc and other such functions. And no I don't know much about libgit2. > Thanks much and hope that helps, > Jonathan Thanks for your kind review, Christian. ``` ## Christian Couder, 2012-05-22 20:03 Subject: Re: [PATCH 4/7] revert: free opts.revs to do a bit of cleanup Message-ID: <20120522.220310.2276783773227531713.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120522.220310.2276783773227531713.chriscool%40tuxfamily.org In-Reply-To: ``` Hi Ram, From: Ramkumar Ramachandra > Hi Christian, > > Christian Couder wrote: >> diff --git a/builtin/revert.c b/builtin/revert.c >> index 82d1bf8..5f82a84 100644 >> --- a/builtin/revert.c >> +++ b/builtin/revert.c >> @@ -217,6 +217,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix) >>        git_config(git_default_config, NULL); >>        parse_args(argc, argv, &opts); >>        res = sequencer_pick_revisions(&opts); >> +       free(opts.revs); >>        if (res < 0) >>                die(_("revert failed")); >>        return res; >> @@ -232,6 +233,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) >>        git_config(git_default_config, NULL); >>        parse_args(argc, argv, &opts); >>        res = sequencer_pick_revisions(&opts); >> +       free(opts.revs); >>        if (res < 0) >>                die(_("cherry-pick failed")); >>        return res; > > Technically, memory is allocated to both opts->revs and opts->xopts in > parse_args(). Why not free both? I didn't see any leak with opts->xopts but I will have a look. Thanks, Christian. ``` ## Christian Couder, 2012-05-22 20:22 Subject: Re: [PATCH 6/7] unpack-trees: record which unpack error messages should be freed Message-ID: <20120522.222242.2217647953969252595.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120522.222242.2217647953969252595.chriscool%40tuxfamily.org In-Reply-To: <20120521204350.GC28331@burratino> ``` From: Jonathan Nieder > Christian Couder wrote: > >> "struct unpack_trees_options" has a "const char *msgs[]" field >> that is setup with string values in setup_unpack_trees_porcelain(). > > Hmm. Incidentally, should callers (e.g., the 100 single-picks > involved in a large multi-pick) be reusing these strings instead of > allocating them again and again? Some strings filling the msgs field are created with sprintf() using the cmd argument passed to setup_unpack_trees_porcelain(). So they could be different if different cmd arguments are used. In practice now cmd is either "checkout" or "merge", so we could probably reuse the same strings. But if we ever add a checkout command to the sequencer for example, we might want to use different error messages. Another possibility is to xmalloc all the strings and then free them all. Thanks, Christian. ``` ## Christian Couder, 2012-05-22 20:24 Subject: Re: [PATCH 1/7] sequencer: fix leaked todo_list memory Message-ID: <20120522.222431.25966410445815006.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120522.222431.25966410445815006.chriscool%40tuxfamily.org In-Reply-To: <20120521205701.GD28331@burratino> ``` From: Jonathan Nieder > > So for what it's worth, I like this change as long as it is done > consistently (meaning "in all code paths that return from the > function, not just the success case"). Ok, I will do that. Thanks, Christian. ``` ## Christian Couder, 2012-05-22 20:30 Subject: Re: [PATCH 2/7] sequencer: release a strbuf used in save_head() Message-ID: <20120522.223045.876953043846314069.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/20120522.223045.876953043846314069.chriscool%40tuxfamily.org In-Reply-To: <20120522141735.GA6660@burratino> ``` From: Jonathan Nieder > >> + strbuf_release(&buf); > > Makes good sense. Actually, I'm not sure why this allocation is > needed in the first place. Would something like the following work? Ok, I will use your patch in the next iteration. Thanks, Christian. ``` ## Jonathan Nieder, 2012-05-22 20:40 Subject: Re: [PATCH 5/7] revert: free revs->cmdline.rev Message-ID: <20120522204014.GA19789@burratino> URL: https://gitlist.dev/e/20120522204014.GA19789%40burratino In-Reply-To: <20120522.220142.31626389365969573.chriscool@tuxfamily.org> ``` Christian Couder wrote: > About the constantly growing hash table, perhaps it should be taken > care of in a try_to_free_routine() used by xmalloc and other such > functions. And no I don't know much about libgit2. Hm. At first glance that sounds interesting (some kind of mark-and-sweep garbage collection, so at least allocation would never _fail_ due to too large a multipick). At second glance it is less exciting, since the logic would only kick in when malloc() fails, and if I have a lot of swap then my system will have slowed to a crawl long before then. Here are a couple of alternative ideas, with no code to back them up. A. Save a copy of obj_hash after revision traversal and before cherry-picking anything, and reset the table to that state after every (let's say) 5 commits cherry-picked. Or perhaps empty the table completely after every 5 commits or so cherry-picked. B. Re-exec git using "git sequence --continue" so everything gets allocated anew after every 5 commits or so. That second idea sounds pretty ugly, but it would probably be effective. On Windows it's fork(), not execve(), that is expensive, right? :) Just musing, Jonathan ```