repository: prevent memory leak when releasing ref stores

PATCH In review Sven Strickroth via GitGitGadget 4 participants 7 messages Aug 5, 2024 – Aug 6, 2024

lore

Share & tools

lore

Sven Strickroth via GitGitGadget

author

2024-08-05 10:56 UTC #0 lore raw

PATCH 1/1 repository: prevent memory leak when releasing ref stores

From: Sven Strickroth <[email protected]>

ref_store_release does not free the ref_store allocated in ref_store_init.

Signed-off-by Sven Strickroth <[email protected]>

---
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(-)

repository.c +6 -2

index 9825a308993..46f1eadfe95 100644
@@ -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

Signature · 1 line

gitgitgadget

Sven Strickroth

2024-08-05 15:50 UTC #1 lore raw

Am 05.08.2024 um 12:56 schrieb Sven Strickroth via GitGitGadget:

repository.c void repo_clear(struct repository *repo) · line 373

Quoted 6 lines

repository.c +3 -1

-	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);

After further checking this does not seem to be necessary. The ref stores are already free'd in strmap_clear.

Signature · 3 lines

Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

Junio C Hamano

2024-08-05 16:28 UTC #3 lore raw

In reply to Sven Strickroth via GitGitGadget

Quoted 7 lines · Sven Strickroth via GitGitGadget

From: Sven Strickroth <[email protected]>
ref_store_release does not free the ref_store allocated in ref_store_init.
Signed-off-by Sven Strickroth <[email protected]>
---

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.

Quoted 34 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(-)
repository.c +6 -2
index 9825a308993..46f1eadfe95 100644
@@ -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

Junio C Hamano

2024-08-05 17:24 UTC #4 lore raw

Quoted 21 lines · Junio C Hamano

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);
        }

Quoted 7 lines

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.

Ahh, that was the leak that you plugged in a separate patch.

So it does point us in the other direction to redefine _release with a different behaviour that releases the resource held by the structure, and frees the structure itself.

Something along the following line (caution: totally untested) that allows your two patches to become empty, and also allows a few callers to lose their existing explicit free()s immediately after they call _release(), perhaps?

If this were to become a real patch, I think debug backend should learn to use the same _downcast() to become more like the real ones before it happens in a preliminary clean-up patch.

 refs.h                  |  5 +++--
 refs.c                  | 19 ++++++++-----------
 refs/refs-internal.h    |  2 +-
 refs/files-backend.c    |  6 +++---
 refs/packed-backend.c   |  5 +++--
 refs/reftable-backend.c |  6 +++---
 refs/debug.c            |  6 +++---
 7 files changed, 24 insertions(+), 25 deletions(-)

refs.h +3 -2

index b3e39bc257..e4f092f6ac 100644
@@ -119,9 +119,10 @@ int is_branch(const char *refname);
 int ref_store_create_on_disk(struct ref_store *refs, int flags, struct strbuf *err);
 
 /*
- * Release all memory and resources associated with the ref store.
+ * Release all memory and resources associated with the ref store, including
+ * the ref_store itself.
  */
-void ref_store_release(struct ref_store *ref_store);
+void ref_store_release(struct ref_store **ref_store);
 
 /*
  * Remove the ref store from disk. This deletes all associated data.

refs.c +8 ~~-11~~

index 915aeb4d1d..cb76a5d4bd 100644
@@ -1936,10 +1936,11 @@ static struct ref_store *ref_store_init(struct repository *repo,
 	return refs;
 }
 
-void ref_store_release(struct ref_store *ref_store)
+void ref_store_release(struct ref_store **ref_store)
 {
-	ref_store->be->release(ref_store);
-	free(ref_store->gitdir);
+	(*ref_store)->be->release(ref_store);
+	free((*ref_store)->gitdir);
+	FREE_AND_NULL(*ref_store);
 }
 
 struct ref_store *get_main_ref_store(struct repository *r)

@@ -2848,8 +2849,7 @@ int repo_migrate_ref_storage_format(struct repository *repo,
 	 * be closed. This is required for platforms like Cygwin, where
 	 * renaming an open file results in EPERM.
 	 */
