[PATCH 0/4] Memory leak fixes

[PATCH 0/4] Memory leak fixes

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

Hi,

this patch series fixes some memory leaks. All of these leaks have been
found while working on the reftable backend.

Patrick

Patrick Steinhardt (4):
  test-bloom: stop setting up Git directory twice
  shallow: fix memory leak when registering shallow roots
  setup: refactor `upgrade_repository_format()` to have common exit
  setup: fix leaking repository format

 setup.c                         | 33 ++++++++++++++++++++++-----------
 shallow.c                       |  4 +++-
 t/helper/test-bloom.c           |  1 -
 t/t5311-pack-bitmaps-shallow.sh |  2 ++
 t/t5530-upload-pack-error.sh    |  1 +
 5 files changed, 28 insertions(+), 13 deletions(-)

-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/4] test-bloom: stop setting up Git directory twice

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

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 <ps@pks.im>
---
 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 aabe31d724b..1281e66876f 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,
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/4] shallow: fix memory leak when registering shallow roots

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

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 <ps@pks.im>
---
 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 5413719fd4e..ac728cdd778 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 9dae60f73e3..4fe71fe8cd2 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 7c1460eaa99..de6e14a6c24 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)
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/4] setup: refactor `upgrade_repository_format()` to have common exit

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]

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 <ps@pks.im>
---
 setup.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/setup.c b/setup.c
index 2e607632dbd..b9474b163a0 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)
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/4] setup: fix leaking repository format

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

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 <ps@pks.im>
---
 setup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/setup.c b/setup.c
index b9474b163a0..fc592dc6dd5 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;
 }
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] test-bloom: stop setting up Git directory twice

[Jeff King]

On Mon, Nov 06, 2023 at 11:45:53AM +0100, Patrick Steinhardt wrote:

> 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.

Makes sense. This situation was created by 094a685cd7 (t: make
test-bloom initialize repository, 2020-07-29), which added the setup
call at the start of the program.

That commit closed the door to running test-bloom outside of a
repository for sub-commands that could handle it (perhaps the murmur3
one, but I didn't test). So there are two possible directions here:

  - drop the call in cmd__bloom() and make sure all of the relevant
    sub-command functions do the setup.

  - drop the one command-specific one (i.e., your patch)

In practice I don't think anybody cares about running this test-helper
outside of a repository, and having to do setup in each sub-command is
an extra maintenance burden. So I think your patch is the best
direction.

-Peff

Re: [PATCH 2/4] shallow: fix memory leak when registering shallow roots

[Jeff King]

On Mon, Nov 06, 2023 at 11:45:57AM +0100, Patrick Steinhardt wrote:

> --- 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);
>  }

Good catch. When I've dealt with leaks around commit_lists in the past,
one gotcha is that sometimes we've saved a pointer to the list
elsewhere. But in this case it looks pretty clear that the parent list
just goes away. So this patch is doing the right thing.

-Peff

Re: [PATCH 0/4] Memory leak fixes

[Jeff King]

On Mon, Nov 06, 2023 at 11:45:48AM +0100, Patrick Steinhardt wrote:

> this patch series fixes some memory leaks. All of these leaks have been
> found while working on the reftable backend.

All four look good to me (and the refactoring in 3/4 is very cleanly
done).

-Peff

Re: [PATCH 0/4] Memory leak fixes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Nov 06, 2023 at 11:45:48AM +0100, Patrick Steinhardt wrote:
>
>> this patch series fixes some memory leaks. All of these leaks have been
>> found while working on the reftable backend.
>
> All four look good to me (and the refactoring in 3/4 is very cleanly
> done).

Thanks, both.