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

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

Junio C Hamano <gitster@pobox.com> writes:

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

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

diff --git c/refs.h w/refs.h
index b3e39bc257..e4f092f6ac 100644
--- c/refs.h
+++ w/refs.h
@@ -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.
diff --git c/refs.c w/refs.c
index 915aeb4d1d..cb76a5d4bd 100644
--- c/refs.c
+++ w/refs.c
@@ -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;
diff --git c/refs/refs-internal.h w/refs/refs-internal.h
index fa975d69aa..2ba2372acb 100644
--- c/refs/refs-internal.h
+++ w/refs/refs-internal.h
@@ -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,
diff --git c/refs/files-backend.c w/refs/files-backend.c
index aa52d9be7c..8ebb1681ac 100644
--- c/refs/files-backend.c
+++ w/refs/files-backend.c
@@ -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,
diff --git c/refs/packed-backend.c w/refs/packed-backend.c
index a0666407cd..8321a4cc17 100644
--- c/refs/packed-backend.c
+++ w/refs/packed-backend.c
@@ -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,
diff --git c/refs/reftable-backend.c w/refs/reftable-backend.c
index fbe74c239d..0af010acfb 100644
--- c/refs/reftable-backend.c
+++ w/refs/reftable-backend.c
@@ -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) {
diff --git c/refs/debug.c w/refs/debug.c
index 547d9245b9..be6230045c 100644
--- c/refs/debug.c
+++ w/refs/debug.c
@@ -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");
 }