{"thread":{"id":"60475","subject":"[PATCH 0/4] Memory leak fixes","startedAt":"2023-11-06T10:45:57Z","lastAt":"2023-11-07T02:21:00Z","messageCount":9,"participants":["Patrick Steinhardt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"484452","messageId":"cover.1699267422.git.ps@pks.im","threadId":"60475","inReplyTo":null,"subject":"[PATCH 0/4] Memory leak fixes","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-06T10:45:48Z","receivedAt":"2023-11-06T10:45:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series fixes some memory leaks. All of these leaks have been\nfound while working on the reftable backend.\n\nPatrick\n\nPatrick Steinhardt (4):\n  test-bloom: stop setting up Git directory twice\n  shallow: fix memory leak when registering shallow roots\n  setup: refactor `upgrade_repository_format()` to have common exit\n  setup: fix leaking repository format\n\n setup.c                         | 33 ++++++++++++++++++++++-----------\n shallow.c                       |  4 +++-\n t/helper/test-bloom.c           |  1 -\n t/t5311-pack-bitmaps-shallow.sh |  2 ++\n t/t5530-upload-pack-error.sh    |  1 +\n 5 files changed, 28 insertions(+), 13 deletions(-)\n\n-- \n2.42.0\n\n"},{"id":"484453","messageId":"fb1cc73ed3395ba3de72aba70d7679dd85ebb034.1699267422.git.ps@pks.im","threadId":"60475","inReplyTo":"cover.1699267422.git.ps@pks.im","subject":"[PATCH 1/4] test-bloom: stop setting up Git directory twice","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-06T10:45:53Z","receivedAt":"2023-11-06T10:45:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're setting up the Git directory twice in the `test-tool bloom`\nhelper, once at the beginning of `cmd_bloom()` and once in the local\nsubcommand implementation `get_bloom_filter_for_commit()`. This can lead\nto memory leaks as we'll overwrite variables of `the_repository` with\nnewly allocated data structures. On top of that it's simply unnecessary.\n\nFix this by only setting up the Git directory once.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/helper/test-bloom.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/helper/test-bloom.c b/t/helper/test-bloom.c\nindex aabe31d724b..1281e66876f 100644\n--- a/t/helper/test-bloom.c\n+++ b/t/helper/test-bloom.c\n@@ -40,7 +40,6 @@ static void get_bloom_filter_for_commit(const struct object_id *commit_oid)\n {\n \tstruct commit *c;\n \tstruct bloom_filter *filter;\n-\tsetup_git_directory();\n \tc = lookup_commit(the_repository, commit_oid);\n \tfilter = get_or_compute_bloom_filter(the_repository, c, 1,\n \t\t\t\t\t     &settings,\n-- \n2.42.0\n\n"},{"id":"484454","messageId":"137c150e358a8248200e0c7174a79358adb92e45.1699267422.git.ps@pks.im","threadId":"60475","inReplyTo":"cover.1699267422.git.ps@pks.im","subject":"[PATCH 2/4] shallow: fix memory leak when registering shallow roots","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-06T10:45:57Z","receivedAt":"2023-11-06T10:46:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When registering shallow roots, we unset the list of parents of the\nto-be-registered commit if it's already been parsed. This causes us to\nleak memory though because we never free this list. Fix this.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n shallow.c                       | 4 +++-\n t/t5311-pack-bitmaps-shallow.sh | 2 ++\n t/t5530-upload-pack-error.sh    | 1 +\n 3 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex 5413719fd4e..ac728cdd778 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -38,8 +38,10 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n \n \toidcpy(&graft->oid, oid);\n \tgraft->nr_parent = -1;\n-\tif (commit && commit->object.parsed)\n+\tif (commit && commit->object.parsed) {\n+\t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n+\t}\n \treturn register_commit_graft(r, graft, 0);\n }\n \ndiff --git a/t/t5311-pack-bitmaps-shallow.sh b/t/t5311-pack-bitmaps-shallow.sh\nindex 9dae60f73e3..4fe71fe8cd2 100755\n--- a/t/t5311-pack-bitmaps-shallow.sh\n+++ b/t/t5311-pack-bitmaps-shallow.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='check bitmap operation with shallow repositories'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # We want to create a situation where the shallow, grafted\ndiff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh\nindex 7c1460eaa99..de6e14a6c24 100755\n--- a/t/t5530-upload-pack-error.sh\n+++ b/t/t5530-upload-pack-error.sh\n@@ -2,6 +2,7 @@\n \n test_description='errors in upload-pack'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n D=$(pwd)\n-- \n2.42.0\n\n"},{"id":"484455","messageId":"44e133dc8d8e2b2e2172edff0847968bed9c3f27.1699267422.git.ps@pks.im","threadId":"60475","inReplyTo":"cover.1699267422.git.ps@pks.im","subject":"[PATCH 3/4] setup: refactor `upgrade_repository_format()` to have common exit","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-06T10:46:01Z","receivedAt":"2023-11-06T10:46:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `upgrade_repository_format()` function has multiple exit paths,\nwhich means that there is no common cleanup of acquired resources.\nWhile this isn't much of a problem right now, we're about to fix a\nmemory leak that would require us to free the resource in every one of\nthose exit paths.\n\nRefactor the code to have a common exit path so that the subsequent\nmemory leak fix becomes easier to implement.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n setup.c | 31 ++++++++++++++++++++-----------\n 1 file changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 2e607632dbd..b9474b163a0 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -693,29 +693,38 @@ int upgrade_repository_format(int target_version)\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct strbuf repo_version = STRBUF_INIT;\n \tstruct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;\n+\tint ret;\n \n \tstrbuf_git_common_path(&sb, the_repository, \"config\");\n \tread_repository_format(&repo_fmt, sb.buf);\n \tstrbuf_release(&sb);\n \n-\tif (repo_fmt.version >= target_version)\n-\t\treturn 0;\n+\tif (repo_fmt.version >= target_version) {\n+\t\tret = 0;\n+\t\tgoto out;\n+\t}\n \n \tif (verify_repository_format(&repo_fmt, &err) < 0) {\n-\t\terror(\"cannot upgrade repository format from %d to %d: %s\",\n-\t\t      repo_fmt.version, target_version, err.buf);\n-\t\tstrbuf_release(&err);\n-\t\treturn -1;\n+\t\tret = error(\"cannot upgrade repository format from %d to %d: %s\",\n+\t\t\t    repo_fmt.version, target_version, err.buf);\n+\t\tgoto out;\n+\t}\n+\tif (!repo_fmt.version && repo_fmt.unknown_extensions.nr) {\n+\t\tret = error(\"cannot upgrade repository format: \"\n+\t\t\t    \"unknown extension %s\",\n+\t\t\t    repo_fmt.unknown_extensions.items[0].string);\n+\t\tgoto out;\n \t}\n-\tif (!repo_fmt.version && repo_fmt.unknown_extensions.nr)\n-\t\treturn error(\"cannot upgrade repository format: \"\n-\t\t\t     \"unknown extension %s\",\n-\t\t\t     repo_fmt.unknown_extensions.items[0].string);\n \n \tstrbuf_addf(&repo_version, \"%d\", target_version);\n \tgit_config_set(\"core.repositoryformatversion\", repo_version.buf);\n+\n+\tret = 1;\n+\n+out:\n \tstrbuf_release(&repo_version);\n-\treturn 1;\n+\tstrbuf_release(&err);\n+\treturn ret;\n }\n \n static void init_repository_format(struct repository_format *format)\n-- \n2.42.0\n\n"},{"id":"484456","messageId":"1bbddbcb0b0b91128b9ddec69d8374c92e9128d5.1699267422.git.ps@pks.im","threadId":"60475","inReplyTo":"cover.1699267422.git.ps@pks.im","subject":"[PATCH 4/4] setup: fix leaking repository format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-06T10:46:05Z","receivedAt":"2023-11-06T10:46:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While populating the `repository_format` structure may cause us to\nallocate memory, we do not call `clear_repository_format()` in some\nplaces and thus potentially leak memory. Fix this.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n setup.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex b9474b163a0..fc592dc6dd5 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -722,6 +722,7 @@ int upgrade_repository_format(int target_version)\n \tret = 1;\n \n out:\n+\tclear_repository_format(&repo_fmt);\n \tstrbuf_release(&repo_version);\n \tstrbuf_release(&err);\n \treturn ret;\n@@ -2199,6 +2200,7 @@ int init_db(const char *git_dir, const char *real_git_dir,\n \t\t\t       git_dir, len && git_dir[len-1] != '/' ? \"/\" : \"\");\n \t}\n \n+\tclear_repository_format(&repo_fmt);\n \tfree(original_git_dir);\n \treturn 0;\n }\n-- \n2.42.0\n\n"},{"id":"484466","messageId":"20231106172024.GA10414@coredump.intra.peff.net","threadId":"60475","inReplyTo":"fb1cc73ed3395ba3de72aba70d7679dd85ebb034.1699267422.git.ps@pks.im","subject":"Re: [PATCH 1/4] test-bloom: stop setting up Git directory twice","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-06T17:20:24Z","receivedAt":"2023-11-06T17:20:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 06, 2023 at 11:45:53AM +0100, Patrick Steinhardt wrote:\n\n> We're setting up the Git directory twice in the `test-tool bloom`\n> helper, once at the beginning of `cmd_bloom()` and once in the local\n> subcommand implementation `get_bloom_filter_for_commit()`. This can lead\n> to memory leaks as we'll overwrite variables of `the_repository` with\n> newly allocated data structures. On top of that it's simply unnecessary.\n> \n> Fix this by only setting up the Git directory once.\n\nMakes sense. This situation was created by 094a685cd7 (t: make\ntest-bloom initialize repository, 2020-07-29), which added the setup\ncall at the start of the program.\n\nThat commit closed the door to running test-bloom outside of a\nrepository for sub-commands that could handle it (perhaps the murmur3\none, but I didn't test). So there are two possible directions here:\n\n  - drop the call in cmd__bloom() and make sure all of the relevant\n    sub-command functions do the setup.\n\n  - drop the one command-specific one (i.e., your patch)\n\nIn practice I don't think anybody cares about running this test-helper\noutside of a repository, and having to do setup in each sub-command is\nan extra maintenance burden. So I think your patch is the best\ndirection.\n\n-Peff\n"},{"id":"484467","messageId":"20231106172154.GB10414@coredump.intra.peff.net","threadId":"60475","inReplyTo":"137c150e358a8248200e0c7174a79358adb92e45.1699267422.git.ps@pks.im","subject":"Re: [PATCH 2/4] shallow: fix memory leak when registering shallow roots","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-06T17:21:54Z","receivedAt":"2023-11-06T17:21:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 06, 2023 at 11:45:57AM +0100, Patrick Steinhardt wrote:\n\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -38,8 +38,10 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n>  \n>  \toidcpy(&graft->oid, oid);\n>  \tgraft->nr_parent = -1;\n> -\tif (commit && commit->object.parsed)\n> +\tif (commit && commit->object.parsed) {\n> +\t\tfree_commit_list(commit->parents);\n>  \t\tcommit->parents = NULL;\n> +\t}\n>  \treturn register_commit_graft(r, graft, 0);\n>  }\n\nGood catch. When I've dealt with leaks around commit_lists in the past,\none gotcha is that sometimes we've saved a pointer to the list\nelsewhere. But in this case it looks pretty clear that the parent list\njust goes away. So this patch is doing the right thing.\n\n-Peff\n"},{"id":"484469","messageId":"20231106173205.GD10414@coredump.intra.peff.net","threadId":"60475","inReplyTo":"cover.1699267422.git.ps@pks.im","subject":"Re: [PATCH 0/4] Memory leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-06T17:32:05Z","receivedAt":"2023-11-06T17:32:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 06, 2023 at 11:45:48AM +0100, Patrick Steinhardt wrote:\n\n> this patch series fixes some memory leaks. All of these leaks have been\n> found while working on the reftable backend.\n\nAll four look good to me (and the refactoring in 3/4 is very cleanly\ndone).\n\n-Peff\n"},{"id":"484498","messageId":"xmqqedh2b8xj.fsf@gitster.g","threadId":"60475","inReplyTo":"20231106173205.GD10414@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] Memory leak fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-07T02:20:56Z","receivedAt":"2023-11-07T02:21:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Nov 06, 2023 at 11:45:48AM +0100, Patrick Steinhardt wrote:\n>\n>> this patch series fixes some memory leaks. All of these leaks have been\n>> found while working on the reftable backend.\n>\n> All four look good to me (and the refactoring in 3/4 is very cleanly\n> done).\n\nThanks, both.\n"}]}