-	ref_store_release(new_refs);
-	FREE_AND_NULL(new_refs);
+	ref_store_release(&new_refs);
 
 	/*
 	 * Until now we were in the non-destructive phase, where we only

@@ -2887,8 +2887,7 @@ int repo_migrate_ref_storage_format(struct repository *repo,
 	 * make sure to lazily re-initialize the repository's ref store with
 	 * the new format.
 	 */
-	ref_store_release(old_refs);
-	FREE_AND_NULL(old_refs);
+	ref_store_release(&old_refs);
 	repo->refs_private = NULL;
 
 	ret = 0;

@@ -2900,10 +2899,8 @@ int repo_migrate_ref_storage_format(struct repository *repo,
 			    new_gitdir.buf);
 	}
 
-	if (new_refs) {
-		ref_store_release(new_refs);
-		free(new_refs);
-	}
+	if (new_refs)
+		ref_store_release(&new_refs);
 	ref_transaction_free(transaction);
 	strbuf_release(&new_gitdir);
 	return ret;

refs/refs-internal.h +1 -1

index fa975d69aa..2ba2372acb 100644
@@ -511,7 +511,7 @@ typedef struct ref_store *ref_store_init_fn(struct repository *repo,
 /*
  * Release all memory and resources associated with the ref store.
  */
-typedef void ref_store_release_fn(struct ref_store *refs);
+typedef void ref_store_release_fn(struct ref_store **refs);
 
 typedef int ref_store_create_on_disk_fn(struct ref_store *refs,
 					int flags,

refs/files-backend.c +3 -3

index aa52d9be7c..8ebb1681ac 100644
@@ -151,12 +151,12 @@ static struct files_ref_store *files_downcast(struct ref_store *ref_store,
 	return refs;
 }
 
-static void files_ref_store_release(struct ref_store *ref_store)
+static void files_ref_store_release(struct ref_store **ref_store)
 {
-	struct files_ref_store *refs = files_downcast(ref_store, 0, "release");
+	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);
+	ref_store_release(&refs->packed_ref_store);
 }
 
 static void files_reflog_path(struct files_ref_store *refs,

refs/packed-backend.c +3 -2

index a0666407cd..8321a4cc17 100644
@@ -260,13 +260,14 @@ static void clear_snapshot(struct packed_ref_store *refs)
 	}
 }
 
