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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2024, 16:28 UTC
Message-ID: <xmqq34nj3pez.fsf@gitster.g>
In-Reply-To: <pull.1758.git.git.1722855364436.gitgitgadget@gmail.com>

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

Show 7 quoted lines

> 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.

Show 34 quoted lines

>     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