{"thread":{"id":"61895","subject":"[PATCH] refs/files: prevent memory leak by freeing packed_ref_store","startedAt":"2024-08-03T10:37:54Z","lastAt":"2024-08-05T15:58:59Z","messageCount":6,"participants":["Sven Strickroth via GitGitGadget","Patrick Steinhardt","Sven Strickroth","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"499998","messageId":"pull.1757.git.git.1722681471550.gitgitgadget@gmail.com","threadId":"61895","inReplyTo":null,"subject":"[PATCH] refs/files: prevent memory leak by freeing packed_ref_store","fromName":"Sven Strickroth via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-03T10:37:51Z","receivedAt":"2024-08-03T10:37:54Z","isPatch":true,"sender":{"key":"sven.strickroth@tu-clausthal.de","avatar":null},"body":"From: Sven Strickroth \n\nThis complements \"refs: implement removal of ref storages\" (64a6dd8ffc2f).\n\nSigned-off-by: Sven Strickroth \n---\n refs/files: prevent memory leak by freeing packed_ref_store\n \n This complements \"refs: implement removal of ref storages\"\n (64a6dd8ffc2f).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1757%2Fcsware%2Frefs-files-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1757/csware/refs-files-v1\nPull-Request: https://github.com/git/git/pull/1757\n\n refs/files-backend.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex aa52d9be7c7..11551de8f84 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -157,6 +157,7 @@ static void files_ref_store_release(struct ref_store *ref_store)\n \tfree_ref_cache(refs->loose);\n \tfree(refs->gitcommondir);\n \tref_store_release(refs->packed_ref_store);\n+\tfree(refs->packed_ref_store);\n }\n \n static void files_reflog_path(struct files_ref_store *refs,\n\nbase-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7\n-- \ngitgitgadget\n"},{"id":"500046","messageId":"ZrCPBXql7ySbEeXG@tanuki","threadId":"61895","inReplyTo":"pull.1757.git.git.1722681471550.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs/files: prevent memory leak by freeing packed_ref_store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-05T08:36:21Z","receivedAt":"2024-08-05T08:36:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 03, 2024 at 10:37:51AM +0000, Sven Strickroth via GitGitGadget wrote:\n> From: Sven Strickroth \n> \n> This complements \"refs: implement removal of ref storages\" (64a6dd8ffc2f).\n\nThe format of references should match `git log --format=reference`,\nwhich would be:\n\n 64a6dd8ffc (refs: implement removal of ref storages, 2024-06-06)\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index aa52d9be7c7..11551de8f84 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -157,6 +157,7 @@ static void files_ref_store_release(struct ref_store *ref_store)\n> \tfree_ref_cache(refs->loose);\n> \tfree(refs->gitcommondir);\n> \tref_store_release(refs->packed_ref_store);\n> +\tfree(refs->packed_ref_store);\n> }\n\nMakes sense. `packed_ref_store_init()` returns a newly-allocated ref\nstore, and `ref_store_release()` only releases the store contents.\nConsequently, we have to manually free the store here.\n\nThat does highlight that `packed_ref_store_init()` is misnamed and\nreally should be called `packed_ref_store_new()`, as it also allocates\nthe structure itself. But that's a #leftoverbit for another day, I'd\nsay.\n\nOut of curiosity, did you hit this memory leak in some of our tests, or\ndid you just happen to stumble over it by chance?\n\nThanks!\n\nPatrick\n"},{"id":"500054","messageId":"8953733a-7553-4514-8990-866947b54ae7@cs-ware.de","threadId":"61895","inReplyTo":"ZrCPBXql7ySbEeXG@tanuki","subject":"Re: [PATCH] refs/files: prevent memory leak by freeing packed_ref_store","fromName":"Sven Strickroth","fromEmail":"email@cs-ware.de","sentAt":"2024-08-05T09:45:37Z","receivedAt":"2024-08-05T09:52:59Z","isPatch":true,"sender":{"key":"email@cs-ware.de","avatar":"https://avatars.githubusercontent.com/u/428133?v=4"},"body":"Am 05.08.2024 um 10:36 schrieb Patrick Steinhardt:\n> That does highlight that `packed_ref_store_init()` is misnamed and\n> really should be called `packed_ref_store_new()`, as it also allocates\n> the structure itself. But that's a #leftoverbit for another day, I'd\n> say.\n\nThis would also be true for ref for `files_ref_store_init` and \n`reftable_be_init`.\n\n> Out of curiosity, did you hit this memory leak in some of our tests, or\n> did you just happen to stumble over it by chance?\n\nI found this while working on TortoiseGit which also uses libgit internally.\n\n-- \nBest regards,\n Sven Strickroth\n PGP key id F5A9D4C4 @ any key-server\n"},{"id":"500055","messageId":"pull.1757.v2.git.git.1722851612505.gitgitgadget@gmail.com","threadId":"61895","inReplyTo":"pull.1757.git.git.1722681471550.gitgitgadget@gmail.com","subject":"[PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store","fromName":"Sven Strickroth via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-05T09:53:32Z","receivedAt":"2024-08-05T09:53:35Z","isPatch":true,"sender":{"key":"sven.strickroth@tu-clausthal.de","avatar":null},"body":"From: Sven Strickroth \n\nThis complements 64a6dd8ffc (refs: implement removal of ref storages,\n2024-06-06).\n\nSigned-off-by: Sven Strickroth \n---\n refs/files: prevent memory leak by freeing packed_ref_store\n \n This complements \"refs: implement removal of ref storages\"\n (64a6dd8ffc2f).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1757%2Fcsware%2Frefs-files-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1757/csware/refs-files-v2\nPull-Request: https://github.com/git/git/pull/1757\n\nRange-diff vs v1:\n\n 1: c68d0de3d58 ! 1: 96018e7257c refs/files: prevent memory leak by freeing packed_ref_store\n @@ Metadata\n ## Commit message ##\n refs/files: prevent memory leak by freeing packed_ref_store\n \n - This complements \"refs: implement removal of ref storages\" (64a6dd8ffc2f).\n + This complements 64a6dd8ffc (refs: implement removal of ref storages,\n + 2024-06-06).\n \n Signed-off-by: Sven Strickroth \n \n\n\n refs/files-backend.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex aa52d9be7c7..11551de8f84 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -157,6 +157,7 @@ static void files_ref_store_release(struct ref_store *ref_store)\n \tfree_ref_cache(refs->loose);\n \tfree(refs->gitcommondir);\n \tref_store_release(refs->packed_ref_store);\n+\tfree(refs->packed_ref_store);\n }\n \n static void files_reflog_path(struct files_ref_store *refs,\n\nbase-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7\n-- \ngitgitgadget\n"},{"id":"500059","messageId":"ZrC0sbxnCONqnPPI@tanuki","threadId":"61895","inReplyTo":"pull.1757.v2.git.git.1722851612505.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-05T11:17:05Z","receivedAt":"2024-08-05T11:17:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 05, 2024 at 09:53:32AM +0000, Sven Strickroth via GitGitGadget wrote:\n> From: Sven Strickroth \n> \n> This complements 64a6dd8ffc (refs: implement removal of ref storages,\n> 2024-06-06).\n> \n> Signed-off-by: Sven Strickroth \n\nThanks, this version looks good to me!\n\nPatrick\n"},{"id":"500094","messageId":"xmqqmslr3qrj.fsf@gitster.g","threadId":"61895","inReplyTo":"ZrC0sbxnCONqnPPI@tanuki","subject":"Re: [PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-05T15:58:56Z","receivedAt":"2024-08-05T15:58:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Mon, Aug 05, 2024 at 09:53:32AM +0000, Sven Strickroth via GitGitGadget wrote:\n>> From: Sven Strickroth \n>> \n>> This complements 64a6dd8ffc (refs: implement removal of ref storages,\n>> 2024-06-06).\n>> \n>> Signed-off-by: Sven Strickroth \n>\n> Thanks, this version looks good to me!\n\nThanks, both.\n"}]}