-static void packed_ref_store_release(struct ref_store *ref_store)
+static void packed_ref_store_release(struct ref_store **ref_store)
 {
-	struct packed_ref_store *refs = packed_downcast(ref_store, 0, "release");
+	struct packed_ref_store *refs = packed_downcast(*ref_store, 0, "release");
 	clear_snapshot(refs);
 	rollback_lock_file(&refs->lock);
 	delete_tempfile(&refs->tempfile);
 	free(refs->path);
+	FREE_AND_NULL(*ref_store);
 }
 
 static NORETURN void die_unterminated_line(const char *path,

refs/reftable-backend.c +3 -3

index fbe74c239d..0af010acfb 100644
@@ -337,9 +337,9 @@ static struct ref_store *reftable_be_init(struct repository *repo,
 	return &refs->base;
 }
 
-static void reftable_be_release(struct ref_store *ref_store)
+static void reftable_be_release(struct ref_store **ref_store)
 {
-	struct reftable_ref_store *refs = reftable_be_downcast(ref_store, 0, "release");
+	struct reftable_ref_store *refs = reftable_be_downcast(*ref_store, 0, "release");
 	struct strmap_entry *entry;
 	struct hashmap_iter iter;

@@ -400,7 +400,7 @@ static int reftable_be_remove_on_disk(struct ref_store *ref_store,
 	 * required so that the "tables.list" file is not open anymore, which
 	 * would otherwise make it impossible to remove the file on Windows.
 	 */
-	reftable_be_release(ref_store);
+	reftable_be_release(&ref_store);
 
 	strbuf_addf(&sb, "%s/reftable", refs->base.gitdir);
 	if (remove_dir_recursively(&sb, 0) < 0) {

refs/debug.c +3 -3

index 547d9245b9..be6230045c 100644
@@ -33,10 +33,10 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor
 	return (struct ref_store *)res;
 }
 
-static void debug_release(struct ref_store *refs)
+static void debug_release(struct ref_store **refs)
 {
-	struct debug_ref_store *drefs = (struct debug_ref_store *)refs;
-	drefs->refs->be->release(drefs->refs);
+	struct debug_ref_store *drefs = *(struct debug_ref_store **)refs;
+	drefs->refs->be->release(&drefs->refs);
 	trace_printf_key(&trace_refs, "release\n");
 }

Junio C Hamano

2024-08-05 17:42 UTC #2 lore raw

In reply to Sven Strickroth

Quoted 10 lines · Sven Strickroth

Am 05.08.2024 um 12:56 schrieb Sven Strickroth via GitGitGadget:
repository.c void repo_clear(struct repository *repo) · line 373
Quoted 6 lines
repository.c +3 -1
-	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);
After further checking this does not seem to be necessary. The ref stores are already free'd in strmap_clear.

Is it "not necessary" or "actively harmful"? It sounds like the latter?

Patrick Steinhardt

2024-08-06 06:20 UTC #5 lore raw

In reply to Junio C Hamano

Quoted 33 lines · Junio C Hamano

Quoted 21 lines · Junio C Hamano
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);
        }
Quoted 7 lines
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.
Ahh, that was the leak that you plugged in a separate patch.

Quoted 8 lines

So it does point us in the other direction to redefine _release with a different behaviour that releases the resource held by the structure, and frees the structure itself.
Something along the following line (caution: totally untested) that allows your two patches to become empty, and also allows a few callers to lose their existing explicit free()s immediately after they call _release(), perhaps?

I don't really know whether it's worth the churn, but if somebody wants to pull through with this I'm game :) But: if we are going to do this, we should rename the function to be called ref_store_free() instead of ref_store_release() according to our recent coding style update :)

Quoted 3 lines

If this were to become a real patch, I think debug backend should learn to use the same _downcast() to become more like the real ones before it happens in a preliminary clean-up patch.

That certainly wouldn't hurt, yeah.

Patrick

Junio C Hamano

2024-08-06 15:44 UTC #6 lore raw

Quoted 9 lines · Patrick Steinhardt

Quoted 4 lines
Something along the following line (caution: totally untested) that allows your two patches to become empty, and also allows a few callers to lose their existing explicit free()s immediately after they call _release(), perhaps?
I don't really know whether it's worth the churn, but if somebody wants to pull through with this I'm game :) But: if we are going to do this, we should rename the function to be called ref_store_free() instead of ref_store_release() according to our recent coding style update :)

Yes, we had "what is release, clear, and free?" discussion recently.

Quoted 5 lines

Quoted 3 lines
If this were to become a real patch, I think debug backend should learn to use the same _downcast() to become more like the real ones before it happens in a preliminary clean-up patch.
That certainly wouldn't hurt, yeah.

I am not short of (other) things to do, and expect that I will not touching this for a while, but in case somebody finds #leftoverbits here, I'll leave a note here.

There are "hidden" freeing that we have to adjust, if we were to follow through this approach. For example, those free()'s added in the patch in the message that started the thread are introduing double free---after the strmap_for_each_entry() loop, a strmap_clear() callis done with free_values=1. If we freed inside ref_store_release(), we'd need to adjust the call to strmap_clear() to pass free_values=0 to compensate.