Re: [PATCH] repository: prevent memory leak when releasing ref stores

[Junio C Hamano <gitster@pobox.com>]

"Sven Strickroth via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sven Strickroth <email@cs-ware.de>
>
> `ref_store_release` does not free the ref_store allocated in
> `ref_store_init`.
>
> Signed-off-by: Sven Strickroth <email@cs-ware.de>
> ---

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