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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 5, 2024, 08:36 UTC
Message-ID: <ZrCPBXql7ySbEeXG@tanuki>
In-Reply-To: <pull.1757.git.git.1722681471550.gitgitgadget@gmail.com>

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)

Show 10 quoted lines

> 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