[PATCH] refs/files: prevent memory leak by freeing packed_ref_store

[PATCH] refs/files: prevent memory leak by freeing packed_ref_store

[Sven Strickroth via GitGitGadget]

From: Sven Strickroth <email@cs-ware.de>

This complements "refs: implement removal of ref storages" (64a6dd8ffc2f).

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
    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

Re: [PATCH] refs/files: prevent memory leak by freeing packed_ref_store

[Patrick Steinhardt]

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

On Sat, Aug 03, 2024 at 10:37:51AM +0000, Sven Strickroth via GitGitGadget wrote:
> From: Sven Strickroth <email@cs-ware.de>
> 
> 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

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

Re: [PATCH] refs/files: prevent memory leak by freeing packed_ref_store

[Sven Strickroth]

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

[PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store

[Sven Strickroth via GitGitGadget]

From: Sven Strickroth <email@cs-ware.de>

This complements 64a6dd8ffc (refs: implement removal of ref storages,
2024-06-06).

Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
    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 <email@cs-ware.de>
      


 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

Re: [PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store

[Patrick Steinhardt]

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

On Mon, Aug 05, 2024 at 09:53:32AM +0000, Sven Strickroth via GitGitGadget wrote:
> From: Sven Strickroth <email@cs-ware.de>
> 
> This complements 64a6dd8ffc (refs: implement removal of ref storages,
> 2024-06-06).
> 
> Signed-off-by: Sven Strickroth <email@cs-ware.de>

Thanks, this version looks good to me!

Patrick

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

Re: [PATCH v2] refs/files: prevent memory leak by freeing packed_ref_store

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Aug 05, 2024 at 09:53:32AM +0000, Sven Strickroth via GitGitGadget wrote:
>> From: Sven Strickroth <email@cs-ware.de>
>> 
>> This complements 64a6dd8ffc (refs: implement removal of ref storages,
>> 2024-06-06).
>> 
>> Signed-off-by: Sven Strickroth <email@cs-ware.de>
>
> Thanks, this version looks good to me!

Thanks, both.