From 4a0479086a9bea5d31c4588b07bd45ae92a12b71 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 4 Mar 2022 19:32:12 +0100 Subject: commit-graph: fix memory leak in misused string_list API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When this code was migrated to the string_list API in d88b14b3fd6 (commit-graph: use string-list API for input, 2018-06-27) it was made to use used both STRING_LIST_INIT_NODUP and a strbuf_detach() pattern. Those should not be used together if string_list_clear() is expected to free the memory, instead we need to either use STRING_LIST_INIT_DUP with a string_list_append_nodup(), or a STRING_LIST_INIT_NODUP and manually fiddle with the "strdup_strings" member before calling string_list_clear(). Let's do the former. Since "strdup_strings = 1" is set now other code might be broken by relying on "pack_indexes" not to duplicate it strings, but that doesn't happen. When we pass this down to write_commit_graph() that code uses the "struct string_list" without modifying it. Let's add a "const" to the variable to have the compiler enforce that assumption. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- commit-graph.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'commit-graph.c') diff --git a/commit-graph.c b/commit-graph.c index 265c010122..d0c94600ba 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1679,7 +1679,7 @@ int write_commit_graph_reachable(struct object_directory *odb, } static int fill_oids_from_packs(struct write_commit_graph_context *ctx, - struct string_list *pack_indexes) + const struct string_list *pack_indexes) { uint32_t i; struct strbuf progress_title = STRBUF_INIT; @@ -2259,7 +2259,7 @@ out: } int write_commit_graph(struct object_directory *odb, - struct string_list *pack_indexes, + const struct string_list *const pack_indexes, struct oidset *commits, enum commit_graph_write_flags flags, const struct commit_graph_opts *opts) -- cgit v1.2.3 From 51a94d8ffe57ff40e1db1d5e46a1864a5ded7f49 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 4 Mar 2022 19:32:13 +0100 Subject: commit-graph: stop fill_oids_from_packs() progress on error and free() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a bug in fill_oids_from_packs(), we should always stop_progress(), but did not do so if we returned an error here. This also plugs a memory leak in those cases by releasing the two "struct strbuf" variables the function uses. While I'm at it stop hardcoding "-1" here and just use the return value of error() instead, which happens to be "-1". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- commit-graph.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) (limited to 'commit-graph.c') diff --git a/commit-graph.c b/commit-graph.c index d0c94600ba..aab0b29277 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1685,6 +1685,7 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx, struct strbuf progress_title = STRBUF_INIT; struct strbuf packname = STRBUF_INIT; int dirlen; + int ret = 0; strbuf_addf(&packname, "%s/pack/", ctx->odb->path); dirlen = packname.len; @@ -1703,12 +1704,12 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx, strbuf_addstr(&packname, pack_indexes->items[i].string); p = add_packed_git(packname.buf, packname.len, 1); if (!p) { - error(_("error adding pack %s"), packname.buf); - return -1; + ret = error(_("error adding pack %s"), packname.buf); + goto cleanup; } if (open_pack_index(p)) { - error(_("error opening index for %s"), packname.buf); - return -1; + ret = error(_("error opening index for %s"), packname.buf); + goto cleanup; } for_each_object_in_pack(p, add_packed_commits, ctx, FOR_EACH_OBJECT_PACK_ORDER); @@ -1716,11 +1717,12 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx, free(p); } +cleanup: stop_progress(&ctx->progress); strbuf_release(&progress_title); strbuf_release(&packname); - return 0; + return ret; } static int fill_oids_from_commits(struct write_commit_graph_context *ctx, -- cgit v1.2.3 From ef3fe214484f79afdad4204d56e0ee7ff6e85a0f Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 4 Mar 2022 19:32:14 +0100 Subject: lockfile API users: simplify and don't leak "path" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a memory leak in code added in 6c622f9f0bb (commit-graph: write commit-graph chains, 2019-06-18). We needed to free the "lock_name" if we encounter errors, and the "graph_name" after we'd run unlink() on it. For the case of write_commit_graph_file() refactoring the code to free the "lock_name" after we were done using the "struct lock_file lk" would have made the control flow more complex. Luckily we can free the "lock_file" right after the hold_lock_file_for_update() call, if it makes use of "path" at all it'll have copied its contents to a "struct strbuf" of its own. While I'm at it let's fix code added in fb10ca5b543 (sparse-checkout: write using lockfile, 2019-11-21) in write_patterns_and_update() to avoid the same complexity that I thought I needed when I wrote the initial fix for write_commit_graph_file(). We can free the "sparse_filename" right after calling hold_lock_file_for_update(), we don't need to wait until we're exiting the function to do so. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 3 +-- commit-graph.c | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) (limited to 'commit-graph.c') diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 9c338d33ea..270ad49c2b 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -328,11 +328,11 @@ static int write_patterns_and_update(struct pattern_list *pl) fd = hold_lock_file_for_update(&lk, sparse_filename, LOCK_DIE_ON_ERROR); + free(sparse_filename); result = update_working_directory(pl); if (result) { rollback_lock_file(&lk); - free(sparse_filename); clear_pattern_list(pl); update_working_directory(NULL); return result; @@ -348,7 +348,6 @@ static int write_patterns_and_update(struct pattern_list *pl) fflush(fp); commit_lock_file(&lk); - free(sparse_filename); clear_pattern_list(pl); return 0; diff --git a/commit-graph.c b/commit-graph.c index aab0b29277..b8cde7ea27 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1854,6 +1854,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) hold_lock_file_for_update_mode(&lk, lock_name, LOCK_DIE_ON_ERROR, 0444); + free(lock_name); fd = git_mkstemp_mode(ctx->graph_name, 0444); if (fd < 0) { @@ -1978,6 +1979,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) } else { char *graph_name = get_commit_graph_filename(ctx->odb); unlink(graph_name); + free(graph_name); } ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(hash_to_hex(file_hash)); -- cgit v1.2.3