"Sven Strickroth via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Sven Strickroth <email@xxxxxxxxxx>
>
> `ref_store_release` does not free the ref_store allocated in
> `ref_store_init`.
>
> Signed-off-by: Sven Strickroth <email@xxxxxxxxxx>
> ---
This may certainly plug the two leaking callers, but stepping back a
bit and looking at other existing calls to ref_store_release(), I
wonder if many existing and more importantly future callers benefit
if ref_store_release() did the freeing of the surrounding shell, as
we can see in these existing calls:
refs.c:2851: ref_store_release(new_refs);
refs.c-2852- FREE_AND_NULL(new_refs);
refs.c:2890: ref_store_release(old_refs);
refs.c-2891- FREE_AND_NULL(old_refs);
refs.c:2904: ref_store_release(new_refs);
refs.c-2905- free(new_refs);
If we change the type of ref_store_release() to take a pointer to a
pointer to ref_store, so that the above callers can just become
ref_store_release(&new_refs);
to release the resources and new_refs variable cleared, the
callsites in this patch can do the same.
However, I am fuzzy on the existing uses in the backend
implementation. For example:
static void files_ref_store_release(struct ref_store *ref_store)
{
struct files_ref_store *refs = files_downcast(ref_store, 0, "release");
free_ref_cache(refs->loose);
free(refs->gitcommondir);
ref_store_release(refs->packed_ref_store);
}
The packed-ref-store is "released" here, as part of "releasing" the
files-ref-store that uses it as a fallback backend. The caller of
files_ref_store_release() is refs.c:ref_store_release()
void ref_store_release(struct ref_store *ref_store)
{
ref_store->be->release(ref_store);
free(ref_store->gitdir);
}
So if you have a files based ref store, when you are done you'd be
calling ref_store_release() on it, releasing the resources held by
the files_ref_store structure, but I do not know who frees the
packed_ref_store allocated by files_ref_store_init(). Perhaps it is
already leaking? If that is the case then an API update like I
suggested above would make even more sense to make it less likely
for such a leak to be added to the system in the future, I suspect.
I dunno.
Thanks.
> repository: prevent memory leak when releasing ref stores
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1758%2Fcsware%2Frepository-memory-leak-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1758/csware/repository-memory-leak-v1
> Pull-Request: https://github.com/git/git/pull/1758
>
> repository.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/repository.c b/repository.c
> index 9825a308993..46f1eadfe95 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -366,12 +366,16 @@ void repo_clear(struct repository *repo)
> FREE_AND_NULL(repo->remote_state);
> }
>
> - strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e)
> + strmap_for_each_entry(&repo->submodule_ref_stores, &iter, e) {
> ref_store_release(e->value);
> + free(e->value);
> + }
> strmap_clear(&repo->submodule_ref_stores, 1);
>
> - strmap_for_each_entry(&repo->worktree_ref_stores, &iter, e)
> + strmap_for_each_entry(&repo->worktree_ref_stores, &iter, e) {
> ref_store_release(e->value);
> + free(e->value);
> + }
> strmap_clear(&repo->worktree_ref_stores, 1);
>
> repo_clear_path_cache(&repo->cached_paths);
>
> base-commit: e559c4bf1a306cf5814418d318cc0fea070da3c7
