From 40e9136ff641682e2ef739a3dbce03047ed5426f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 6 Nov 2023 11:45:53 +0100 Subject: [PATCH 1/4] test-bloom: stop setting up Git directory twice We're setting up the Git directory twice in the `test-tool bloom` helper, once at the beginning of `cmd_bloom()` and once in the local subcommand implementation `get_bloom_filter_for_commit()`. This can lead to memory leaks as we'll overwrite variables of `the_repository` with newly allocated data structures. On top of that it's simply unnecessary. Fix this by only setting up the Git directory once. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- t/helper/test-bloom.c | 1 - 1 file changed, 1 deletion(-) diff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c index aabe31d724b675..1281e66876f35d 100644 --- a/t/helper/test-bloom.c +++ b/t/helper/test-bloom.c @@ -40,7 +40,6 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid) { struct commit *c; struct bloom_filter *filter; - setup_git_directory(); c = lookup_commit(the_repository, commit_oid); filter = get_or_compute_bloom_filter(the_repository, c, 1, &settings, From 568cc818cc26955eb0b94084d3068caabab1edd7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 6 Nov 2023 11:45:57 +0100 Subject: [PATCH 2/4] shallow: fix memory leak when registering shallow roots When registering shallow roots, we unset the list of parents of the to-be-registered commit if it's already been parsed. This causes us to leak memory though because we never free this list. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- shallow.c | 4 +++- t/t5311-pack-bitmaps-shallow.sh | 2 ++ t/t5530-upload-pack-error.sh | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/shallow.c b/shallow.c index 5413719fd4ef16..ac728cdd77892d 100644 --- a/shallow.c +++ b/shallow.c @@ -38,8 +38,10 @@ int register_shallow(struct repository *r, const struct object_id *oid) oidcpy(&graft->oid, oid); graft->nr_parent = -1; - if (commit && commit->object.parsed) + if (commit && commit->object.parsed) { + free_commit_list(commit->parents); commit->parents = NULL; + } return register_commit_graft(r, graft, 0); } diff --git a/t/t5311-pack-bitmaps-shallow.sh b/t/t5311-pack-bitmaps-shallow.sh index 9dae60f73e3253..4fe71fe8cd21dd 100755 --- a/t/t5311-pack-bitmaps-shallow.sh +++ b/t/t5311-pack-bitmaps-shallow.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='check bitmap operation with shallow repositories' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # We want to create a situation where the shallow, grafted diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh index 7c1460eaa99865..de6e14a6c24ae6 100755 --- a/t/t5530-upload-pack-error.sh +++ b/t/t5530-upload-pack-error.sh @@ -2,6 +2,7 @@ test_description='errors in upload-pack' +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh D=$(pwd) From 4ce14e13250e824b7d410d9bff88061525346a15 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 6 Nov 2023 11:46:01 +0100 Subject: [PATCH 3/4] setup: refactor `upgrade_repository_format()` to have common exit The `upgrade_repository_format()` function has multiple exit paths, which means that there is no common cleanup of acquired resources. While this isn't much of a problem right now, we're about to fix a memory leak that would require us to free the resource in every one of those exit paths. Refactor the code to have a common exit path so that the subsequent memory leak fix becomes easier to implement. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- setup.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/setup.c b/setup.c index 2e607632dbde80..b9474b163a0f53 100644 --- a/setup.c +++ b/setup.c @@ -693,29 +693,38 @@ int upgrade_repository_format(int target_version) struct strbuf err = STRBUF_INIT; struct strbuf repo_version = STRBUF_INIT; struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT; + int ret; strbuf_git_common_path(&sb, the_repository, "config"); read_repository_format(&repo_fmt, sb.buf); strbuf_release(&sb); - if (repo_fmt.version >= target_version) - return 0; + if (repo_fmt.version >= target_version) { + ret = 0; + goto out; + } if (verify_repository_format(&repo_fmt, &err) < 0) { - error("cannot upgrade repository format from %d to %d: %s", - repo_fmt.version, target_version, err.buf); - strbuf_release(&err); - return -1; + ret = error("cannot upgrade repository format from %d to %d: %s", + repo_fmt.version, target_version, err.buf); + goto out; + } + if (!repo_fmt.version && repo_fmt.unknown_extensions.nr) { + ret = error("cannot upgrade repository format: " + "unknown extension %s", + repo_fmt.unknown_extensions.items[0].string); + goto out; } - if (!repo_fmt.version && repo_fmt.unknown_extensions.nr) - return error("cannot upgrade repository format: " - "unknown extension %s", - repo_fmt.unknown_extensions.items[0].string); strbuf_addf(&repo_version, "%d", target_version); git_config_set("core.repositoryformatversion", repo_version.buf); + + ret = 1; + +out: strbuf_release(&repo_version); - return 1; + strbuf_release(&err); + return ret; } static void init_repository_format(struct repository_format *format) From 9972cd6004ac46a919d2e8773be976ef1e2d6a65 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Mon, 6 Nov 2023 11:46:05 +0100 Subject: [PATCH 4/4] setup: fix leaking repository format While populating the `repository_format` structure may cause us to allocate memory, we do not call `clear_repository_format()` in some places and thus potentially leak memory. Fix this. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- setup.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/setup.c b/setup.c index b9474b163a0f53..fc592dc6dd5bf3 100644 --- a/setup.c +++ b/setup.c @@ -722,6 +722,7 @@ int upgrade_repository_format(int target_version) ret = 1; out: + clear_repository_format(&repo_fmt); strbuf_release(&repo_version); strbuf_release(&err); return ret; @@ -2199,6 +2200,7 @@ int init_db(const char *git_dir, const char *real_git_dir, git_dir, len && git_dir[len-1] != '/' ? "/" : ""); } + clear_repository_format(&repo_fmt); free(original_git_dir); return 0; }