refs/files: prevent memory leak by freeing packed_ref_store

Commit Message

Sven Strickroth Aug. 3, 2024, 10:37 a.m. UTC

From: Sven Strickroth <[e-mail]>

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

Signed-off-by: Sven Strickroth <[e-mail]>
---
    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(+)


base-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7

Comments

Patrick Steinhardt Aug. 5, 2024, 8:36 a.m. UTC | #1

On Sat, Aug 03, 2024 at 10:37:51AM +0000, Sven Strickroth via GitGitGadget wrote:
> From: Sven Strickroth <[e-mail]>
> 
> 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 Aug. 5, 2024, 9:45 a.m. UTC | #2

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.

Patch

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,