* [PATCH 01/10] unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 0:46 ` [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
Almost all users of "struct unpack_trees_options" allocate it on the
stack already, let's provide an *_INIT macro for them to use. This
will make later changes that would like to initialize other members of
the struct easier[1][2], but for now we're keeping it compatible with
a memset() to "0" with an "{ 0 }" initialization.
This covers all callers except a special-case in "merge-recursive.c",
which will be dealt with in a subsequent commit.
1. https://lore.kernel.org/git/87k0ixrv23.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/87fstlrumj.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
archive.c | 3 +--
builtin/am.c | 6 ++----
builtin/checkout.c | 6 ++----
builtin/clone.c | 3 +--
builtin/commit.c | 3 +--
builtin/merge.c | 3 +--
builtin/read-tree.c | 3 +--
builtin/reset.c | 3 +--
builtin/sparse-checkout.c | 3 +--
builtin/stash.c | 6 ++----
diff-lib.c | 3 +--
merge-ort.c | 3 +--
merge.c | 3 +--
reset.c | 2 +-
sequencer.c | 3 +--
unpack-trees.h | 2 ++
16 files changed, 20 insertions(+), 35 deletions(-)
diff --git a/archive.c b/archive.c
index a3bbb091256..210d7235c5a 100644
--- a/archive.c
+++ b/archive.c
@@ -269,7 +269,7 @@ int write_archive_entries(struct archiver_args *args,
write_archive_entry_fn_t write_entry)
{
struct archiver_context context;
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t;
int err;
struct strbuf path_in_archive = STRBUF_INIT;
@@ -300,7 +300,6 @@ int write_archive_entries(struct archiver_args *args,
* Setup index and instruct attr to read index only
*/
if (!args->worktree_attributes) {
- memset(&opts, 0, sizeof(opts));
opts.index_only = 1;
opts.head_idx = -1;
opts.src_index = args->repo->index;
diff --git a/builtin/am.c b/builtin/am.c
index e4a0ff9cd7c..82641ce68ec 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1901,7 +1901,7 @@ static void am_resolve(struct am_state *state)
static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
{
struct lock_file lock_file = LOCK_INIT;
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t[2];
if (parse_tree(head) || parse_tree(remote))
@@ -1911,7 +1911,6 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
refresh_cache(REFRESH_QUIET);
- memset(&opts, 0, sizeof(opts));
opts.head_idx = 1;
opts.src_index = &the_index;
opts.dst_index = &the_index;
@@ -1940,7 +1939,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
static int merge_tree(struct tree *tree)
{
struct lock_file lock_file = LOCK_INIT;
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t[1];
if (parse_tree(tree))
@@ -1948,7 +1947,6 @@ static int merge_tree(struct tree *tree)
hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
- memset(&opts, 0, sizeof(opts));
opts.head_idx = 1;
opts.src_index = &the_index;
opts.dst_index = &the_index;
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8c69dcdf72a..fea4533dbec 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -639,10 +639,9 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
int worktree, int *writeout_error,
struct branch_info *info)
{
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc tree_desc;
- memset(&opts, 0, sizeof(opts));
opts.head_idx = -1;
opts.update = worktree;
opts.skip_unmerged = !worktree;
@@ -719,9 +718,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
} else {
struct tree_desc trees[2];
struct tree *tree;
- struct unpack_trees_options topts;
+ struct unpack_trees_options topts = UNPACK_TREES_OPTIONS_INIT;
- memset(&topts, 0, sizeof(topts));
topts.head_idx = -1;
topts.src_index = &the_index;
topts.dst_index = &the_index;
diff --git a/builtin/clone.c b/builtin/clone.c
index ff1d3d447a3..0df820c5970 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -655,7 +655,7 @@ static int checkout(int submodule_progress)
struct object_id oid;
char *head;
struct lock_file lock_file = LOCK_INIT;
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree *tree;
struct tree_desc t;
int err = 0;
@@ -683,7 +683,6 @@ static int checkout(int submodule_progress)
hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
- memset(&opts, 0, sizeof opts);
opts.update = 1;
opts.merge = 1;
opts.clone = 1;
diff --git a/builtin/commit.c b/builtin/commit.c
index e7320f66f95..6cc7313bad8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -303,7 +303,7 @@ static void add_remove_files(struct string_list *list)
static void create_base_index(const struct commit *current_head)
{
struct tree *tree;
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t;
if (!current_head) {
@@ -311,7 +311,6 @@ static void create_base_index(const struct commit *current_head)
return;
}
- memset(&opts, 0, sizeof(opts));
opts.head_idx = 1;
opts.index_only = 1;
opts.merge = 1;
diff --git a/builtin/merge.c b/builtin/merge.c
index 3fbdacc7db4..73290a07fcc 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -670,9 +670,8 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
int i, nr_trees = 0;
struct tree *trees[MAX_UNPACK_TREES];
struct tree_desc t[MAX_UNPACK_TREES];
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
- memset(&opts, 0, sizeof(opts));
opts.head_idx = 2;
opts.src_index = &the_index;
opts.dst_index = &the_index;
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 485e7b04794..847182fdad6 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -116,7 +116,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
int i, stage = 0;
struct object_id oid;
struct tree_desc t[MAX_UNPACK_TREES];
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
int prefix_set = 0;
struct lock_file lock_file = LOCK_INIT;
const struct option read_tree_options[] = {
@@ -158,7 +158,6 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
OPT_END()
};
- memset(&opts, 0, sizeof(opts));
opts.head_idx = -1;
opts.src_index = &the_index;
opts.dst_index = &the_index;
diff --git a/builtin/reset.c b/builtin/reset.c
index 51c9e2f43ff..86c604b21e9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -51,10 +51,9 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
int i, nr = 0;
struct tree_desc desc[2];
struct tree *tree;
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
int ret = -1;
- memset(&opts, 0, sizeof(opts));
opts.head_idx = 1;
opts.src_index = &the_index;
opts.dst_index = &the_index;
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index d0f5c4702be..4c3c29fb580 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -195,7 +195,7 @@ static void clean_tracked_sparse_directories(struct repository *r)
static int update_working_directory(struct pattern_list *pl)
{
enum update_sparsity_result result;
- struct unpack_trees_options o;
+ struct unpack_trees_options o = UNPACK_TREES_OPTIONS_INIT;
struct lock_file lock_file = LOCK_INIT;
struct repository *r = the_repository;
@@ -205,7 +205,6 @@ static int update_working_directory(struct pattern_list *pl)
r->index->sparse_checkout_patterns = pl;
- memset(&o, 0, sizeof(o));
o.verbose_update = isatty(2);
o.update = 1;
o.head_idx = -1;
diff --git a/builtin/stash.c b/builtin/stash.c
index 8f42360ca91..1137e5fcbe8 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -233,7 +233,7 @@ static int clear_stash(int argc, const char **argv, const char *prefix)
static int reset_tree(struct object_id *i_tree, int update, int reset)
{
int nr_trees = 1;
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t[MAX_UNPACK_TREES];
struct tree *tree;
struct lock_file lock_file = LOCK_INIT;
@@ -244,8 +244,6 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
- memset(&opts, 0, sizeof(opts));
-
tree = parse_tree_indirect(i_tree);
if (parse_tree(tree))
return -1;
@@ -799,7 +797,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
const struct object_id *oid[] = { &info->w_commit, &info->u_tree };
struct tree *tree[ARRAY_SIZE(oid)];
struct tree_desc tree_desc[ARRAY_SIZE(oid)];
- struct unpack_trees_options unpack_tree_opt = { 0 };
+ struct unpack_trees_options unpack_tree_opt = UNPACK_TREES_OPTIONS_INIT;
int i;
for (i = 0; i < ARRAY_SIZE(oid); i++) {
diff --git a/diff-lib.c b/diff-lib.c
index ca085a03efc..8a08d9af4eb 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -526,13 +526,12 @@ static int diff_cache(struct rev_info *revs,
{
struct tree *tree;
struct tree_desc t;
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
tree = parse_tree_indirect(tree_oid);
if (!tree)
return error("bad tree object %s",
tree_name ? tree_name : oid_to_hex(tree_oid));
- memset(&opts, 0, sizeof(opts));
opts.head_idx = 1;
opts.index_only = cached;
opts.diff_index_cached = (cached &&
diff --git a/merge-ort.c b/merge-ort.c
index 35aa979c3a4..75d2b8e4b99 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4021,9 +4021,8 @@ static int checkout(struct merge_options *opt,
/* Switch the index/working copy from old to new */
int ret;
struct tree_desc trees[2];
- struct unpack_trees_options unpack_opts;
+ struct unpack_trees_options unpack_opts = UNPACK_TREES_OPTIONS_INIT;
- memset(&unpack_opts, 0, sizeof(unpack_opts));
unpack_opts.head_idx = -1;
unpack_opts.src_index = opt->repo->index;
unpack_opts.dst_index = opt->repo->index;
diff --git a/merge.c b/merge.c
index 6e736881d90..086f04b0f97 100644
--- a/merge.c
+++ b/merge.c
@@ -50,7 +50,7 @@ int checkout_fast_forward(struct repository *r,
int overwrite_ignore)
{
struct tree *trees[MAX_UNPACK_TREES];
- struct unpack_trees_options opts;
+ struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t[MAX_UNPACK_TREES];
int i, nr_trees = 0;
struct dir_struct dir = DIR_INIT;
@@ -79,7 +79,6 @@ int checkout_fast_forward(struct repository *r,
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
- memset(&opts, 0, sizeof(opts));
if (overwrite_ignore) {
dir.flags |= DIR_SHOW_IGNORED;
setup_standard_excludes(&dir);
diff --git a/reset.c b/reset.c
index 79310ae071b..d13984ab781 100644
--- a/reset.c
+++ b/reset.c
@@ -21,7 +21,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
struct object_id head_oid;
struct tree_desc desc[2] = { { NULL }, { NULL } };
struct lock_file lock = LOCK_INIT;
- struct unpack_trees_options unpack_tree_opts = { 0 };
+ struct unpack_trees_options unpack_tree_opts = UNPACK_TREES_OPTIONS_INIT;
struct tree *tree;
const char *reflog_action;
struct strbuf msg = STRBUF_INIT;
diff --git a/sequencer.c b/sequencer.c
index 614d56f5e21..abd85b6c562 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3652,7 +3652,7 @@ static int do_reset(struct repository *r,
struct lock_file lock = LOCK_INIT;
struct tree_desc desc;
struct tree *tree;
- struct unpack_trees_options unpack_tree_opts;
+ struct unpack_trees_options unpack_tree_opts = UNPACK_TREES_OPTIONS_INIT;
int ret = 0;
if (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0)
@@ -3691,7 +3691,6 @@ static int do_reset(struct repository *r,
}
}
- memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
unpack_tree_opts.head_idx = 1;
unpack_tree_opts.src_index = r->index;
diff --git a/unpack-trees.h b/unpack-trees.h
index 2d88b19dca7..ecf256cbcea 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -91,6 +91,8 @@ struct unpack_trees_options {
struct checkout_metadata meta;
};
+#define UNPACK_TREES_OPTIONS_INIT { 0 }
+
int unpack_trees(unsigned n, struct tree_desc *t,
struct unpack_trees_options *options);
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
2021-10-04 0:46 ` [PATCH 01/10] unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
2021-10-04 0:46 ` [PATCH 03/10] unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT
macro, but "merge-recursive.c" could not be converted to it since
it (re-)initializes a "struct unpack_trees_options" on the heap.
In order to convert use the macro as the source of truth for
initialization we need to not only convert the initialization in
unpack_trees_start(), but also call the new
unpack_trees_options_init() just after the CALLOC_ARRAY() in
merge_start().
When we call merge_trees() we'll call merge_start() followed by
merge_trees_internal(), and it will call unpack_trees_start() before
it does much of anything. So it's covered by an initialization in
unpack_trees_start().
But when merge_recursive() is called it will call merge_start()
followed by merge_recursive_internal(), which won't call
unpack_trees_start() until its own call call to
merge_trees_internal(), but in the meantime it might end up using that
calloc'd "struct unpack_trees_options".
This was OK before, as setup_unpack_trees_porcelain() would call
strvec_init(), and our "struct dir_struct" in turn is OK with being
NULL'd. Let's convert the former to macro initialization, we'll deal
with the latter in a subsequent commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
merge-recursive.c | 3 ++-
unpack-trees.c | 8 ++++++--
unpack-trees.h | 5 ++++-
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index e594d4c3fa1..d24a4903f1d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,
struct tree_desc t[3];
struct index_state tmp_index = { NULL };
- memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
+ unpack_trees_options_init(&opt->priv->unpack_opts);
if (opt->priv->call_depth)
opt->priv->unpack_opts.index_only = 1;
else
@@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
CALLOC_ARRAY(opt->priv, 1);
string_list_init_dup(&opt->priv->df_conflict_file_set);
+ unpack_trees_options_init(&opt->priv->unpack_opts);
return 0;
}
diff --git a/unpack-trees.c b/unpack-trees.c
index 8ea0a542da8..393c1f35a5d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
const char **msgs = opts->msgs;
const char *msg;
- strvec_init(&opts->msgs_to_free);
-
if (!strcmp(cmd, "checkout"))
msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
@@ -189,6 +187,12 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
opts->unpack_rejects[i].strdup_strings = 1;
}
+void unpack_trees_options_init(struct unpack_trees_options *o)
+{
+ struct unpack_trees_options blank = UNPACK_TREES_OPTIONS_INIT;
+ memcpy(o, &blank, sizeof(*o));
+}
+
void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
{
strvec_clear(&opts->msgs_to_free);
diff --git a/unpack-trees.h b/unpack-trees.h
index ecf256cbcea..892b65ea623 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -91,7 +91,10 @@ struct unpack_trees_options {
struct checkout_metadata meta;
};
-#define UNPACK_TREES_OPTIONS_INIT { 0 }
+#define UNPACK_TREES_OPTIONS_INIT { \
+ .msgs_to_free = STRVEC_INIT, \
+}
+void unpack_trees_options_init(struct unpack_trees_options *o);
int unpack_trees(unsigned n, struct tree_desc *t,
struct unpack_trees_options *options);
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function
2021-10-04 0:46 ` [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function Ævar Arnfjörð Bjarmason
@ 2021-10-04 13:45 ` Elijah Newren
2021-10-04 14:41 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT
> macro, but "merge-recursive.c" could not be converted to it since
> it (re-)initializes a "struct unpack_trees_options" on the heap.
>
> In order to convert use the macro as the source of truth for
> initialization we need to not only convert the initialization in
> unpack_trees_start(), but also call the new
> unpack_trees_options_init() just after the CALLOC_ARRAY() in
> merge_start().
Um...why?
> When we call merge_trees() we'll call merge_start() followed by
> merge_trees_internal(), and it will call unpack_trees_start() before
> it does much of anything. So it's covered by an initialization in
> unpack_trees_start().
>
> But when merge_recursive() is called it will call merge_start()
> followed by merge_recursive_internal(), which won't call
> unpack_trees_start() until its own call call to
> merge_trees_internal(), but in the meantime it might end up using that
> calloc'd "struct unpack_trees_options".
Nothing in merge-recursive.c usings unpack_opts before
unpack_trees_start() or after unpack_trees_finish(). If anyone
attempts to use it elsewhere, that would itself be a bug. So, I'd
replace the above three paragraphs with:
"Change the initialization of opt->priv_unpack_opts from using memset
to 0, with unpack_trees_options_init()."
or something like that, and then drop your change to merge_start().
> This was OK before, as setup_unpack_trees_porcelain() would call
> strvec_init(), and our "struct dir_struct" in turn is OK with being
> NULL'd. Let's convert the former to macro initialization, we'll deal
> with the latter in a subsequent commit.
This is quite confusing; it's really hard to understand how this
relates to the rest of the commit message. I have to read the code to
see what you're doing, and then write my own commit message in my head
because the wording in this paragraph still doesn't parse.
I'd make the change strvec_init/STRVEC_INIT changes be a separate
patch. I suspect it'll be easier to write the commit message for it
as a standalone commit as well.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> merge-recursive.c | 3 ++-
> unpack-trees.c | 8 ++++++--
> unpack-trees.h | 5 ++++-
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index e594d4c3fa1..d24a4903f1d 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,
> struct tree_desc t[3];
> struct index_state tmp_index = { NULL };
>
> - memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
> + unpack_trees_options_init(&opt->priv->unpack_opts);
> if (opt->priv->call_depth)
> opt->priv->unpack_opts.index_only = 1;
> else
> @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
>
> CALLOC_ARRAY(opt->priv, 1);
> string_list_init_dup(&opt->priv->df_conflict_file_set);
> + unpack_trees_options_init(&opt->priv->unpack_opts);
Drop this hunk.
> return 0;
> }
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 8ea0a542da8..393c1f35a5d 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -108,8 +108,6 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
> const char **msgs = opts->msgs;
> const char *msg;
>
> - strvec_init(&opts->msgs_to_free);
> -
> if (!strcmp(cmd, "checkout"))
> msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
> ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
> @@ -189,6 +187,12 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
> opts->unpack_rejects[i].strdup_strings = 1;
> }
>
> +void unpack_trees_options_init(struct unpack_trees_options *o)
> +{
> + struct unpack_trees_options blank = UNPACK_TREES_OPTIONS_INIT;
> + memcpy(o, &blank, sizeof(*o));
> +}
> +
> void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
> {
> strvec_clear(&opts->msgs_to_free);
> diff --git a/unpack-trees.h b/unpack-trees.h
> index ecf256cbcea..892b65ea623 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -91,7 +91,10 @@ struct unpack_trees_options {
> struct checkout_metadata meta;
> };
>
> -#define UNPACK_TREES_OPTIONS_INIT { 0 }
> +#define UNPACK_TREES_OPTIONS_INIT { \
> + .msgs_to_free = STRVEC_INIT, \
> +}
> +void unpack_trees_options_init(struct unpack_trees_options *o);
>
> int unpack_trees(unsigned n, struct tree_desc *t,
> struct unpack_trees_options *options);
> --
> 2.33.0.1404.g83021034c5d
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function
2021-10-04 13:45 ` Elijah Newren
@ 2021-10-04 14:41 ` Ævar Arnfjörð Bjarmason
2021-10-04 15:04 ` Elijah Newren
0 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 14:41 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Mon, Oct 04 2021, Elijah Newren wrote:
> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT
>> macro, but "merge-recursive.c" could not be converted to it since
>> it (re-)initializes a "struct unpack_trees_options" on the heap.
>>
>> In order to convert use the macro as the source of truth for
>> initialization we need to not only convert the initialization in
>> unpack_trees_start(), but also call the new
>> unpack_trees_options_init() just after the CALLOC_ARRAY() in
>> merge_start().
>
> Um...why?
Replied below.
>> When we call merge_trees() we'll call merge_start() followed by
>> merge_trees_internal(), and it will call unpack_trees_start() before
>> it does much of anything. So it's covered by an initialization in
>> unpack_trees_start().
>>
>> But when merge_recursive() is called it will call merge_start()
>> followed by merge_recursive_internal(), which won't call
>> unpack_trees_start() until its own call call to
>> merge_trees_internal(), but in the meantime it might end up using that
>> calloc'd "struct unpack_trees_options".
>
> Nothing in merge-recursive.c usings unpack_opts before
> unpack_trees_start() or after unpack_trees_finish(). If anyone
> attempts to use it elsewhere, that would itself be a bug. So, I'd
> replace the above three paragraphs with:
>
> "Change the initialization of opt->priv_unpack_opts from using memset
> to 0, with unpack_trees_options_init()."
>
> or something like that, and then drop your change to merge_start().
Sure, I'll defer to you about being confident about that. I didn't want
to leave a copy if it hanging without the proper initialization in case
there were new callers.
>> This was OK before, as setup_unpack_trees_porcelain() would call
>> strvec_init(), and our "struct dir_struct" in turn is OK with being
>> NULL'd. Let's convert the former to macro initialization, we'll deal
>> with the latter in a subsequent commit.
>
> This is quite confusing; it's really hard to understand how this
> relates to the rest of the commit message. I have to read the code to
> see what you're doing, and then write my own commit message in my head
> because the wording in this paragraph still doesn't parse.
>
> I'd make the change strvec_init/STRVEC_INIT changes be a separate
> patch. I suspect it'll be easier to write the commit message for it
> as a standalone commit as well.
Sure, FWIW I had it split up locally, and figured it would be easier to
explain if the API usage change came with the initialization change that
made it possible.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> merge-recursive.c | 3 ++-
>> unpack-trees.c | 8 ++++++--
>> unpack-trees.h | 5 ++++-
>> 3 files changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index e594d4c3fa1..d24a4903f1d 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,
>> struct tree_desc t[3];
>> struct index_state tmp_index = { NULL };
>>
>> - memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
>> + unpack_trees_options_init(&opt->priv->unpack_opts);
>> if (opt->priv->call_depth)
>> opt->priv->unpack_opts.index_only = 1;
>> else
>> @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
>>
>> CALLOC_ARRAY(opt->priv, 1);
>> string_list_init_dup(&opt->priv->df_conflict_file_set);
>> + unpack_trees_options_init(&opt->priv->unpack_opts);
>
> Drop this hunk.
Speaking of splitting things up in more understandable ways: If we're
going to hard rely on the interaction between merge_start() and
unpack_trees_start() wouldn't it make sense to lead with a change that
dropped that memset, which if this invariant holds is also redundant to
the CALLOC() of opt->priv in merge_start() in the pre-image.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function
2021-10-04 14:41 ` Ævar Arnfjörð Bjarmason
@ 2021-10-04 15:04 ` Elijah Newren
0 siblings, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 15:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Mon, Oct 4, 2021 at 7:49 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Mon, Oct 04 2021, Elijah Newren wrote:
>
> > On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >>
> >> In the preceding commit we introduced a new UNPACK_TREES_OPTIONS_INIT
> >> macro, but "merge-recursive.c" could not be converted to it since
> >> it (re-)initializes a "struct unpack_trees_options" on the heap.
> >>
> >> In order to convert use the macro as the source of truth for
> >> initialization we need to not only convert the initialization in
> >> unpack_trees_start(), but also call the new
> >> unpack_trees_options_init() just after the CALLOC_ARRAY() in
> >> merge_start().
> >
> > Um...why?
>
> Replied below.
>
> >> When we call merge_trees() we'll call merge_start() followed by
> >> merge_trees_internal(), and it will call unpack_trees_start() before
> >> it does much of anything. So it's covered by an initialization in
> >> unpack_trees_start().
> >>
> >> But when merge_recursive() is called it will call merge_start()
> >> followed by merge_recursive_internal(), which won't call
> >> unpack_trees_start() until its own call call to
> >> merge_trees_internal(), but in the meantime it might end up using that
> >> calloc'd "struct unpack_trees_options".
> >
> > Nothing in merge-recursive.c usings unpack_opts before
> > unpack_trees_start() or after unpack_trees_finish(). If anyone
> > attempts to use it elsewhere, that would itself be a bug. So, I'd
> > replace the above three paragraphs with:
> >
> > "Change the initialization of opt->priv_unpack_opts from using memset
> > to 0, with unpack_trees_options_init()."
> >
> > or something like that, and then drop your change to merge_start().
>
> Sure, I'll defer to you about being confident about that. I didn't want
> to leave a copy if it hanging without the proper initialization in case
> there were new callers.
>
> >> This was OK before, as setup_unpack_trees_porcelain() would call
> >> strvec_init(), and our "struct dir_struct" in turn is OK with being
> >> NULL'd. Let's convert the former to macro initialization, we'll deal
> >> with the latter in a subsequent commit.
> >
> > This is quite confusing; it's really hard to understand how this
> > relates to the rest of the commit message. I have to read the code to
> > see what you're doing, and then write my own commit message in my head
> > because the wording in this paragraph still doesn't parse.
> >
> > I'd make the change strvec_init/STRVEC_INIT changes be a separate
> > patch. I suspect it'll be easier to write the commit message for it
> > as a standalone commit as well.
>
> Sure, FWIW I had it split up locally, and figured it would be easier to
> explain if the API usage change came with the initialization change that
> made it possible.
>
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >> merge-recursive.c | 3 ++-
> >> unpack-trees.c | 8 ++++++--
> >> unpack-trees.h | 5 ++++-
> >> 3 files changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/merge-recursive.c b/merge-recursive.c
> >> index e594d4c3fa1..d24a4903f1d 100644
> >> --- a/merge-recursive.c
> >> +++ b/merge-recursive.c
> >> @@ -405,7 +405,7 @@ static int unpack_trees_start(struct merge_options *opt,
> >> struct tree_desc t[3];
> >> struct index_state tmp_index = { NULL };
> >>
> >> - memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
> >> + unpack_trees_options_init(&opt->priv->unpack_opts);
> >> if (opt->priv->call_depth)
> >> opt->priv->unpack_opts.index_only = 1;
> >> else
> >> @@ -3704,6 +3704,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
> >>
> >> CALLOC_ARRAY(opt->priv, 1);
> >> string_list_init_dup(&opt->priv->df_conflict_file_set);
> >> + unpack_trees_options_init(&opt->priv->unpack_opts);
> >
> > Drop this hunk.
>
> Speaking of splitting things up in more understandable ways: If we're
> going to hard rely on the interaction between merge_start() and
> unpack_trees_start() wouldn't it make sense to lead with a change that
> dropped that memset, which if this invariant holds is also redundant to
> the CALLOC() of opt->priv in merge_start() in the pre-image.
It is not redundant, and that would be a very confusing change.
merge-recursive is so named because its main driving function contains
recursive calls to itself. merge_start() is not involved in that
recursion. For readability, we should initialize unpack_opts before
each use.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 03/10] unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
2021-10-04 0:46 ` [PATCH 01/10] unpack-trees.[ch]: define and use a UNPACK_TREES_OPTIONS_INIT Ævar Arnfjörð Bjarmason
2021-10-04 0:46 ` [PATCH 02/10] merge-recursive.c: call a new unpack_trees_options_init() function Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
2021-10-04 0:46 ` [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
Change the "struct dir" in "struct unpack_trees_options" away from a
pointer to being embedded in the struct itself, this mean that we can
initialize it with our new UNPACK_TREES_OPTIONS_INIT macro.
As it turned out nothing actually needed to provide its own "struct
dir" to this API, there's another patch to "hide" the struct instead,
see the discussion at [1], but I think just allocating it on the stack
along with the rest makes more sense.
This fixes a memory leak in "builtin/checkout.c" that's been there
since the clear_unpack_trees_porcelain() function was added in
1c41d2805e4 (unpack_trees_options: free messages when done,
2018-05-21), i.e. we still had a memory leak from allocating the "dir"
member here. That "dir" member had in turn been with us ever since
782c2d65c24 (Build in checkout, 2008-02-07).
This fixes that memory leak, and allows us to remove the boilerplate
dir_clear() elsewhere in favor of just using
clear_unpack_trees_porcelain().
1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/checkout.c | 5 ++---
builtin/read-tree.c | 11 ++++-------
merge-ort.c | 7 ++-----
merge.c | 7 ++-----
unpack-trees.c | 10 ++++------
unpack-trees.h | 4 +++-
6 files changed, 17 insertions(+), 27 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index fea4533dbec..d4b88affba7 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -745,9 +745,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
&new_branch_info->commit->object.oid :
&new_branch_info->oid, NULL);
if (opts->overwrite_ignore) {
- topts.dir = xcalloc(1, sizeof(*topts.dir));
- topts.dir->flags |= DIR_SHOW_IGNORED;
- setup_standard_excludes(topts.dir);
+ topts.dir.flags |= DIR_SHOW_IGNORED;
+ setup_standard_excludes(&topts.dir);
}
tree = parse_tree_indirect(old_branch_info->commit ?
&old_branch_info->commit->object.oid :
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 847182fdad6..06f3b97ac05 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -53,20 +53,17 @@ static int index_output_cb(const struct option *opt, const char *arg,
static int exclude_per_directory_cb(const struct option *opt, const char *arg,
int unset)
{
- struct dir_struct *dir;
struct unpack_trees_options *opts;
BUG_ON_OPT_NEG(unset);
opts = (struct unpack_trees_options *)opt->value;
- if (opts->dir)
+ if (opts->dir.exclude_per_dir)
die("more than one --exclude-per-directory given.");
- dir = xcalloc(1, sizeof(*opts->dir));
- dir->flags |= DIR_SHOW_IGNORED;
- dir->exclude_per_dir = arg;
- opts->dir = dir;
+ opts->dir.flags |= DIR_SHOW_IGNORED;
+ opts->dir.exclude_per_dir = arg;
/* We do not need to nor want to do read-directory
* here; we are merely interested in reusing the
* per directory ignore stack mechanism.
@@ -208,7 +205,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
if ((opts.update || opts.index_only) && !opts.merge)
die("%s is meaningless without -m, --reset, or --prefix",
opts.update ? "-u" : "-i");
- if ((opts.dir && !opts.update))
+ if ((opts.dir.exclude_per_dir && !opts.update))
die("--exclude-per-directory is meaningless unless -u");
if (opts.merge && !opts.index_only)
setup_work_tree();
diff --git a/merge-ort.c b/merge-ort.c
index 75d2b8e4b99..e526b78b88d 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4045,9 +4045,8 @@ static int checkout(struct merge_options *opt,
unpack_opts.verbose_update = (opt->verbosity > 2);
unpack_opts.fn = twoway_merge;
if (1/* FIXME: opts->overwrite_ignore*/) {
- CALLOC_ARRAY(unpack_opts.dir, 1);
- unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
- setup_standard_excludes(unpack_opts.dir);
+ unpack_opts.dir.flags |= DIR_SHOW_IGNORED;
+ setup_standard_excludes(&unpack_opts.dir);
}
parse_tree(prev);
init_tree_desc(&trees[0], prev->buffer, prev->size);
@@ -4056,8 +4055,6 @@ static int checkout(struct merge_options *opt,
ret = unpack_trees(2, trees, &unpack_opts);
clear_unpack_trees_porcelain(&unpack_opts);
- dir_clear(unpack_opts.dir);
- FREE_AND_NULL(unpack_opts.dir);
return ret;
}
diff --git a/merge.c b/merge.c
index 086f04b0f97..9cb32990dd9 100644
--- a/merge.c
+++ b/merge.c
@@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r,
struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t[MAX_UNPACK_TREES];
int i, nr_trees = 0;
- struct dir_struct dir = DIR_INIT;
struct lock_file lock_file = LOCK_INIT;
refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
@@ -80,9 +79,8 @@ int checkout_fast_forward(struct repository *r,
}
if (overwrite_ignore) {
- dir.flags |= DIR_SHOW_IGNORED;
- setup_standard_excludes(&dir);
- opts.dir = &dir;
+ opts.dir.flags |= DIR_SHOW_IGNORED;
+ setup_standard_excludes(&opts.dir);
}
opts.head_idx = 1;
@@ -100,7 +98,6 @@ int checkout_fast_forward(struct repository *r,
clear_unpack_trees_porcelain(&opts);
return -1;
}
- dir_clear(&dir);
clear_unpack_trees_porcelain(&opts);
if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
diff --git a/unpack-trees.c b/unpack-trees.c
index 393c1f35a5d..94767d3f96f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -196,6 +196,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)
void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
{
strvec_clear(&opts->msgs_to_free);
+ dir_clear(&opts->dir);
memset(opts->msgs, 0, sizeof(opts->msgs));
}
@@ -2085,7 +2086,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
*/
int namelen;
int i;
- struct dir_struct d;
+ struct dir_struct d = DIR_INIT;
char *pathbuf;
int cnt = 0;
@@ -2136,9 +2137,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
*/
pathbuf = xstrfmt("%.*s/", namelen, ce->name);
- memset(&d, 0, sizeof(d));
- if (o->dir)
- d.exclude_per_dir = o->dir->exclude_per_dir;
+ d.exclude_per_dir = o->dir.exclude_per_dir;
i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
if (i)
return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
@@ -2179,8 +2178,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
if (ignore_case && icase_exists(o, name, len, st))
return 0;
- if (o->dir &&
- is_excluded(o->dir, o->src_index, name, &dtype))
+ if (is_excluded(&o->dir, o->src_index, name, &dtype))
/*
* ce->name is explicitly excluded, so it is Ok to
* overwrite it.
diff --git a/unpack-trees.h b/unpack-trees.h
index 892b65ea623..40c4841748d 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -5,6 +5,7 @@
#include "strvec.h"
#include "string-list.h"
#include "tree-walk.h"
+#include "dir.h"
#define MAX_UNPACK_TREES MAX_TRAVERSE_TREES
@@ -66,7 +67,7 @@ struct unpack_trees_options {
dry_run;
const char *prefix;
int cache_bottom;
- struct dir_struct *dir;
+ struct dir_struct dir;
struct pathspec *pathspec;
merge_fn_t fn;
const char *msgs[NB_UNPACK_TREES_WARNING_TYPES];
@@ -93,6 +94,7 @@ struct unpack_trees_options {
#define UNPACK_TREES_OPTIONS_INIT { \
.msgs_to_free = STRVEC_INIT, \
+ .dir = DIR_INIT, \
}
void unpack_trees_options_init(struct unpack_trees_options *o);
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 03/10] unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"
2021-10-04 0:46 ` [PATCH 03/10] unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options" Ævar Arnfjörð Bjarmason
@ 2021-10-04 13:45 ` Elijah Newren
0 siblings, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Change the "struct dir" in "struct unpack_trees_options" away from a
> pointer to being embedded in the struct itself, this mean that we can
> initialize it with our new UNPACK_TREES_OPTIONS_INIT macro.
>
> As it turned out nothing actually needed to provide its own "struct
> dir" to this API, there's another patch to "hide" the struct instead,
> see the discussion at [1], but I think just allocating it on the stack
> along with the rest makes more sense.
>
> This fixes a memory leak in "builtin/checkout.c" that's been there
> since the clear_unpack_trees_porcelain() function was added in
> 1c41d2805e4 (unpack_trees_options: free messages when done,
> 2018-05-21), i.e. we still had a memory leak from allocating the "dir"
> member here. That "dir" member had in turn been with us ever since
> 782c2d65c24 (Build in checkout, 2008-02-07).
>
> This fixes that memory leak, and allows us to remove the boilerplate
> dir_clear() elsewhere in favor of just using
> clear_unpack_trees_porcelain().
>
> 1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/checkout.c | 5 ++---
> builtin/read-tree.c | 11 ++++-------
> merge-ort.c | 7 ++-----
> merge.c | 7 ++-----
> unpack-trees.c | 10 ++++------
> unpack-trees.h | 4 +++-
> 6 files changed, 17 insertions(+), 27 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index fea4533dbec..d4b88affba7 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -745,9 +745,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
> &new_branch_info->commit->object.oid :
> &new_branch_info->oid, NULL);
> if (opts->overwrite_ignore) {
> - topts.dir = xcalloc(1, sizeof(*topts.dir));
> - topts.dir->flags |= DIR_SHOW_IGNORED;
> - setup_standard_excludes(topts.dir);
> + topts.dir.flags |= DIR_SHOW_IGNORED;
> + setup_standard_excludes(&topts.dir);
> }
> tree = parse_tree_indirect(old_branch_info->commit ?
> &old_branch_info->commit->object.oid :
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 847182fdad6..06f3b97ac05 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -53,20 +53,17 @@ static int index_output_cb(const struct option *opt, const char *arg,
> static int exclude_per_directory_cb(const struct option *opt, const char *arg,
> int unset)
> {
> - struct dir_struct *dir;
> struct unpack_trees_options *opts;
>
> BUG_ON_OPT_NEG(unset);
>
> opts = (struct unpack_trees_options *)opt->value;
>
> - if (opts->dir)
> + if (opts->dir.exclude_per_dir)
> die("more than one --exclude-per-directory given.");
>
> - dir = xcalloc(1, sizeof(*opts->dir));
> - dir->flags |= DIR_SHOW_IGNORED;
> - dir->exclude_per_dir = arg;
> - opts->dir = dir;
> + opts->dir.flags |= DIR_SHOW_IGNORED;
> + opts->dir.exclude_per_dir = arg;
> /* We do not need to nor want to do read-directory
> * here; we are merely interested in reusing the
> * per directory ignore stack mechanism.
> @@ -208,7 +205,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
> if ((opts.update || opts.index_only) && !opts.merge)
> die("%s is meaningless without -m, --reset, or --prefix",
> opts.update ? "-u" : "-i");
> - if ((opts.dir && !opts.update))
> + if ((opts.dir.exclude_per_dir && !opts.update))
> die("--exclude-per-directory is meaningless unless -u");
> if (opts.merge && !opts.index_only)
> setup_work_tree();
> diff --git a/merge-ort.c b/merge-ort.c
> index 75d2b8e4b99..e526b78b88d 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4045,9 +4045,8 @@ static int checkout(struct merge_options *opt,
> unpack_opts.verbose_update = (opt->verbosity > 2);
> unpack_opts.fn = twoway_merge;
> if (1/* FIXME: opts->overwrite_ignore*/) {
> - CALLOC_ARRAY(unpack_opts.dir, 1);
> - unpack_opts.dir->flags |= DIR_SHOW_IGNORED;
> - setup_standard_excludes(unpack_opts.dir);
> + unpack_opts.dir.flags |= DIR_SHOW_IGNORED;
> + setup_standard_excludes(&unpack_opts.dir);
> }
> parse_tree(prev);
> init_tree_desc(&trees[0], prev->buffer, prev->size);
> @@ -4056,8 +4055,6 @@ static int checkout(struct merge_options *opt,
>
> ret = unpack_trees(2, trees, &unpack_opts);
> clear_unpack_trees_porcelain(&unpack_opts);
> - dir_clear(unpack_opts.dir);
> - FREE_AND_NULL(unpack_opts.dir);
> return ret;
> }
>
> diff --git a/merge.c b/merge.c
> index 086f04b0f97..9cb32990dd9 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -53,7 +53,6 @@ int checkout_fast_forward(struct repository *r,
> struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> struct tree_desc t[MAX_UNPACK_TREES];
> int i, nr_trees = 0;
> - struct dir_struct dir = DIR_INIT;
> struct lock_file lock_file = LOCK_INIT;
>
> refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
> @@ -80,9 +79,8 @@ int checkout_fast_forward(struct repository *r,
> }
>
> if (overwrite_ignore) {
> - dir.flags |= DIR_SHOW_IGNORED;
> - setup_standard_excludes(&dir);
> - opts.dir = &dir;
> + opts.dir.flags |= DIR_SHOW_IGNORED;
> + setup_standard_excludes(&opts.dir);
> }
>
> opts.head_idx = 1;
> @@ -100,7 +98,6 @@ int checkout_fast_forward(struct repository *r,
> clear_unpack_trees_porcelain(&opts);
> return -1;
> }
> - dir_clear(&dir);
> clear_unpack_trees_porcelain(&opts);
>
> if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 393c1f35a5d..94767d3f96f 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -196,6 +196,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)
> void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
> {
> strvec_clear(&opts->msgs_to_free);
> + dir_clear(&opts->dir);
> memset(opts->msgs, 0, sizeof(opts->msgs));
> }
>
> @@ -2085,7 +2086,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
> */
> int namelen;
> int i;
> - struct dir_struct d;
> + struct dir_struct d = DIR_INIT;
> char *pathbuf;
> int cnt = 0;
>
> @@ -2136,9 +2137,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
> */
> pathbuf = xstrfmt("%.*s/", namelen, ce->name);
>
> - memset(&d, 0, sizeof(d));
> - if (o->dir)
> - d.exclude_per_dir = o->dir->exclude_per_dir;
> + d.exclude_per_dir = o->dir.exclude_per_dir;
> i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
> if (i)
> return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
> @@ -2179,8 +2178,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
> if (ignore_case && icase_exists(o, name, len, st))
> return 0;
>
> - if (o->dir &&
> - is_excluded(o->dir, o->src_index, name, &dtype))
> + if (is_excluded(&o->dir, o->src_index, name, &dtype))
> /*
> * ce->name is explicitly excluded, so it is Ok to
> * overwrite it.
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 892b65ea623..40c4841748d 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -5,6 +5,7 @@
> #include "strvec.h"
> #include "string-list.h"
> #include "tree-walk.h"
> +#include "dir.h"
>
> #define MAX_UNPACK_TREES MAX_TRAVERSE_TREES
>
> @@ -66,7 +67,7 @@ struct unpack_trees_options {
> dry_run;
> const char *prefix;
> int cache_bottom;
> - struct dir_struct *dir;
> + struct dir_struct dir;
> struct pathspec *pathspec;
> merge_fn_t fn;
> const char *msgs[NB_UNPACK_TREES_WARNING_TYPES];
> @@ -93,6 +94,7 @@ struct unpack_trees_options {
>
> #define UNPACK_TREES_OPTIONS_INIT { \
> .msgs_to_free = STRVEC_INIT, \
> + .dir = DIR_INIT, \
> }
> void unpack_trees_options_init(struct unpack_trees_options *o);
>
> --
> 2.33.0.1404.g83021034c5d
This patch is going to conflict HEAVILY with
en/removing-untracked-fixes, which Junio already said he'd merge to
next.
You should rebase your patches on that series.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-10-04 0:46 ` [PATCH 03/10] unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options" Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 9:31 ` Phillip Wood
2021-10-04 13:45 ` Elijah Newren
2021-10-04 0:46 ` [PATCH 05/10] dir.[ch]: make DIR_INIT mandatory Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
Change the clear_unpack_trees_porcelain() to be like a *_release()
function, not a *_reset() (in strbuf.c terms). Let's move the only API
user that relied on the latter to doing its own
unpack_trees_options_init(). See the commit that introduced
unpack_trees_options_init() for details on the control flow involved
here.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
merge-recursive.c | 1 +
unpack-trees.c | 1 -
2 files changed, 1 insertion(+), 1 deletion(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index d24a4903f1d..a77f66b006c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
{
discard_index(&opt->priv->orig_index);
clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
+ unpack_trees_options_init(&opt->priv->unpack_opts);
}
static int save_files_dirs(const struct object_id *oid,
diff --git a/unpack-trees.c b/unpack-trees.c
index 94767d3f96f..e7365322e82 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
{
strvec_clear(&opts->msgs_to_free);
dir_clear(&opts->dir);
- memset(opts->msgs, 0, sizeof(opts->msgs));
}
static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset
2021-10-04 0:46 ` [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset Ævar Arnfjörð Bjarmason
@ 2021-10-04 9:31 ` Phillip Wood
2021-10-04 11:12 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
1 sibling, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2021-10-04 9:31 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
Hi Ævar
On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
> Change the clear_unpack_trees_porcelain() to be like a *_release()
> function, not a *_reset() (in strbuf.c terms). Let's move the only API
> user that relied on the latter to doing its own
> unpack_trees_options_init(). See the commit that introduced
> unpack_trees_options_init() for details on the control flow involved
> here.
Before this change if there was a call to unpack_trees() after
clear_unpack_trees_porcelain() then that caller would get the default
error messages. After this change we end up with a use-after-free error
in that situation. I found the subject line of this patch hard to
understand, the commit message explains what it is doing but I'm still
not sure what the motivation for this change is.
Best Wishes
Phillip
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> merge-recursive.c | 1 +
> unpack-trees.c | 1 -
> 2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d24a4903f1d..a77f66b006c 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
> {
> discard_index(&opt->priv->orig_index);
> clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
> + unpack_trees_options_init(&opt->priv->unpack_opts);
> }
>
> static int save_files_dirs(const struct object_id *oid,
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 94767d3f96f..e7365322e82 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
> {
> strvec_clear(&opts->msgs_to_free);
> dir_clear(&opts->dir);
> - memset(opts->msgs, 0, sizeof(opts->msgs));
> }
>
> static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset
2021-10-04 9:31 ` Phillip Wood
@ 2021-10-04 11:12 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 11:12 UTC (permalink / raw)
To: phillip.wood
Cc: git, Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Mon, Oct 04 2021, Phillip Wood wrote:
> Hi Ævar
>
> On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
>> Change the clear_unpack_trees_porcelain() to be like a *_release()
>> function, not a *_reset() (in strbuf.c terms). Let's move the only API
>> user that relied on the latter to doing its own
>> unpack_trees_options_init(). See the commit that introduced
>> unpack_trees_options_init() for details on the control flow involved
>> here.
>
> Before this change if there was a call to unpack_trees() after
> clear_unpack_trees_porcelain() then that caller would get the default
> error messages. After this change we end up with a use-after-free
> error in that situation. I found the subject line of this patch hard
> to understand, the commit message explains what it is doing but I'm
> still not sure what the motivation for this change is.
I'll work on the commit message part.
With this series such a caller is purely hypothetical, isn't it?
I.e. the journey in 02/10 & 04/10, and later in the 07/10 you commented
on is to make the API behave similarly to e.g. strbuf, where there's a
release() that leaves it in such a state, different from a "reset".
Perhaps this step in isolation is confusing at it leaves the function
named as clear_unpack_trees_porcelain(). I had this all in one change
initially, but figured having such a large rename diff & one behavior
change was worse.
We could just leave the "reset" semantics in place for everyone, but
just like "strbuf_release()" et al I think it's good for
self-documentation purposes to explicitly make clear if you're re-using
the struct, or just freeing it at the end.
For this API only one user of the API cares about the re-use case,
merge-recursive.c.
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> merge-recursive.c | 1 +
>> unpack-trees.c | 1 -
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index d24a4903f1d..a77f66b006c 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
>> {
>> discard_index(&opt->priv->orig_index);
>> clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
>> + unpack_trees_options_init(&opt->priv->unpack_opts);
>> }
>> static int save_files_dirs(const struct object_id *oid,
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 94767d3f96f..e7365322e82 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
>> {
>> strvec_clear(&opts->msgs_to_free);
>> dir_clear(&opts->dir);
>> - memset(opts->msgs, 0, sizeof(opts->msgs));
>> }
>> static int do_add_entry(struct unpack_trees_options *o, struct
>> cache_entry *ce,
>>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset
2021-10-04 0:46 ` [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset Ævar Arnfjörð Bjarmason
2021-10-04 9:31 ` Phillip Wood
@ 2021-10-04 13:45 ` Elijah Newren
2021-10-04 15:20 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Change the clear_unpack_trees_porcelain() to be like a *_release()
> function, not a *_reset() (in strbuf.c terms). Let's move the only API
> user that relied on the latter to doing its own
> unpack_trees_options_init(). See the commit that introduced
> unpack_trees_options_init() for details on the control flow involved
> here.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> merge-recursive.c | 1 +
> unpack-trees.c | 1 -
> 2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d24a4903f1d..a77f66b006c 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
> {
> discard_index(&opt->priv->orig_index);
> clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
> + unpack_trees_options_init(&opt->priv->unpack_opts);
This is wrong. It suggests that unpack_opts is used after
unpack_trees_finish() (other than an outer merge first calling
unpack_trees_start() again), which can only serve to greatly confuse
future readers. Drop this hunk.
> }
>
> static int save_files_dirs(const struct object_id *oid,
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 94767d3f96f..e7365322e82 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
> {
> strvec_clear(&opts->msgs_to_free);
> dir_clear(&opts->dir);
> - memset(opts->msgs, 0, sizeof(opts->msgs));
This seems like a very dangerous change. You want to leave opts->msgs
pointing at freed memory?
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset
2021-10-04 13:45 ` Elijah Newren
@ 2021-10-04 15:20 ` Ævar Arnfjörð Bjarmason
2021-10-04 16:28 ` Elijah Newren
0 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 15:20 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Mon, Oct 04 2021, Elijah Newren wrote:
> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> Change the clear_unpack_trees_porcelain() to be like a *_release()
>> function, not a *_reset() (in strbuf.c terms). Let's move the only API
>> user that relied on the latter to doing its own
>> unpack_trees_options_init(). See the commit that introduced
>> unpack_trees_options_init() for details on the control flow involved
>> here.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> merge-recursive.c | 1 +
>> unpack-trees.c | 1 -
>> 2 files changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index d24a4903f1d..a77f66b006c 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
>> {
>> discard_index(&opt->priv->orig_index);
>> clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
>> + unpack_trees_options_init(&opt->priv->unpack_opts);
>
> This is wrong. It suggests that unpack_opts is used after
> unpack_trees_finish() (other than an outer merge first calling
> unpack_trees_start() again), which can only serve to greatly confuse
> future readers. Drop this hunk.
Sure, but (and also re:
https://lore.kernel.org/git/CABPp-BEA2myh2Np_YpFWnE+jqmT5vz7ohigZ0=2tL-wizgYQmg@mail.gmail.com/)
if you'd like not initialize things in merge_start() just for good
measure wouldn't the diff-at-the-end on top of your 5bf7e5779ec
(merge-recursive: split internal fields into a separate struct,
2019-08-17) also make sense?
I.e. the reason I entered this particular rabbit hole was in looking at
existing members of "struct merge_options_internal" & past commits and
seeing how we did its initialization. That canary on top passes all our
tests, and per my reading we also don't use "df_conflict_file_set" until
as late as the things we setup in unpack_trees_start(). Should those be
moved to do the post-merge_start() setup at the same time?
>> }
>>
>> static int save_files_dirs(const struct object_id *oid,
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 94767d3f96f..e7365322e82 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
>> {
>> strvec_clear(&opts->msgs_to_free);
>> dir_clear(&opts->dir);
>> - memset(opts->msgs, 0, sizeof(opts->msgs));
>
> This seems like a very dangerous change. You want to leave opts->msgs
> pointing at freed memory?
Yes, as argued in
http://lore.kernel.org/git/87bl45niqs.fsf@evledraar.gmail.com; In this
series we can see that nothing re-uses it, so it's as safe as our
strbuf_release(), or a plain free().
Maybe I'm misunderstanding what you're getting at, and I could
understand a "let's just reset it for good measure" POV. But I can't
square your view that we shouldn't do setup in merge_start() for good
measure in case some new future code accidentally uses the data earlier
(which I'm fine with), but then also not finding it OK to skip the
memset() here ...
diff --git a/merge-recursive.c b/merge-recursive.c
index e594d4c3fa1..6d2b8e78896 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -601,6 +601,7 @@ static void record_df_conflict_files(struct merge_options *opt,
df_sorted_entries.cmp = string_list_df_name_compare;
string_list_sort(&df_sorted_entries);
+ assert(opt->priv->df_conflict_file_set.nr != 123456);
string_list_clear(&opt->priv->df_conflict_file_set, 1);
for (i = 0; i < df_sorted_entries.nr; i++) {
const char *path = df_sorted_entries.items[i].string;
@@ -869,6 +870,7 @@ static int make_room_for_path(struct merge_options *opt, const char *path)
const char *msg = _("failed to create path '%s'%s");
/* Unlink any D/F conflict files that are in the way */
+ assert(opt->priv->df_conflict_file_set.nr != 123456);
for (i = 0; i < opt->priv->df_conflict_file_set.nr; i++) {
const char *df_path = opt->priv->df_conflict_file_set.items[i].string;
size_t pathlen = strlen(path);
@@ -3467,6 +3469,7 @@ static int merge_trees_internal(struct merge_options *opt,
return 1;
}
+ string_list_init_dup(&opt->priv->df_conflict_file_set);
code = unpack_trees_start(opt, merge_base, head, merge);
if (code != 0) {
@@ -3703,7 +3706,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
}
CALLOC_ARRAY(opt->priv, 1);
- string_list_init_dup(&opt->priv->df_conflict_file_set);
+ opt->priv->df_conflict_file_set.nr = 123456;
return 0;
}
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset
2021-10-04 15:20 ` Ævar Arnfjörð Bjarmason
@ 2021-10-04 16:28 ` Elijah Newren
0 siblings, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 16:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Mon, Oct 4, 2021 at 8:42 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Mon, Oct 04 2021, Elijah Newren wrote:
>
> > On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >>
> >> Change the clear_unpack_trees_porcelain() to be like a *_release()
> >> function, not a *_reset() (in strbuf.c terms). Let's move the only API
> >> user that relied on the latter to doing its own
> >> unpack_trees_options_init(). See the commit that introduced
> >> unpack_trees_options_init() for details on the control flow involved
> >> here.
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >> merge-recursive.c | 1 +
> >> unpack-trees.c | 1 -
> >> 2 files changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/merge-recursive.c b/merge-recursive.c
> >> index d24a4903f1d..a77f66b006c 100644
> >> --- a/merge-recursive.c
> >> +++ b/merge-recursive.c
> >> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
> >> {
> >> discard_index(&opt->priv->orig_index);
> >> clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
> >> + unpack_trees_options_init(&opt->priv->unpack_opts);
> >
> > This is wrong. It suggests that unpack_opts is used after
> > unpack_trees_finish() (other than an outer merge first calling
> > unpack_trees_start() again), which can only serve to greatly confuse
> > future readers. Drop this hunk.
>
> Sure, but (and also re:
> https://lore.kernel.org/git/CABPp-BEA2myh2Np_YpFWnE+jqmT5vz7ohigZ0=2tL-wizgYQmg@mail.gmail.com/)
> if you'd like not initialize things in merge_start() just for good
> measure wouldn't the diff-at-the-end on top of your 5bf7e5779ec
> (merge-recursive: split internal fields into a separate struct,
> 2019-08-17) also make sense?
Sorry, I can't parse this sentence. Could you retry?
> I.e. the reason I entered this particular rabbit hole was in looking at
> existing members of "struct merge_options_internal" & past commits and
> seeing how we did its initialization. That canary on top passes all our
> tests, and per my reading we also don't use "df_conflict_file_set" until
> as late as the things we setup in unpack_trees_start(). Should those be
> moved to do the post-merge_start() setup at the same time?
It appears df_conflict_file_set has some theoretical memory leaks
(though in practice unlikely and quite small in the few cases that
could be constructed to trigger it). Initializing it nearer to use
and free'ing when done (in merge_trees_internal()) would make more
sense, yes.
But, merge-recursive.c right now is supposed to be the stable fallback
in case someone runs into an issue with merge-ort. I'd rather keep it
stable in preparation for deleting it, not churning its code
unnecessarily.
> >> }
> >>
> >> static int save_files_dirs(const struct object_id *oid,
> >> diff --git a/unpack-trees.c b/unpack-trees.c
> >> index 94767d3f96f..e7365322e82 100644
> >> --- a/unpack-trees.c
> >> +++ b/unpack-trees.c
> >> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
> >> {
> >> strvec_clear(&opts->msgs_to_free);
> >> dir_clear(&opts->dir);
> >> - memset(opts->msgs, 0, sizeof(opts->msgs));
> >
> > This seems like a very dangerous change. You want to leave opts->msgs
> > pointing at freed memory?
>
> Yes, as argued in
> http://lore.kernel.org/git/87bl45niqs.fsf@evledraar.gmail.com; In this
> series we can see that nothing re-uses it, so it's as safe as our
> strbuf_release(), or a plain free().
strbuf_release() sets sb->buf to strbuf_slopbuf, and sets sb->len =
sb->alloc = 0. The strbuf can thus be reused after calling
strbuf_release().
strvec_clear() also calls strvec_init() afterwards to set the vector
to be usable though 0-sized.
hashmap_clear() also clears out existing data, but makes it ready for
reuse (as per 6da1a25814)
strmap_clear(), strintmap_clear(), and strset_clear() also set up the
data structure for reuse.
There's a longstanding presumption that something named `*_clear()`
will make it still usable afterwards. Rename it to end with `_free`
if you want it to be an analogy to free() where usage afterwards would
cause use-after-free errors.
> Maybe I'm misunderstanding what you're getting at, and I could
> understand a "let's just reset it for good measure" POV. But I can't
> square your view that we shouldn't do setup in merge_start() for good
> measure in case some new future code accidentally uses the data earlier
> (which I'm fine with), but then also not finding it OK to skip the
> memset() here ...
No existing caller needs to make use of the fact that it's a `_clear`
function rather than a `_free` function, but if you want to take
advantage of that to do less work, you should both call it out in your
commit message and rename the function. You didn't do either. In
fact, your existing commit message mentions strbuf_release(), which
reinforces the `_clear` presumption of reusability and thus makes me
flag the change as dangerous.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 05/10] dir.[ch]: make DIR_INIT mandatory
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-10-04 0:46 ` [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
2021-10-04 0:46 ` [PATCH 06/10] dir.c: get rid of lazy initialization Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
The dir_init() initializer has been documented as being mandatory
since eceba532141 (dir: fix problematic API to avoid memory leaks,
2020-08-18), but both it and my ce93a4c6127 (dir.[ch]: replace
dir_init() with DIR_INIT, 2021-07-01) managed to miss this callsite in
"add-interactive.c" added before those two commits in
ab1e1cccaf6 (built-in add -i: re-implement `add-untracked` in C,
2019-11-29).
In addition my change to remove dir_init() neglected to update this
documentation. Let's use "must be initialized with" in reference to
"DIR_INIT". We have one lazy initialization which pre-dates
eceba532141 in dir.c. Adjusting this callsite is a prerequisite for
removing it in favor of trusting the macro to initialize the "struct
dir_struct" correctly.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
add-interactive.c | 2 +-
dir.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/add-interactive.c b/add-interactive.c
index 6498ae196f1..27daea8d1b3 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -826,7 +826,7 @@ static int get_untracked_files(struct repository *r,
struct prefix_item_list *files,
const struct pathspec *ps)
{
- struct dir_struct dir = { 0 };
+ struct dir_struct dir = DIR_INIT;
size_t i;
struct strbuf buf = STRBUF_INIT;
diff --git a/dir.h b/dir.h
index 83f46c0fb4c..ff3b4a7f602 100644
--- a/dir.h
+++ b/dir.h
@@ -19,7 +19,7 @@
* CE_SKIP_WORKTREE marked. If you want to exclude files, make sure you have
* loaded the index first.
*
- * - Prepare `struct dir_struct dir` using `dir_init()` function.
+ * - The `struct dir_struct dir` must be initialized with `DIR_INIT`.
*
* - To add single exclude pattern, call `add_pattern_list()` and then
* `add_pattern()`.
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 05/10] dir.[ch]: make DIR_INIT mandatory
2021-10-04 0:46 ` [PATCH 05/10] dir.[ch]: make DIR_INIT mandatory Ævar Arnfjörð Bjarmason
@ 2021-10-04 13:45 ` Elijah Newren
0 siblings, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
The subject seems misleading; this patch doesn't make DIR_INIT
mandatory, it is just updating the docs to reflect that and updating
an out-of-date callsite.
>
> The dir_init() initializer has been documented as being mandatory
> since eceba532141 (dir: fix problematic API to avoid memory leaks,
> 2020-08-18), but both it and my ce93a4c6127 (dir.[ch]: replace
> dir_init() with DIR_INIT, 2021-07-01) managed to miss this callsite in
> "add-interactive.c" added before those two commits in
> ab1e1cccaf6 (built-in add -i: re-implement `add-untracked` in C,
> 2019-11-29).
>
> In addition my change to remove dir_init() neglected to update this
> documentation. Let's use "must be initialized with" in reference to
> "DIR_INIT". We have one lazy initialization which pre-dates
> eceba532141 in dir.c. Adjusting this callsite is a prerequisite for
> removing it in favor of trusting the macro to initialize the "struct
> dir_struct" correctly.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> add-interactive.c | 2 +-
> dir.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index 6498ae196f1..27daea8d1b3 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -826,7 +826,7 @@ static int get_untracked_files(struct repository *r,
> struct prefix_item_list *files,
> const struct pathspec *ps)
> {
> - struct dir_struct dir = { 0 };
> + struct dir_struct dir = DIR_INIT;
> size_t i;
> struct strbuf buf = STRBUF_INIT;
>
> diff --git a/dir.h b/dir.h
> index 83f46c0fb4c..ff3b4a7f602 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -19,7 +19,7 @@
> * CE_SKIP_WORKTREE marked. If you want to exclude files, make sure you have
> * loaded the index first.
> *
> - * - Prepare `struct dir_struct dir` using `dir_init()` function.
> + * - The `struct dir_struct dir` must be initialized with `DIR_INIT`.
> *
> * - To add single exclude pattern, call `add_pattern_list()` and then
> * `add_pattern()`.
> --
> 2.33.0.1404.g83021034c5d
...other than the subject, the patch looks good
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 06/10] dir.c: get rid of lazy initialization
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-10-04 0:46 ` [PATCH 05/10] dir.[ch]: make DIR_INIT mandatory Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
2021-10-04 0:46 ` [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain() Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
Remove the "Lazy initialization" in prep_exclude() left behind by
aceb9429b37 (prep_exclude: remove the artificial PATH_MAX limit,
2014-07-14).
Now that every caller who sets up a "struct dir_struct" is using the
DIR_INIT macro we can rely on it to have done the initialization. As
noted in an analysis of the previous control flow[1] an earlier
passing of of "dir->basebuf.buf" to strncmp() wasn't buggy, as we'd
only reach that code on subsequent invocations of prep_exclude(),
i.e. after this strbuf_init() had been run. But keeping track of that
makes for hard-to-read code. Let's just rely on the initialization
instead.
This does change the behavior of this code in that it won't be
pre-growing the strbuf to a size of PATH_MAX. I think that's OK.
That we were using PATH_MAX at all is just a relic from this being a
fixed buffer from way back in f87f9497486 (git-ls-files: --exclude
mechanism updates., 2005-07-24).
Pre-allocating PATH_MAX was the opposite of an optimization in this
case. I logged all "basebuf.buf" values when running the test suite,
and by far the most common one (around 80%) is "", which we now won't
allocate at all for, and just use the "strbuf_slopbuf".
The second most common one was "a/", followed by other common cases of
short relative paths. So using the default "struct strbuf" growth
pattern is a much better allocation optimization in this case.
1. https://lore.kernel.org/git/87sfxhohsj.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
dir.c | 8 --------
dir.h | 4 +++-
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/dir.c b/dir.c
index 39fce3bcba7..efc87c2e405 100644
--- a/dir.c
+++ b/dir.c
@@ -1550,14 +1550,6 @@ static void prep_exclude(struct dir_struct *dir,
if (dir->pattern)
return;
- /*
- * Lazy initialization. All call sites currently just
- * memset(dir, 0, sizeof(*dir)) before use. Changing all of
- * them seems lots of work for little benefit.
- */
- if (!dir->basebuf.buf)
- strbuf_init(&dir->basebuf, PATH_MAX);
-
/* Read from the parent directories and push them down. */
current = stk ? stk->baselen : -1;
strbuf_setlen(&dir->basebuf, current < 0 ? 0 : current);
diff --git a/dir.h b/dir.h
index ff3b4a7f602..e3757c6099e 100644
--- a/dir.h
+++ b/dir.h
@@ -342,7 +342,9 @@ struct dir_struct {
unsigned visited_directories;
};
-#define DIR_INIT { 0 }
+#define DIR_INIT { \
+ .basebuf = STRBUF_INIT, \
+}
struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp);
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 06/10] dir.c: get rid of lazy initialization
2021-10-04 0:46 ` [PATCH 06/10] dir.c: get rid of lazy initialization Ævar Arnfjörð Bjarmason
@ 2021-10-04 13:45 ` Elijah Newren
0 siblings, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Remove the "Lazy initialization" in prep_exclude() left behind by
> aceb9429b37 (prep_exclude: remove the artificial PATH_MAX limit,
> 2014-07-14).
>
> Now that every caller who sets up a "struct dir_struct" is using the
> DIR_INIT macro we can rely on it to have done the initialization. As
> noted in an analysis of the previous control flow[1] an earlier
> passing of of "dir->basebuf.buf" to strncmp() wasn't buggy, as we'd
> only reach that code on subsequent invocations of prep_exclude(),
> i.e. after this strbuf_init() had been run. But keeping track of that
> makes for hard-to-read code. Let's just rely on the initialization
> instead.
Having read through the link previously, this all makes sense to me,
but I'm not sure if this paragraph motivates the change without that
context. Maybe another reader can comment.
> This does change the behavior of this code in that it won't be
> pre-growing the strbuf to a size of PATH_MAX. I think that's OK.
>
> That we were using PATH_MAX at all is just a relic from this being a
> fixed buffer from way back in f87f9497486 (git-ls-files: --exclude
> mechanism updates., 2005-07-24).
>
> Pre-allocating PATH_MAX was the opposite of an optimization in this
> case. I logged all "basebuf.buf" values when running the test suite,
> and by far the most common one (around 80%) is "", which we now won't
> allocate at all for, and just use the "strbuf_slopbuf".
>
> The second most common one was "a/", followed by other common cases of
> short relative paths. So using the default "struct strbuf" growth
> pattern is a much better allocation optimization in this case.
>
> 1. https://lore.kernel.org/git/87sfxhohsj.fsf@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> dir.c | 8 --------
> dir.h | 4 +++-
> 2 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 39fce3bcba7..efc87c2e405 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1550,14 +1550,6 @@ static void prep_exclude(struct dir_struct *dir,
> if (dir->pattern)
> return;
>
> - /*
> - * Lazy initialization. All call sites currently just
> - * memset(dir, 0, sizeof(*dir)) before use. Changing all of
> - * them seems lots of work for little benefit.
> - */
> - if (!dir->basebuf.buf)
> - strbuf_init(&dir->basebuf, PATH_MAX);
> -
> /* Read from the parent directories and push them down. */
> current = stk ? stk->baselen : -1;
> strbuf_setlen(&dir->basebuf, current < 0 ? 0 : current);
> diff --git a/dir.h b/dir.h
> index ff3b4a7f602..e3757c6099e 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -342,7 +342,9 @@ struct dir_struct {
> unsigned visited_directories;
> };
>
> -#define DIR_INIT { 0 }
> +#define DIR_INIT { \
> + .basebuf = STRBUF_INIT, \
> +}
>
> struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp);
>
> --
> 2.33.0.1404.g83021034c5d
Wahoo! Nice code cleanup.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-10-04 0:46 ` [PATCH 06/10] dir.c: get rid of lazy initialization Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 9:38 ` Phillip Wood
2021-10-04 13:45 ` Elijah Newren
2021-10-04 0:46 ` [PATCH 08/10] unpack-trees: don't leak memory in verify_clean_subdirectory() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
Since a preceding commit we've been using
clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
a function that corresponds to setup_unpack_trees_porcelain(), as it
was when it was added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21).
Instead it's similar to strbuf_release() and other similar generic
"free" functions. Let's rename it to avoid any future confusion on the
topic.
Let's also update the API documentation for it to note this, and to
cover e.g. the code added around update_sparsity() in
4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
messages, 2020-03-27).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/checkout.c | 2 +-
builtin/sparse-checkout.c | 2 +-
merge-ort.c | 2 +-
merge-recursive.c | 2 +-
merge.c | 4 ++--
reset.c | 2 +-
unpack-trees.c | 2 +-
unpack-trees.h | 8 +++++---
8 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index d4b88affba7..482d17676a0 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -757,7 +757,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);
+ unpack_trees_options_release(&topts);
if (ret == -1) {
/*
* Unpack couldn't do a trivial merge; either
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 4c3c29fb580..b1221fd01d3 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -219,7 +219,7 @@ static int update_working_directory(struct pattern_list *pl)
setup_unpack_trees_porcelain(&o, "sparse-checkout");
result = update_sparsity(&o);
- clear_unpack_trees_porcelain(&o);
+ unpack_trees_options_release(&o);
if (result == UPDATE_SPARSITY_WARNINGS)
/*
diff --git a/merge-ort.c b/merge-ort.c
index e526b78b88d..0a5937364c9 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4054,7 +4054,7 @@ static int checkout(struct merge_options *opt,
init_tree_desc(&trees[1], next->buffer, next->size);
ret = unpack_trees(2, trees, &unpack_opts);
- clear_unpack_trees_porcelain(&unpack_opts);
+ unpack_trees_options_release(&unpack_opts);
return ret;
}
diff --git a/merge-recursive.c b/merge-recursive.c
index a77f66b006c..316cb2ca907 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -441,7 +441,7 @@ static int unpack_trees_start(struct merge_options *opt,
static void unpack_trees_finish(struct merge_options *opt)
{
discard_index(&opt->priv->orig_index);
- clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
+ unpack_trees_options_release(&opt->priv->unpack_opts);
unpack_trees_options_init(&opt->priv->unpack_opts);
}
diff --git a/merge.c b/merge.c
index 9cb32990dd9..2f618425aff 100644
--- a/merge.c
+++ b/merge.c
@@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,
if (unpack_trees(nr_trees, t, &opts)) {
rollback_lock_file(&lock_file);
- clear_unpack_trees_porcelain(&opts);
+ unpack_trees_options_release(&opts);
return -1;
}
- clear_unpack_trees_porcelain(&opts);
+ unpack_trees_options_release(&opts);
if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
return error(_("unable to write new index file"));
diff --git a/reset.c b/reset.c
index d13984ab781..f4bf3fbfac0 100644
--- a/reset.c
+++ b/reset.c
@@ -133,7 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
leave_reset_head:
strbuf_release(&msg);
rollback_lock_file(&lock);
- clear_unpack_trees_porcelain(&unpack_tree_opts);
+ unpack_trees_options_release(&unpack_tree_opts);
while (nr)
free((void *)desc[--nr].buffer);
return ret;
diff --git a/unpack-trees.c b/unpack-trees.c
index e7365322e82..bea598c9ece 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -193,7 +193,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)
memcpy(o, &blank, sizeof(*o));
}
-void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
+void unpack_trees_options_release(struct unpack_trees_options *opts)
{
strvec_clear(&opts->msgs_to_free);
dir_clear(&opts->dir);
diff --git a/unpack-trees.h b/unpack-trees.h
index 40c4841748d..a8d1f083b33 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -41,10 +41,12 @@ 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().
+/**
+ * Frees resources allocated by function that take the "struct
+ * unpack_trees_options". Always call this after using unpack_trees(),
+ * update_sparsity() etc.
*/
-void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
+void unpack_trees_options_release(struct unpack_trees_options *opts);
struct unpack_trees_options {
unsigned int reset,
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()
2021-10-04 0:46 ` [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain() Ævar Arnfjörð Bjarmason
@ 2021-10-04 9:38 ` Phillip Wood
2021-10-04 11:10 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
1 sibling, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2021-10-04 9:38 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
Hi Ævar
On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
> Since a preceding commit we've been using
> clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
> a function that corresponds to setup_unpack_trees_porcelain(), as it
> was when it was added in 1c41d2805e4 (unpack_trees_options: free
> messages when done, 2018-05-21).
>
> Instead it's similar to strbuf_release() and other similar generic
> "free" functions. Let's rename it to avoid any future confusion on the
> topic.
>
> Let's also update the API documentation for it to note this, and to
> cover e.g. the code added around update_sparsity() in
> 4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
> messages, 2020-03-27).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/checkout.c | 2 +-
> builtin/sparse-checkout.c | 2 +-
> merge-ort.c | 2 +-
> merge-recursive.c | 2 +-
> merge.c | 4 ++--
> reset.c | 2 +-
I was expecting to see sequencer.c here, but it appears it does not call
clear_unpack_trees_porcelain() to free the memory allocated with
setup_unpack_trees_porcelain(). If you're interested in fixing memory
leaks from unpack trees it might be worth checking that all the calls to
setup_unpack_trees_porcelain() have a matching call to
clear_unpack_trees_porcelain().
Best Wishes
Phillip
> unpack-trees.c | 2 +-
> unpack-trees.h | 8 +++++---
> 8 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index d4b88affba7..482d17676a0 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -757,7 +757,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);
> + unpack_trees_options_release(&topts);
> if (ret == -1) {
> /*
> * Unpack couldn't do a trivial merge; either
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 4c3c29fb580..b1221fd01d3 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -219,7 +219,7 @@ static int update_working_directory(struct pattern_list *pl)
>
> setup_unpack_trees_porcelain(&o, "sparse-checkout");
> result = update_sparsity(&o);
> - clear_unpack_trees_porcelain(&o);
> + unpack_trees_options_release(&o);
>
> if (result == UPDATE_SPARSITY_WARNINGS)
> /*
> diff --git a/merge-ort.c b/merge-ort.c
> index e526b78b88d..0a5937364c9 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4054,7 +4054,7 @@ static int checkout(struct merge_options *opt,
> init_tree_desc(&trees[1], next->buffer, next->size);
>
> ret = unpack_trees(2, trees, &unpack_opts);
> - clear_unpack_trees_porcelain(&unpack_opts);
> + unpack_trees_options_release(&unpack_opts);
> return ret;
> }
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index a77f66b006c..316cb2ca907 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -441,7 +441,7 @@ static int unpack_trees_start(struct merge_options *opt,
> static void unpack_trees_finish(struct merge_options *opt)
> {
> discard_index(&opt->priv->orig_index);
> - clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
> + unpack_trees_options_release(&opt->priv->unpack_opts);
> unpack_trees_options_init(&opt->priv->unpack_opts);
> }
>
> diff --git a/merge.c b/merge.c
> index 9cb32990dd9..2f618425aff 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,
>
> if (unpack_trees(nr_trees, t, &opts)) {
> rollback_lock_file(&lock_file);
> - clear_unpack_trees_porcelain(&opts);
> + unpack_trees_options_release(&opts);
> return -1;
> }
> - clear_unpack_trees_porcelain(&opts);
> + unpack_trees_options_release(&opts);
>
> if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
> return error(_("unable to write new index file"));
> diff --git a/reset.c b/reset.c
> index d13984ab781..f4bf3fbfac0 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -133,7 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
> leave_reset_head:
> strbuf_release(&msg);
> rollback_lock_file(&lock);
> - clear_unpack_trees_porcelain(&unpack_tree_opts);
> + unpack_trees_options_release(&unpack_tree_opts);
> while (nr)
> free((void *)desc[--nr].buffer);
> return ret;
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e7365322e82..bea598c9ece 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -193,7 +193,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)
> memcpy(o, &blank, sizeof(*o));
> }
>
> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
> +void unpack_trees_options_release(struct unpack_trees_options *opts)
> {
> strvec_clear(&opts->msgs_to_free);
> dir_clear(&opts->dir);
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 40c4841748d..a8d1f083b33 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -41,10 +41,12 @@ 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().
> +/**
> + * Frees resources allocated by function that take the "struct
> + * unpack_trees_options". Always call this after using unpack_trees(),
> + * update_sparsity() etc.
> */
> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
> +void unpack_trees_options_release(struct unpack_trees_options *opts);
>
> struct unpack_trees_options {
> unsigned int reset,
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()
2021-10-04 9:38 ` Phillip Wood
@ 2021-10-04 11:10 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 11:10 UTC (permalink / raw)
To: phillip.wood
Cc: git, Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Mon, Oct 04 2021, Phillip Wood wrote:
> Hi Ævar
>
> On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
>> Since a preceding commit we've been using
>> clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
>> a function that corresponds to setup_unpack_trees_porcelain(), as it
>> was when it was added in 1c41d2805e4 (unpack_trees_options: free
>> messages when done, 2018-05-21).
>> Instead it's similar to strbuf_release() and other similar generic
>> "free" functions. Let's rename it to avoid any future confusion on the
>> topic.
>> Let's also update the API documentation for it to note this, and to
>> cover e.g. the code added around update_sparsity() in
>> 4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
>> messages, 2020-03-27).
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> builtin/checkout.c | 2 +-
>> builtin/sparse-checkout.c | 2 +-
>> merge-ort.c | 2 +-
>> merge-recursive.c | 2 +-
>> merge.c | 4 ++--
>> reset.c | 2 +-
>
> I was expecting to see sequencer.c here, but it appears it does not
> call clear_unpack_trees_porcelain() to free the memory allocated with
> setup_unpack_trees_porcelain(). If you're interested in fixing memory
> leaks from unpack trees it might be worth checking that all the calls
> to setup_unpack_trees_porcelain() have a matching call to
> clear_unpack_trees_porcelain().
Well spotted, I thought I was doing that, but see that I missed this one
special-case, will fix.
With this series grepping for "UNPACK_TREES_OPTIONS_INIT" is better (and
knowing about the one special-case of merge-recursive.c).
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain()
2021-10-04 0:46 ` [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain() Ævar Arnfjörð Bjarmason
2021-10-04 9:38 ` Phillip Wood
@ 2021-10-04 13:45 ` Elijah Newren
1 sibling, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Since a preceding commit we've been using
> clear_unpack_trees_porcelain() to call dir_clear(). So it's no longer
> a function that corresponds to setup_unpack_trees_porcelain(), as it
> was when it was added in 1c41d2805e4 (unpack_trees_options: free
> messages when done, 2018-05-21).
>
> Instead it's similar to strbuf_release() and other similar generic
> "free" functions. Let's rename it to avoid any future confusion on the
> topic.
>
> Let's also update the API documentation for it to note this, and to
> cover e.g. the code added around update_sparsity() in
> 4ee5d50fc39 (sparse-checkout: use improved unpack_trees porcelain
> messages, 2020-03-27).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/checkout.c | 2 +-
> builtin/sparse-checkout.c | 2 +-
> merge-ort.c | 2 +-
> merge-recursive.c | 2 +-
> merge.c | 4 ++--
> reset.c | 2 +-
> unpack-trees.c | 2 +-
> unpack-trees.h | 8 +++++---
> 8 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index d4b88affba7..482d17676a0 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -757,7 +757,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);
> + unpack_trees_options_release(&topts);
> if (ret == -1) {
> /*
> * Unpack couldn't do a trivial merge; either
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 4c3c29fb580..b1221fd01d3 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -219,7 +219,7 @@ static int update_working_directory(struct pattern_list *pl)
>
> setup_unpack_trees_porcelain(&o, "sparse-checkout");
> result = update_sparsity(&o);
> - clear_unpack_trees_porcelain(&o);
> + unpack_trees_options_release(&o);
>
> if (result == UPDATE_SPARSITY_WARNINGS)
> /*
> diff --git a/merge-ort.c b/merge-ort.c
> index e526b78b88d..0a5937364c9 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4054,7 +4054,7 @@ static int checkout(struct merge_options *opt,
> init_tree_desc(&trees[1], next->buffer, next->size);
>
> ret = unpack_trees(2, trees, &unpack_opts);
> - clear_unpack_trees_porcelain(&unpack_opts);
> + unpack_trees_options_release(&unpack_opts);
> return ret;
> }
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index a77f66b006c..316cb2ca907 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -441,7 +441,7 @@ static int unpack_trees_start(struct merge_options *opt,
> static void unpack_trees_finish(struct merge_options *opt)
> {
> discard_index(&opt->priv->orig_index);
> - clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
> + unpack_trees_options_release(&opt->priv->unpack_opts);
> unpack_trees_options_init(&opt->priv->unpack_opts);
> }
>
> diff --git a/merge.c b/merge.c
> index 9cb32990dd9..2f618425aff 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -95,10 +95,10 @@ int checkout_fast_forward(struct repository *r,
>
> if (unpack_trees(nr_trees, t, &opts)) {
> rollback_lock_file(&lock_file);
> - clear_unpack_trees_porcelain(&opts);
> + unpack_trees_options_release(&opts);
> return -1;
> }
> - clear_unpack_trees_porcelain(&opts);
> + unpack_trees_options_release(&opts);
>
> if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
> return error(_("unable to write new index file"));
> diff --git a/reset.c b/reset.c
> index d13984ab781..f4bf3fbfac0 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -133,7 +133,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
> leave_reset_head:
> strbuf_release(&msg);
> rollback_lock_file(&lock);
> - clear_unpack_trees_porcelain(&unpack_tree_opts);
> + unpack_trees_options_release(&unpack_tree_opts);
> while (nr)
> free((void *)desc[--nr].buffer);
> return ret;
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e7365322e82..bea598c9ece 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -193,7 +193,7 @@ void unpack_trees_options_init(struct unpack_trees_options *o)
> memcpy(o, &blank, sizeof(*o));
> }
>
> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
> +void unpack_trees_options_release(struct unpack_trees_options *opts)
> {
> strvec_clear(&opts->msgs_to_free);
> dir_clear(&opts->dir);
> diff --git a/unpack-trees.h b/unpack-trees.h
> index 40c4841748d..a8d1f083b33 100644
> --- a/unpack-trees.h
> +++ b/unpack-trees.h
> @@ -41,10 +41,12 @@ 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().
> +/**
> + * Frees resources allocated by function that take the "struct
> + * unpack_trees_options". Always call this after using unpack_trees(),
> + * update_sparsity() etc.
> */
> -void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
> +void unpack_trees_options_release(struct unpack_trees_options *opts);
>
> struct unpack_trees_options {
> unsigned int reset,
> --
> 2.33.0.1404.g83021034c5d
Makes sense.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 08/10] unpack-trees: don't leak memory in verify_clean_subdirectory()
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-10-04 0:46 ` [PATCH 07/10] unpack-trees API: rename clear_unpack_trees_porcelain() Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
2021-10-04 0:46 ` [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
Fix two different but related memory leaks in
verify_clean_subdirectory(). We leaked both the "pathbuf" if
read_directory() returned non-zero, and we never cleaned up our own
"struct dir_struct" either.
* "pathbuf": When the read_directory() call followed by the
free(pathbuf) was added in c81935348be (Fix switching to a branch
with D/F when current branch has file D., 2007-03-15) we didn't
bother to free() before we called die().
But when this code was later libified in 203a2fe1170 (Allow callers
of unpack_trees() to handle failure, 2008-02-07) we started to leak
as we returned data to the caller. This fixes that memory leak,
which can be observed under SANITIZE=leak with e.g. the
"t1001-read-tree-m-2way.sh" test.
* "struct dir_struct": We've leaked the dir_struct ever since this
code was added back in c81935348be.
When that commit was written there wasn't an equivalent of
dir_clear(). Since it was added in 270be816049 (dir.c: provide
clear_directory() for reclaiming dir_struct memory, 2013-01-06)
we've omitted freeing the memory allocated here.
This memory leak could also be observed under SANITIZE=leak and the
"t1001-read-tree-m-2way.sh" test.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
unpack-trees.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index bea598c9ece..260e7ec5bb4 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2138,9 +2138,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
d.exclude_per_dir = o->dir.exclude_per_dir;
i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
+ dir_clear(&d);
+ free(pathbuf);
if (i)
return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
- free(pathbuf);
return cnt;
}
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 08/10] unpack-trees: don't leak memory in verify_clean_subdirectory()
2021-10-04 0:46 ` [PATCH 08/10] unpack-trees: don't leak memory in verify_clean_subdirectory() Ævar Arnfjörð Bjarmason
@ 2021-10-04 13:45 ` Elijah Newren
0 siblings, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Fix two different but related memory leaks in
> verify_clean_subdirectory(). We leaked both the "pathbuf" if
> read_directory() returned non-zero, and we never cleaned up our own
> "struct dir_struct" either.
>
> * "pathbuf": When the read_directory() call followed by the
> free(pathbuf) was added in c81935348be (Fix switching to a branch
> with D/F when current branch has file D., 2007-03-15) we didn't
> bother to free() before we called die().
>
> But when this code was later libified in 203a2fe1170 (Allow callers
> of unpack_trees() to handle failure, 2008-02-07) we started to leak
> as we returned data to the caller. This fixes that memory leak,
> which can be observed under SANITIZE=leak with e.g. the
> "t1001-read-tree-m-2way.sh" test.
>
> * "struct dir_struct": We've leaked the dir_struct ever since this
> code was added back in c81935348be.
>
> When that commit was written there wasn't an equivalent of
> dir_clear(). Since it was added in 270be816049 (dir.c: provide
> clear_directory() for reclaiming dir_struct memory, 2013-01-06)
> we've omitted freeing the memory allocated here.
>
> This memory leak could also be observed under SANITIZE=leak and the
> "t1001-read-tree-m-2way.sh" test.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> unpack-trees.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index bea598c9ece..260e7ec5bb4 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2138,9 +2138,10 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
>
> d.exclude_per_dir = o->dir.exclude_per_dir;
> i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
> + dir_clear(&d);
> + free(pathbuf);
> if (i)
> return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
> - free(pathbuf);
> return cnt;
> }
>
> --
> 2.33.0.1404.g83021034c5d
Good catches, and nicely explained.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-10-04 0:46 ` [PATCH 08/10] unpack-trees: don't leak memory in verify_clean_subdirectory() Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
2021-10-04 0:46 ` [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release() Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Elijah Newren
10 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
Refactor code added in 1c41d2805e4 (unpack_trees_options: free
messages when done, 2018-05-21) to use a ret/goto pattern, rather than
duplicating the end cleanup in the function.
This control flow will be matched in subsequent commits by various
other similar code, which often needs to do more than just call
unpack_trees_options_release(). Let's change this to consistently use
the same pattern everywhere.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
merge.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/merge.c b/merge.c
index 2f618425aff..2e3714ccaa0 100644
--- a/merge.c
+++ b/merge.c
@@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,
struct tree_desc t[MAX_UNPACK_TREES];
int i, nr_trees = 0;
struct lock_file lock_file = LOCK_INIT;
+ int ret = 0;
refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
@@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,
if (unpack_trees(nr_trees, t, &opts)) {
rollback_lock_file(&lock_file);
- unpack_trees_options_release(&opts);
- return -1;
+ ret = -1;
+ goto cleanup;
}
- unpack_trees_options_release(&opts);
if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
- return error(_("unable to write new index file"));
- return 0;
+ ret = error(_("unable to write new index file"));
+
+cleanup:
+ unpack_trees_options_release(&opts);
+ return ret;
}
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code
2021-10-04 0:46 ` [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code Ævar Arnfjörð Bjarmason
@ 2021-10-04 13:45 ` Elijah Newren
2021-10-04 14:50 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Refactor code added in 1c41d2805e4 (unpack_trees_options: free
> messages when done, 2018-05-21) to use a ret/goto pattern, rather than
> duplicating the end cleanup in the function.
>
> This control flow will be matched in subsequent commits by various
> other similar code, which often needs to do more than just call
> unpack_trees_options_release(). Let's change this to consistently use
> the same pattern everywhere.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> merge.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/merge.c b/merge.c
> index 2f618425aff..2e3714ccaa0 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,
> struct tree_desc t[MAX_UNPACK_TREES];
> int i, nr_trees = 0;
> struct lock_file lock_file = LOCK_INIT;
> + int ret = 0;
>
> refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
>
> @@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,
>
> if (unpack_trees(nr_trees, t, &opts)) {
> rollback_lock_file(&lock_file);
> - unpack_trees_options_release(&opts);
> - return -1;
> + ret = -1;
> + goto cleanup;
> }
> - unpack_trees_options_release(&opts);
>
> if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
> - return error(_("unable to write new index file"));
> - return 0;
> + ret = error(_("unable to write new index file"));
> +
> +cleanup:
> + unpack_trees_options_release(&opts);
> + return ret;
> }
> --
> 2.33.0.1404.g83021034c5d
I would say 'meh'...but the commit message is downright confusing. It
suggests that subsequent commits, plural, will be modifying this code
further. There is only one more commit in this series, and looking
ahead, it doesn't even touch this file. So, there actually isn't any
reason for these changes beyond what we see in this file, at least not
for this series. And as far as this series is concerned, I think it's
actually less readable. If there's a subsequent series that will use
this and make further changes I could be convinced, but then I'd say
this commit belongs as part of the later series, not this one.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code
2021-10-04 13:45 ` Elijah Newren
@ 2021-10-04 14:50 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 14:50 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Mon, Oct 04 2021, Elijah Newren wrote:
> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> Refactor code added in 1c41d2805e4 (unpack_trees_options: free
>> messages when done, 2018-05-21) to use a ret/goto pattern, rather than
>> duplicating the end cleanup in the function.
>>
>> This control flow will be matched in subsequent commits by various
>> other similar code, which often needs to do more than just call
>> unpack_trees_options_release(). Let's change this to consistently use
>> the same pattern everywhere.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> merge.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/merge.c b/merge.c
>> index 2f618425aff..2e3714ccaa0 100644
>> --- a/merge.c
>> +++ b/merge.c
>> @@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r,
>> struct tree_desc t[MAX_UNPACK_TREES];
>> int i, nr_trees = 0;
>> struct lock_file lock_file = LOCK_INIT;
>> + int ret = 0;
>>
>> refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
>>
>> @@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r,
>>
>> if (unpack_trees(nr_trees, t, &opts)) {
>> rollback_lock_file(&lock_file);
>> - unpack_trees_options_release(&opts);
>> - return -1;
>> + ret = -1;
>> + goto cleanup;
>> }
>> - unpack_trees_options_release(&opts);
>>
>> if (write_locked_index(r->index, &lock_file, COMMIT_LOCK))
>> - return error(_("unable to write new index file"));
>> - return 0;
>> + ret = error(_("unable to write new index file"));
>> +
>> +cleanup:
>> + unpack_trees_options_release(&opts);
>> + return ret;
>> }
>> --
>> 2.33.0.1404.g83021034c5d
>
> I would say 'meh'...but the commit message is downright confusing. It
> suggests that subsequent commits, plural, will be modifying this code
> further. There is only one more commit in this series, and looking
> ahead, it doesn't even touch this file. So, there actually isn't any
> reason for these changes beyond what we see in this file, at least not
> for this series. And as far as this series is concerned, I think it's
> actually less readable. If there's a subsequent series that will use
> this and make further changes I could be convinced, but then I'd say
> this commit belongs as part of the later series, not this one.
Will fix, initially had these built-in fixes split into one commit per
built-in, but decided it was too verbose and squashed them all, and
didn't copyedit the commit message properly.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release()
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-10-04 0:46 ` [PATCH 09/10] merge.c: avoid duplicate unpack_trees_options_release() code Ævar Arnfjörð Bjarmason
@ 2021-10-04 0:46 ` Ævar Arnfjörð Bjarmason
2021-10-04 13:45 ` Elijah Newren
2021-10-04 13:45 ` [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Elijah Newren
10 siblings, 1 reply; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 0:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King, Ævar Arnfjörð Bjarmason
Plug memory leaks in various built-ins that were missing
unpack_trees_options_release() calls. In an earlier commit these
functions were all made to use the "UNPACK_TREES_OPTIONS_INIT" macro,
now let's have the ones that didn't clean up their memory do so.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
archive.c | 9 +++++++--
builtin/am.c | 17 ++++++++++++-----
builtin/checkout.c | 9 +++++++--
builtin/clone.c | 1 +
builtin/commit.c | 6 +++++-
builtin/merge.c | 6 ++++--
builtin/read-tree.c | 14 ++++++++++----
builtin/reset.c | 13 +++++++++----
builtin/stash.c | 14 ++++++++++----
diff-lib.c | 5 ++++-
10 files changed, 69 insertions(+), 25 deletions(-)
diff --git a/archive.c b/archive.c
index 210d7235c5a..003db7d355d 100644
--- a/archive.c
+++ b/archive.c
@@ -306,8 +306,10 @@ int write_archive_entries(struct archiver_args *args,
opts.dst_index = args->repo->index;
opts.fn = oneway_merge;
init_tree_desc(&t, args->tree->buffer, args->tree->size);
- if (unpack_trees(1, &t, &opts))
- return -1;
+ if (unpack_trees(1, &t, &opts)) {
+ err = -1;
+ goto cleanup;
+ }
git_attr_set_direction(GIT_ATTR_INDEX);
}
@@ -346,8 +348,11 @@ int write_archive_entries(struct archiver_args *args,
if (err)
break;
}
+
+cleanup:
strbuf_release(&path_in_archive);
strbuf_release(&content);
+ unpack_trees_options_release(&opts);
return err;
}
diff --git a/builtin/am.c b/builtin/am.c
index 82641ce68ec..4d4bb473c0f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
struct lock_file lock_file = LOCK_INIT;
struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t[2];
+ int ret = 0;
if (parse_tree(head) || parse_tree(remote))
return -1;
@@ -1923,13 +1924,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
if (unpack_trees(2, t, &opts)) {
rollback_lock_file(&lock_file);
- return -1;
+ ret = -1;
+ goto cleanup;
}
if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
die(_("unable to write new index file"));
-
- return 0;
+cleanup:
+ unpack_trees_options_release(&opts);
+ return ret;
}
/**
@@ -1941,6 +1944,7 @@ static int merge_tree(struct tree *tree)
struct lock_file lock_file = LOCK_INIT;
struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t[1];
+ int ret = 0;
if (parse_tree(tree))
return -1;
@@ -1956,13 +1960,16 @@ static int merge_tree(struct tree *tree)
if (unpack_trees(1, t, &opts)) {
rollback_lock_file(&lock_file);
- return -1;
+ ret = -1;
+ goto cleanup;
}
if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
die(_("unable to write new index file"));
- return 0;
+cleanup:
+ unpack_trees_options_release(&opts);
+ return ret;
}
/**
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 482d17676a0..fd76b504861 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
{
struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc tree_desc;
+ int ret;
opts.head_idx = -1;
opts.update = worktree;
@@ -667,10 +668,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
*/
/* fallthrough */
case 0:
- return 0;
+ ret = 0;
+ break;
default:
- return 128;
+ ret = 128;
}
+
+ unpack_trees_options_release(&opts);
+ return ret;
}
static void setup_branch_path(struct branch_info *branch)
diff --git a/builtin/clone.c b/builtin/clone.c
index 0df820c5970..df3bb9a7884 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -697,6 +697,7 @@ static int checkout(int submodule_progress)
init_tree_desc(&t, tree->buffer, tree->size);
if (unpack_trees(1, &t, &opts) < 0)
die(_("unable to checkout working tree"));
+ unpack_trees_options_release(&opts);
free(head);
diff --git a/builtin/commit.c b/builtin/commit.c
index 6cc7313bad8..84c79ecb5a5 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)
struct tree *tree;
struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
struct tree_desc t;
+ int exit_early = 0;
if (!current_head) {
discard_cache();
@@ -324,7 +325,10 @@ static void create_base_index(const struct commit *current_head)
parse_tree(tree);
init_tree_desc(&t, tree->buffer, tree->size);
if (unpack_trees(1, &t, &opts))
- exit(128); /* We've already reported the error, finish dying */
+ exit_early = 1; /* We've already reported the error, finish dying */
+ unpack_trees_options_release(&opts);
+ if (exit_early)
+ exit(128);
}
static void refresh_cache_or_die(int refresh_flags)
diff --git a/builtin/merge.c b/builtin/merge.c
index 73290a07fcc..28089e2c5ed 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
struct tree *trees[MAX_UNPACK_TREES];
struct tree_desc t[MAX_UNPACK_TREES];
struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
+ int ret = 0;
opts.head_idx = 2;
opts.src_index = &the_index;
@@ -695,8 +696,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
if (unpack_trees(nr_trees, t, &opts))
- return -1;
- return 0;
+ ret = -1;
+ unpack_trees_options_release(&opts);
+ return ret;
}
static void write_tree_trivial(struct object_id *oid)
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 06f3b97ac05..8f1b8a7e74c 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -154,6 +154,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
OPT_END()
};
+ int ret = 0;
opts.head_idx = -1;
opts.src_index = &the_index;
@@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
parse_tree(tree);
init_tree_desc(t+i, tree->buffer, tree->size);
}
- if (unpack_trees(nr_trees, t, &opts))
- return 128;
+ if (unpack_trees(nr_trees, t, &opts)) {
+ ret = 128;
+ goto cleanup;
+ }
if (opts.debug_unpack || opts.dry_run)
- return 0; /* do not write the index out */
+ goto cleanup; /* do not write the index out */
/*
* When reading only one tree (either the most basic form,
@@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
die("unable to write new index file");
- return 0;
+
+cleanup:
+ unpack_trees_options_release(&opts);
+ return ret;
}
diff --git a/builtin/reset.c b/builtin/reset.c
index 86c604b21e9..713d084c3eb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -78,10 +78,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
if (reset_type == KEEP) {
struct object_id head_oid;
- if (get_oid("HEAD", &head_oid))
- return error(_("You do not have a valid HEAD."));
- if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))
- return error(_("Failed to find tree of HEAD."));
+ if (get_oid("HEAD", &head_oid)) {
+ error(_("You do not have a valid HEAD."));
+ goto out;
+ }
+ if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {
+ error(_("Failed to find tree of HEAD."));
+ goto out;
+ }
nr++;
opts.fn = twoway_merge;
}
@@ -103,6 +107,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
ret = 0;
out:
+ unpack_trees_options_release(&opts);
for (i = 0; i < nr; i++)
free((void *)desc[i].buffer);
return ret;
diff --git a/builtin/stash.c b/builtin/stash.c
index 1137e5fcbe8..be6ecb1ae11 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
struct tree_desc t[MAX_UNPACK_TREES];
struct tree *tree;
struct lock_file lock_file = LOCK_INIT;
+ int ret = 0;
read_cache_preload(NULL);
if (refresh_cache(REFRESH_QUIET))
@@ -258,13 +259,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
opts.update = update;
opts.fn = oneway_merge;
- if (unpack_trees(nr_trees, t, &opts))
- return -1;
+ if (unpack_trees(nr_trees, t, &opts)) {
+ ret = -1;
+ goto cleanup;
+ }
if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
- return error(_("unable to write new index file"));
+ ret = error(_("unable to write new index file"));
- return 0;
+cleanup:
+ unpack_trees_options_release(&opts);
+ return ret;
}
static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
@@ -815,6 +820,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
die(_("failed to unpack trees"));
+ unpack_trees_options_release(&unpack_tree_opt);
do_diff_cache(&info->b_commit, diff_opt);
}
diff --git a/diff-lib.c b/diff-lib.c
index 8a08d9af4eb..2d8a90a51b2 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,
struct tree *tree;
struct tree_desc t;
struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
+ int ret;
tree = parse_tree_indirect(tree_oid);
if (!tree)
@@ -545,7 +546,9 @@ static int diff_cache(struct rev_info *revs,
opts.pathspec->recursive = 1;
init_tree_desc(&t, tree->buffer, tree->size);
- return unpack_trees(1, &t, &opts);
+ ret = unpack_trees(1, &t, &opts);
+ unpack_trees_options_release(&opts);
+ return ret;
}
void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
--
2.33.0.1404.g83021034c5d
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release()
2021-10-04 0:46 ` [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release() Ævar Arnfjörð Bjarmason
@ 2021-10-04 13:45 ` Elijah Newren
2021-10-04 14:54 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Plug memory leaks in various built-ins that were missing
> unpack_trees_options_release() calls. In an earlier commit these
> functions were all made to use the "UNPACK_TREES_OPTIONS_INIT" macro,
> now let's have the ones that didn't clean up their memory do so.
This commit message doesn't say anything about what was leaking. You
should really bring up that it was the dir_clear() you added to
unpack_trees_options_release() earlier in the series that is important
here (or at least that's what I presume is the important fix).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> archive.c | 9 +++++++--
> builtin/am.c | 17 ++++++++++++-----
> builtin/checkout.c | 9 +++++++--
> builtin/clone.c | 1 +
> builtin/commit.c | 6 +++++-
> builtin/merge.c | 6 ++++--
> builtin/read-tree.c | 14 ++++++++++----
> builtin/reset.c | 13 +++++++++----
> builtin/stash.c | 14 ++++++++++----
> diff-lib.c | 5 ++++-
> 10 files changed, 69 insertions(+), 25 deletions(-)
>
> diff --git a/archive.c b/archive.c
> index 210d7235c5a..003db7d355d 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -306,8 +306,10 @@ int write_archive_entries(struct archiver_args *args,
> opts.dst_index = args->repo->index;
> opts.fn = oneway_merge;
> init_tree_desc(&t, args->tree->buffer, args->tree->size);
> - if (unpack_trees(1, &t, &opts))
> - return -1;
> + if (unpack_trees(1, &t, &opts)) {
> + err = -1;
> + goto cleanup;
> + }
> git_attr_set_direction(GIT_ATTR_INDEX);
> }
>
> @@ -346,8 +348,11 @@ int write_archive_entries(struct archiver_args *args,
> if (err)
> break;
> }
> +
> +cleanup:
> strbuf_release(&path_in_archive);
> strbuf_release(&content);
> + unpack_trees_options_release(&opts);
>
> return err;
> }
> diff --git a/builtin/am.c b/builtin/am.c
> index 82641ce68ec..4d4bb473c0f 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1903,6 +1903,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
> struct lock_file lock_file = LOCK_INIT;
> struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> struct tree_desc t[2];
> + int ret = 0;
>
> if (parse_tree(head) || parse_tree(remote))
> return -1;
> @@ -1923,13 +1924,15 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
>
> if (unpack_trees(2, t, &opts)) {
> rollback_lock_file(&lock_file);
> - return -1;
> + ret = -1;
> + goto cleanup;
> }
>
> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> die(_("unable to write new index file"));
> -
> - return 0;
> +cleanup:
> + unpack_trees_options_release(&opts);
> + return ret;
> }
>
> /**
> @@ -1941,6 +1944,7 @@ static int merge_tree(struct tree *tree)
> struct lock_file lock_file = LOCK_INIT;
> struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> struct tree_desc t[1];
> + int ret = 0;
>
> if (parse_tree(tree))
> return -1;
> @@ -1956,13 +1960,16 @@ static int merge_tree(struct tree *tree)
>
> if (unpack_trees(1, t, &opts)) {
> rollback_lock_file(&lock_file);
> - return -1;
> + ret = -1;
> + goto cleanup;
> }
>
> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> die(_("unable to write new index file"));
>
> - return 0;
> +cleanup:
> + unpack_trees_options_release(&opts);
> + return ret;
> }
>
> /**
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 482d17676a0..fd76b504861 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -641,6 +641,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
> {
> struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> struct tree_desc tree_desc;
> + int ret;
>
> opts.head_idx = -1;
> opts.update = worktree;
> @@ -667,10 +668,14 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
> */
> /* fallthrough */
> case 0:
> - return 0;
> + ret = 0;
> + break;
> default:
> - return 128;
> + ret = 128;
> }
> +
> + unpack_trees_options_release(&opts);
> + return ret;
> }
>
> static void setup_branch_path(struct branch_info *branch)
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0df820c5970..df3bb9a7884 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -697,6 +697,7 @@ static int checkout(int submodule_progress)
> init_tree_desc(&t, tree->buffer, tree->size);
> if (unpack_trees(1, &t, &opts) < 0)
> die(_("unable to checkout working tree"));
> + unpack_trees_options_release(&opts);
>
> free(head);
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6cc7313bad8..84c79ecb5a5 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -305,6 +305,7 @@ static void create_base_index(const struct commit *current_head)
> struct tree *tree;
> struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> struct tree_desc t;
> + int exit_early = 0;
>
> if (!current_head) {
> discard_cache();
> @@ -324,7 +325,10 @@ static void create_base_index(const struct commit *current_head)
> parse_tree(tree);
> init_tree_desc(&t, tree->buffer, tree->size);
> if (unpack_trees(1, &t, &opts))
> - exit(128); /* We've already reported the error, finish dying */
> + exit_early = 1; /* We've already reported the error, finish dying */
> + unpack_trees_options_release(&opts);
> + if (exit_early)
> + exit(128);
> }
>
> static void refresh_cache_or_die(int refresh_flags)
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 73290a07fcc..28089e2c5ed 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -671,6 +671,7 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
> struct tree *trees[MAX_UNPACK_TREES];
> struct tree_desc t[MAX_UNPACK_TREES];
> struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> + int ret = 0;
>
> opts.head_idx = 2;
> opts.src_index = &the_index;
> @@ -695,8 +696,9 @@ static int read_tree_trivial(struct object_id *common, struct object_id *head,
> init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
> }
> if (unpack_trees(nr_trees, t, &opts))
> - return -1;
> - return 0;
> + ret = -1;
> + unpack_trees_options_release(&opts);
> + return ret;
> }
>
> static void write_tree_trivial(struct object_id *oid)
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 06f3b97ac05..8f1b8a7e74c 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -154,6 +154,7 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
> OPT__QUIET(&opts.quiet, N_("suppress feedback messages")),
> OPT_END()
> };
> + int ret = 0;
>
> opts.head_idx = -1;
> opts.src_index = &the_index;
> @@ -243,11 +244,13 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
> parse_tree(tree);
> init_tree_desc(t+i, tree->buffer, tree->size);
> }
> - if (unpack_trees(nr_trees, t, &opts))
> - return 128;
> + if (unpack_trees(nr_trees, t, &opts)) {
> + ret = 128;
> + goto cleanup;
> + }
>
> if (opts.debug_unpack || opts.dry_run)
> - return 0; /* do not write the index out */
> + goto cleanup; /* do not write the index out */
>
> /*
> * When reading only one tree (either the most basic form,
> @@ -262,5 +265,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
>
> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> die("unable to write new index file");
> - return 0;
> +
> +cleanup:
> + unpack_trees_options_release(&opts);
> + return ret;
> }
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 86c604b21e9..713d084c3eb 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -78,10 +78,14 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
>
> if (reset_type == KEEP) {
> struct object_id head_oid;
> - if (get_oid("HEAD", &head_oid))
> - return error(_("You do not have a valid HEAD."));
> - if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid))
> - return error(_("Failed to find tree of HEAD."));
> + if (get_oid("HEAD", &head_oid)) {
> + error(_("You do not have a valid HEAD."));
> + goto out;
> + }
> + if (!fill_tree_descriptor(the_repository, desc + nr, &head_oid)) {
> + error(_("Failed to find tree of HEAD."));
> + goto out;
> + }
> nr++;
> opts.fn = twoway_merge;
> }
> @@ -103,6 +107,7 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
> ret = 0;
>
> out:
> + unpack_trees_options_release(&opts);
> for (i = 0; i < nr; i++)
> free((void *)desc[i].buffer);
> return ret;
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 1137e5fcbe8..be6ecb1ae11 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -237,6 +237,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
> struct tree_desc t[MAX_UNPACK_TREES];
> struct tree *tree;
> struct lock_file lock_file = LOCK_INIT;
> + int ret = 0;
>
> read_cache_preload(NULL);
> if (refresh_cache(REFRESH_QUIET))
> @@ -258,13 +259,17 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
> opts.update = update;
> opts.fn = oneway_merge;
>
> - if (unpack_trees(nr_trees, t, &opts))
> - return -1;
> + if (unpack_trees(nr_trees, t, &opts)) {
> + ret = -1;
> + goto cleanup;
> + }
>
> if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> - return error(_("unable to write new index file"));
> + ret = error(_("unable to write new index file"));
>
> - return 0;
> +cleanup:
> + unpack_trees_options_release(&opts);
> + return ret;
> }
>
> static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
> @@ -815,6 +820,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
>
> if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
> die(_("failed to unpack trees"));
> + unpack_trees_options_release(&unpack_tree_opt);
>
> do_diff_cache(&info->b_commit, diff_opt);
> }
> diff --git a/diff-lib.c b/diff-lib.c
> index 8a08d9af4eb..2d8a90a51b2 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -527,6 +527,7 @@ static int diff_cache(struct rev_info *revs,
> struct tree *tree;
> struct tree_desc t;
> struct unpack_trees_options opts = UNPACK_TREES_OPTIONS_INIT;
> + int ret;
>
> tree = parse_tree_indirect(tree_oid);
> if (!tree)
> @@ -545,7 +546,9 @@ static int diff_cache(struct rev_info *revs,
> opts.pathspec->recursive = 1;
>
> init_tree_desc(&t, tree->buffer, tree->size);
> - return unpack_trees(1, &t, &opts);
> + ret = unpack_trees(1, &t, &opts);
> + unpack_trees_options_release(&opts);
> + return ret;
> }
>
> void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
> --
> 2.33.0.1404.g83021034c5d
Looks good to me; thanks for catching and fixing these leaks.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release()
2021-10-04 13:45 ` Elijah Newren
@ 2021-10-04 14:54 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 31+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-04 14:54 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Mon, Oct 04 2021, Elijah Newren wrote:
> On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> Plug memory leaks in various built-ins that were missing
>> unpack_trees_options_release() calls. In an earlier commit these
>> functions were all made to use the "UNPACK_TREES_OPTIONS_INIT" macro,
>> now let's have the ones that didn't clean up their memory do so.
>
> This commit message doesn't say anything about what was leaking. You
> should really bring up that it was the dir_clear() you added to
> unpack_trees_options_release() earlier in the series that is important
> here (or at least that's what I presume is the important fix).
*nod*, willdo.
> Looks good to me; thanks for catching and fixing these leaks.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks
2021-10-04 0:46 [PATCH 00/10] unpack-trees & dir APIs: fix memory leaks Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-10-04 0:46 ` [PATCH 10/10] built-ins: plug memory leaks with unpack_trees_options_release() Ævar Arnfjörð Bjarmason
@ 2021-10-04 13:45 ` Elijah Newren
10 siblings, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2021-10-04 13:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git Mailing List, Junio C Hamano,
Nguyễn Thái Ngọc Duy, Martin Ågren,
Andrzej Hunt, Jeff King
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> This series fixes memory leaks in the unpack-trees and dir APIs for
> all their callers.
There are several good fixes in this series. Thanks for working on them!
> There's been a discussion between myself & Elijah
> on his en/removing-untracked-fixes series[1] about the memory leak
> fixing aspect of his series.
Not really, the memory leak fixing aspect of my series was patch 2;
most of our discussion was on patch 4, including your footnote link.
Patch 4 did not in any way involve fixing a memory leak, which you
yourself later acknowledged. So most of our discussion was mostly
about aspects _other_ than leak fixing.
> I've got locally queued patches that fix widespread memory leaks in
> the test suite and make much of it pass under SANITIZE=leak, once the
> common leaks in revisions.c (git rev-list/show/log etc.), "checkout",
> "dir" and "unpack-trees" are fixed a lot of tests become entirely
> leak-free, as much code that needs to setup various basic things will
> require one of those commands.
Yaay! This is great stuff!
> I think that the more narrow fixes[2] to the memory leaks around
> unpack-trees in Elijah's series[3] are better skipped and that series
> rebased on top of this one (I'll submit an RFC version of his that is
> rebased on this as a follow-up).
I *strongly* disagree.
> I.e. his solves a very small amount of the memory leaks in this area,
> whereas this is something I've got running as part of end-to-end
> SANITIZE=leak testing, so I think that the difference in approaches we
> picked when it comes to fixing them is likely because of that.
>
> E.g. continuing to allocate the "struct dir_struct" on the heap in his
> version[4] in his is, I think, something that makes more sense for
> fixes that haven't pulled at the thread of how much merge-recursive.c
> is making that question of ownerhip confusing. There's also changen in
> his that'll become simpler as the complexity of the underlying APIs
> has gone away, e.g. [6].
*Sigh*. unpack_trees_options->dir is not allocated on the heap at the
end of my series. I could understand missing that in the patches, but
I've also pointed it out to you two additional times in discussions on
the patches so far. And you supposedly looked at all the patches
again while rebasing and adding your signed-off-by.
You also continue to refer to our discussion as though it was about
leakfixes, even though the patch we discussed in my series did not
involve any leak fixing. I pointed that out and you said you stood
corrected (last comment at
https://lore.kernel.org/git/87k0ivpzfx.fsf@evledraar.gmail.com/), but
now you're referring to it that way again? Even after rebasing my
series and adding your Signed-off-by, suggesting you should understand
it? The leakfix was a different patch of the series, namely patch #2.
I agree that merge-recursive.c has confusing points. I totally agree.
Unfortunately, both your patches that touch merge-recursive.c make it
worse; more so than the problems you were trying to fix in that file.
> 1. https://lore.kernel.org/git/87ilyjviiy.fsf@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/0c74285b25311c83bb158cf89a551160a0f3a5d3.1632760428.git.gitgitgadget@gmail.com/
> 3. https://lore.kernel.org/git/pull.1036.v3.git.1632760428.gitgitgadget@gmail.com/
> 4. https://lore.kernel.org/git/0d119142778dce8617dd9b2c102b5f5bfdc9dc0f.1632760428.git.gitgitgadget@gmail.com/
> 6. https://lore.kernel.org/git/f1a0700e598e52d6cdb507fe8a09c4fa9291c982.1632760428.git.gitgitgadget@gmail.com/
>
...
> merge-recursive.c: call a new unpack_trees_options_init() function
>
> Details how merge-recursive.c calls unpack_trees() differently than
> every other caller when it comes to "struct unpack_trees_options"
> setup.
Saying:
"merge-recursive.c has a heap-allocated unpack_trees_options, and thus
can't naturally use UNPACK_TREES_OPTIONS_INIT"
would have been shorter, and also explained things in full detail.
Your version makes it sound like it's doing something really weird and
needs a much more expansive explanation.
> unpack-trees.[ch]: embed "dir" in "struct unpack_trees_options"
>
> Elijah's series ends up with a "dir" still heap-allocated in "struct
> unpack_trees_options", just dynamically and "privately".
As noted above, this is not true. I'm confused why you try to claim
otherwise. (I mean, it's really not all that important, I'm just
confused why you find it important to call out, especially when the
stack-based point was highlighted multiple times before but you still
insist on referring to it as heap-allocated.)
...
> unpack-trees: don't leak memory in verify_clean_subdirectory()
> merge.c: avoid duplicate unpack_trees_options_release() code
> built-ins: plug memory leaks with unpack_trees_options_release()
>
> A lot of memory leak fixes both in unpack-trees.c and its users, only
> a subset of this is in Elijah's series.
Not sure why you feel the need to include the final phrase there; it
almost feels like you're trying to portray my series as a leakfix,
which feels misleading. My series wasn't even about fixing leaks. In
my first round, I knew of leaks, and intentionally attempted to avoid
fixing them because it was orthogonal to the point of my series (I
figured I could come back in a follow-on series and deal with it). In
a subsequent round, I fixed one leak incidentally, in part because you
called it out, but more so because otherwise when I attempted to
consolidate code into one place it would appear to reviewers that the
consolidated code didn't match some of the callers. In particular,
some of the sites had a leak and others didn't. Adding a preparatory
leakfix (again, patch #2, NOT patch #4) made clear that the later
consolidation (in patch #4) really was just that -- moving several
identical code chunks into a single place.
....anyway, all that said, you've got some good fixes in this series.
You've also got three very problematic bits that need to be ripped
out. And you should rebase this series on top of v3 of
en/removing-untracked-fixes.
^ permalink raw reply [flat|nested] 31+ messages in thread