# [PATCH] refs/files: prevent memory leak by freeing packed_ref_store 6 messages from 2024-08-03 to 2024-08-05. Participants: Sven Strickroth via GitGitGadget, Patrick Steinhardt, Sven Strickroth, Junio C Hamano. Thread: https://gitlist.dev/t/61895 ## Sven Strickroth via GitGitGadget, 2024-08-03 10:37 Subject: [PATCH] refs/files: prevent memory leak by freeing packed_ref_store Message-ID: URL: https://gitlist.dev/e/pull.1757.git.git.1722681471550.gitgitgadget%40gmail.com ``` From: Sven Strickroth This complements "refs: implement removal of ref storages" (64a6dd8ffc2f). Signed-off-by: Sven Strickroth --- refs/files: prevent memory leak by freeing packed_ref_store This complements "refs: implement removal of ref storages" (64a6dd8ffc2f). Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1757%2Fcsware%2Frefs-files-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1757/csware/refs-files-v1 Pull-Request: https://github.com/git/git/pull/1757 refs/files-backend.c | 1 + 1 file changed, 1 insertion(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index aa52d9be7c7..11551de8f84 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -157,6 +157,7 @@ static void files_ref_store_release(struct ref_store *ref_store) free_ref_cache(refs->loose); free(refs->gitcommondir); ref_store_release(refs->packed_ref_store); + free(refs->packed_ref_store); } static void files_reflog_path(struct files_ref_store *refs, base-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7 -- gitgitgadget ``` ## Patrick Steinhardt, 2024-08-05 08:36 Subject: Re: [PATCH] refs/files: prevent memory leak by freeing packed_ref_store Message-ID: URL: https://gitlist.dev/e/ZrCPBXql7ySbEeXG%40tanuki In-Reply-To: ``` On Sat, Aug 03, 2024 at 10:37:51AM +0000, Sven Strickroth via GitGitGadget wrote: > From: Sven Strickroth > > This complements "refs: implement removal of ref storages" (64a6dd8ffc2f). The format of references should match `git log --format=reference`, which would be: 64a6dd8ffc (refs: implement removal of ref storages, 2024-06-06) > diff --git a/refs/files-backend.c b/refs/files-backend.c > index aa52d9be7c7..11551de8f84 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -157,6 +157,7 @@ static void files_ref_store_release(struct ref_store *ref_store) > free_ref_cache(refs->loose); > free(refs->gitcommondir); > ref_store_release(refs->packed_ref_store); > + free(refs->packed_ref_store); > } Makes sense. `packed_ref_store_init()` returns a newly-allocated ref store, and `ref_store_release()` only releases the store contents. Consequently, we have to manually free the store here. That does highlight that `packed_ref_store_init()` is misnamed and really should be called `packed_ref_store_new()`, as it also allocates the structure itself. But that's a #leftoverbit for another day, I'd say. Out of curiosity, did you hit this memory leak in some of our tests, or did you just happen to stumble over it by chance? Thanks! Patrick ``` ## Sven Strickroth, 2024-08-05 09:45 Subject: Re: [PATCH] refs/files: prevent memory leak by freeing packed_ref_store Message-ID: <8953733a-7553-4514-8990-866947b54ae7@cs-ware.de> URL: https://gitlist.dev/e/8953733a-7553-4514-8990-866947b54ae7%40cs-ware.de In-Reply-To: ``` Am 05.08.2024 um 10:36 schrieb Patrick Steinhardt: > That does highlight that `packed_ref_store_init()` is misnamed and > really should be called `packed_ref_store_new()`, as it also allocates > the structure itself. But that's a #leftoverbit for another day, I'd > say. This would also be true for ref for `files_ref_store_init` and `reftable_be_init`. > Out of curiosity, did you hit this memory leak in some of our tests, or > did you just happen to stumble over it by chance? I found this while working on TortoiseGit which also uses libgit internally. -- Best regards, Sven Strickroth PGP key id F5A9D4C4 @ any key-server ``` ## Sven Strickroth via GitGitGadget, 2024-08-05 09:53 Subject: [PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store Message-ID: URL: https://gitlist.dev/e/pull.1757.v2.git.git.1722851612505.gitgitgadget%40gmail.com In-Reply-To: ``` From: Sven Strickroth This complements 64a6dd8ffc (refs: implement removal of ref storages, 2024-06-06). Signed-off-by: Sven Strickroth --- refs/files: prevent memory leak by freeing packed_ref_store This complements "refs: implement removal of ref storages" (64a6dd8ffc2f). Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1757%2Fcsware%2Frefs-files-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1757/csware/refs-files-v2 Pull-Request: https://github.com/git/git/pull/1757 Range-diff vs v1: 1: c68d0de3d58 ! 1: 96018e7257c refs/files: prevent memory leak by freeing packed_ref_store @@ Metadata ## Commit message ## refs/files: prevent memory leak by freeing packed_ref_store - This complements "refs: implement removal of ref storages" (64a6dd8ffc2f). + This complements 64a6dd8ffc (refs: implement removal of ref storages, + 2024-06-06). Signed-off-by: Sven Strickroth refs/files-backend.c | 1 + 1 file changed, 1 insertion(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index aa52d9be7c7..11551de8f84 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -157,6 +157,7 @@ static void files_ref_store_release(struct ref_store *ref_store) free_ref_cache(refs->loose); free(refs->gitcommondir); ref_store_release(refs->packed_ref_store); + free(refs->packed_ref_store); } static void files_reflog_path(struct files_ref_store *refs, base-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7 -- gitgitgadget ``` ## Patrick Steinhardt, 2024-08-05 11:17 Subject: Re: [PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store Message-ID: URL: https://gitlist.dev/e/ZrC0sbxnCONqnPPI%40tanuki In-Reply-To: ``` On Mon, Aug 05, 2024 at 09:53:32AM +0000, Sven Strickroth via GitGitGadget wrote: > From: Sven Strickroth > > This complements 64a6dd8ffc (refs: implement removal of ref storages, > 2024-06-06). > > Signed-off-by: Sven Strickroth Thanks, this version looks good to me! Patrick ``` ## Junio C Hamano, 2024-08-05 15:58 Subject: Re: [PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store Message-ID: URL: https://gitlist.dev/e/xmqqmslr3qrj.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: > On Mon, Aug 05, 2024 at 09:53:32AM +0000, Sven Strickroth via GitGitGadget wrote: >> From: Sven Strickroth >> >> This complements 64a6dd8ffc (refs: implement removal of ref storages, >> 2024-06-06). >> >> Signed-off-by: Sven Strickroth > > Thanks, this version looks good to me! Thanks, both. ```