From 89e653da5b1bee9cf645e9d4bdd95f6bb31cc4b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Sun, 20 May 2018 12:17:34 +0200 Subject: [PATCH 1/4] merge: setup `opts` later in `checkout_fast_forward()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After we initialize the various fields in `opts` but before we actually use them, we might return early. Move the initialization further down, to immediately before we use `opts`. This limits the scope of `opts` and will help a later commit fix a memory leak without having to worry about those early returns. This patch is best viewed using something like this (note the tab!): --color-moved --anchored=" trees[nr_trees] = parse_tree_indirect" Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- merge.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/merge.c b/merge.c index f06a4773d4..f123658e58 100644 --- a/merge.c +++ b/merge.c @@ -94,8 +94,24 @@ int checkout_fast_forward(const struct object_id *head, return -1; memset(&trees, 0, sizeof(trees)); - memset(&opts, 0, sizeof(opts)); memset(&t, 0, sizeof(t)); + + trees[nr_trees] = parse_tree_indirect(head); + if (!trees[nr_trees++]) { + rollback_lock_file(&lock_file); + return -1; + } + trees[nr_trees] = parse_tree_indirect(remote); + if (!trees[nr_trees++]) { + rollback_lock_file(&lock_file); + return -1; + } + for (i = 0; i < nr_trees; i++) { + parse_tree(trees[i]); + init_tree_desc(t+i, trees[i]->buffer, trees[i]->size); + } + + memset(&opts, 0, sizeof(opts)); if (overwrite_ignore) { memset(&dir, 0, sizeof(dir)); dir.flags |= DIR_SHOW_IGNORED; @@ -112,20 +128,6 @@ int checkout_fast_forward(const struct object_id *head, opts.fn = twoway_merge; setup_unpack_trees_porcelain(&opts, "merge"); - trees[nr_trees] = parse_tree_indirect(head); - if (!trees[nr_trees++]) { - rollback_lock_file(&lock_file); - return -1; - } - trees[nr_trees] = parse_tree_indirect(remote); - if (!trees[nr_trees++]) { - rollback_lock_file(&lock_file); - return -1; - } - for (i = 0; i < nr_trees; i++) { - parse_tree(trees[i]); - init_tree_desc(t+i, trees[i]->buffer, trees[i]->size); - } if (unpack_trees(nr_trees, t, &opts)) { rollback_lock_file(&lock_file); return -1; From 3f1c1c360080114fcc9492211601f41d56112e3c Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sun, 20 May 2018 12:17:35 +0200 Subject: [PATCH 2/4] merge-recursive: provide pair of `unpack_trees_{start,finish}()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename `git_merge_trees()` to `unpack_trees_start()` and extract the call to `discard_index()` into a new function `unpack_trees_finish()`. As a result, these are called early resp. late in `merge_trees()`, making the resource handling clearer. A later commit will expand on that, teaching `..._finish()` to free more memory. (So rather than moving the FIXME-comment, just drop it, since it will be addressed soon enough.) Also call `..._finish()` when `merge_trees()` returns early. Signed-off-by: Elijah Newren Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- merge-recursive.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 680e01226b..ddb0fa7369 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -337,10 +337,10 @@ static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree) init_tree_desc(desc, tree->buffer, tree->size); } -static int git_merge_trees(struct merge_options *o, - struct tree *common, - struct tree *head, - struct tree *merge) +static int unpack_trees_start(struct merge_options *o, + struct tree *common, + struct tree *head, + struct tree *merge) { int rc; struct tree_desc t[3]; @@ -379,6 +379,11 @@ static int git_merge_trees(struct merge_options *o, return rc; } +static void unpack_trees_finish(struct merge_options *o) +{ + discard_index(&o->orig_index); +} + struct tree *write_tree_from_memory(struct merge_options *o) { struct tree *result = NULL; @@ -3088,13 +3093,14 @@ int merge_trees(struct merge_options *o, return 1; } - code = git_merge_trees(o, common, head, merge); + code = unpack_trees_start(o, common, head, merge); if (code != 0) { if (show(o, 4) || o->call_depth) err(o, _("merging of trees %s and %s failed"), oid_to_hex(&head->object.oid), oid_to_hex(&merge->object.oid)); + unpack_trees_finish(o); return -1; } @@ -3147,20 +3153,15 @@ cleanup: hashmap_free(&o->current_file_dir_set, 1); - if (clean < 0) + if (clean < 0) { + unpack_trees_finish(o); return clean; + } } else clean = 1; - /* Free the extra index left from git_merge_trees() */ - /* - * FIXME: Need to also free data allocated by - * setup_unpack_trees_porcelain() tucked away in o->unpack_opts.msgs, - * but the problem is that only half of it refers to dynamically - * allocated data, while the other half points at static strings. - */ - discard_index(&o->orig_index); + unpack_trees_finish(o); if (o->call_depth && !(*result = write_tree_from_memory(o))) return -1; From 342c513a4ae100354097a9ca99a080eeb7e70c0b Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 21 May 2018 16:54:27 +0200 Subject: [PATCH 3/4] argv-array: return the pushed string from argv_push*() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Such an API change allows us to use an argv_array this way: struct argv_array to_free = ARGV_ARRAY_INIT; const char *msg; if (some condition) { msg = "constant string message"; ... other logic ... } else { msg = argv_array_pushf(&to_free, "format %s", var); } ... use "msg" ... ... do other things ... argv_array_clear(&to_free); Note that argv_array_pushl() and argv_array_pushv() are used to push one or more strings with a single call, so we do not return any one of these strings from these two functions in order to reduce the chance to misuse the API. Signed-off-by: Junio C Hamano Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- argv-array.c | 6 ++++-- argv-array.h | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/argv-array.c b/argv-array.c index 5d370fa336..449dfc105a 100644 --- a/argv-array.c +++ b/argv-array.c @@ -21,12 +21,13 @@ static void argv_array_push_nodup(struct argv_array *array, const char *value) array->argv[array->argc] = NULL; } -void argv_array_push(struct argv_array *array, const char *value) +const char *argv_array_push(struct argv_array *array, const char *value) { argv_array_push_nodup(array, xstrdup(value)); + return array->argv[array->argc - 1]; } -void argv_array_pushf(struct argv_array *array, const char *fmt, ...) +const char *argv_array_pushf(struct argv_array *array, const char *fmt, ...) { va_list ap; struct strbuf v = STRBUF_INIT; @@ -36,6 +37,7 @@ void argv_array_pushf(struct argv_array *array, const char *fmt, ...) va_end(ap); argv_array_push_nodup(array, strbuf_detach(&v, NULL)); + return array->argv[array->argc - 1]; } void argv_array_pushl(struct argv_array *array, ...) diff --git a/argv-array.h b/argv-array.h index 29056e49a1..715c93b246 100644 --- a/argv-array.h +++ b/argv-array.h @@ -12,9 +12,9 @@ struct argv_array { #define ARGV_ARRAY_INIT { empty_argv, 0, 0 } void argv_array_init(struct argv_array *); -void argv_array_push(struct argv_array *, const char *); +const char *argv_array_push(struct argv_array *, const char *); __attribute__((format (printf,2,3))) -void argv_array_pushf(struct argv_array *, const char *fmt, ...); +const char *argv_array_pushf(struct argv_array *, const char *fmt, ...); LAST_ARG_MUST_BE_NULL void argv_array_pushl(struct argv_array *, ...); void argv_array_pushv(struct argv_array *, const char **); From 1c41d2805e42d77d943fd3d79ebf5136f74c9ba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Mon, 21 May 2018 16:54:28 +0200 Subject: [PATCH 4/4] unpack_trees_options: free messages when done MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The strings allocated in `setup_unpack_trees_porcelain()` are never freed. Provide a function `clear_unpack_trees_porcelain()` to do so and call it where we use `setup_unpack_trees_porcelain()`. The only non-trivial user is `unpack_trees_start()`, where we should place the new call in `unpack_trees_finish()`. We keep the string pointers in an array, mixing pointers to static memory and memory that we allocate on the heap. We also keep several copies of the individual pointers. So we need to make sure that we do not free what we must not free and that we do not double-free. Let a separate argv_array take ownership of all the strings we create so that we can easily free them. Zero the whole array of string pointers to make sure that we do not leave any dangling pointers. Note that we only take responsibility for the memory allocated in `setup_unpack_trees_porcelain()` and not any other members of the `struct unpack_trees_options`. Helped-by: Junio C Hamano Signed-off-by: Junio C Hamano Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- builtin/checkout.c | 1 + merge-recursive.c | 1 + merge.c | 3 +++ unpack-trees.c | 17 ++++++++++++++--- unpack-trees.h | 8 +++++++- 5 files changed, 26 insertions(+), 4 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index b49b582071..5cebe170fc 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -526,6 +526,7 @@ static int merge_working_tree(const struct checkout_opts *opts, init_tree_desc(&trees[1], tree->buffer, tree->size); ret = unpack_trees(2, trees, &topts); + clear_unpack_trees_porcelain(&topts); if (ret == -1) { /* * Unpack couldn't do a trivial merge; either diff --git a/merge-recursive.c b/merge-recursive.c index ddb0fa7369..338f63a952 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -382,6 +382,7 @@ static int unpack_trees_start(struct merge_options *o, static void unpack_trees_finish(struct merge_options *o) { discard_index(&o->orig_index); + clear_unpack_trees_porcelain(&o->unpack_opts); } struct tree *write_tree_from_memory(struct merge_options *o) diff --git a/merge.c b/merge.c index f123658e58..b433291d0c 100644 --- a/merge.c +++ b/merge.c @@ -130,8 +130,11 @@ int checkout_fast_forward(const struct object_id *head, if (unpack_trees(nr_trees, t, &opts)) { rollback_lock_file(&lock_file); + clear_unpack_trees_porcelain(&opts); return -1; } + clear_unpack_trees_porcelain(&opts); + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) return error(_("unable to write new index file")); return 0; diff --git a/unpack-trees.c b/unpack-trees.c index 79fd97074e..73a6dc1701 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1,5 +1,6 @@ #define NO_THE_INDEX_COMPATIBILITY_MACROS #include "cache.h" +#include "argv-array.h" #include "repository.h" #include "config.h" #include "dir.h" @@ -103,6 +104,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, const char **msgs = opts->msgs; const char *msg; + argv_array_init(&opts->msgs_to_free); + if (!strcmp(cmd, "checkout")) msg = advice_commit_before_merge ? _("Your local changes to the following files would be overwritten by checkout:\n%%s" @@ -119,7 +122,7 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, "Please commit your changes or stash them before you %s.") : _("Your local changes to the following files would be overwritten by %s:\n%%s"); msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] = - xstrfmt(msg, cmd, cmd); + argv_array_pushf(&opts->msgs_to_free, msg, cmd, cmd); msgs[ERROR_NOT_UPTODATE_DIR] = _("Updating the following directories would lose untracked files in them:\n%s"); @@ -139,7 +142,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, ? _("The following untracked working tree files would be removed by %s:\n%%s" "Please move or remove them before you %s.") : _("The following untracked working tree files would be removed by %s:\n%%s"); - msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrfmt(msg, cmd, cmd); + msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = + argv_array_pushf(&opts->msgs_to_free, msg, cmd, cmd); if (!strcmp(cmd, "checkout")) msg = advice_commit_before_merge @@ -156,7 +160,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, ? _("The following untracked working tree files would be overwritten by %s:\n%%s" "Please move or remove them before you %s.") : _("The following untracked working tree files would be overwritten by %s:\n%%s"); - msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrfmt(msg, cmd, cmd); + msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = + argv_array_pushf(&opts->msgs_to_free, msg, cmd, cmd); /* * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we @@ -179,6 +184,12 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, opts->unpack_rejects[i].strdup_strings = 1; } +void clear_unpack_trees_porcelain(struct unpack_trees_options *opts) +{ + argv_array_clear(&opts->msgs_to_free); + memset(opts->msgs, 0, sizeof(opts->msgs)); +} + static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce, unsigned int set, unsigned int clear) { diff --git a/unpack-trees.h b/unpack-trees.h index 41178ada94..c2b434c606 100644 --- a/unpack-trees.h +++ b/unpack-trees.h @@ -2,7 +2,7 @@ #define UNPACK_TREES_H #include "tree-walk.h" -#include "string-list.h" +#include "argv-array.h" #define MAX_UNPACK_TREES 8 @@ -33,6 +33,11 @@ enum unpack_trees_error_types { void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, const char *cmd); +/* + * Frees resources allocated by setup_unpack_trees_porcelain(). + */ +void clear_unpack_trees_porcelain(struct unpack_trees_options *opts); + struct unpack_trees_options { unsigned int reset, merge, @@ -57,6 +62,7 @@ struct unpack_trees_options { struct pathspec *pathspec; merge_fn_t fn; const char *msgs[NB_UNPACK_TREES_ERROR_TYPES]; + struct argv_array msgs_to_free; /* * Store error messages in an array, each case * corresponding to a error message type