[PATCH 0/2] fix a leak in submodule error path

[PATCH 0/2] fix a leak in submodule error path

[Jeff King]

This fixes a small leak noticed by Coverity. I think it has been there
for a while, but nearby code movement caused it to be marked as "new".

  [1/2]: repository: make repo_clear() idempotent
  [2/2]: submodule--helper: free URL when repository setup fails

 builtin/submodule--helper.c             | 10 +++++++---
 repository.c                            |  3 ++-
 t/t7426-submodule-get-default-remote.sh | 17 +++++++++++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

-Peff

[PATCH 1/2] repository: make repo_clear() idempotent

[Jeff King]

Calling repo_clear() twice in a row will segfault because the second
call will invoke parse_object_pool_clear() on a NULL pointer. This is
not usually a big deal, but we can make some error cleanup a little
simpler if callers do not need to worry about invoking it twice.

We can fix it by catching the NULL case. The rest of repo_clear()
appears to be idempotent.

Signed-off-by: Jeff King <peff@peff.net>
---
This helps in the next patch, but I think it's also just the path of
least surprise.

Arguably this should not be a pointer at all, but the pool code is
weirdly asymmetric. It offers only "new" which allocates a struct, but
only "clear" to clean it up (but not deallocate). Might be worth fixing,
but out of scope for this series.

 repository.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/repository.c b/repository.c
index db4f9d006e..33ad8984bc 100644
--- a/repository.c
+++ b/repository.c
@@ -385,7 +385,8 @@ void repo_clear(struct repository *repo)
 	odb_free(repo->objects);
 	repo->objects = NULL;
 
-	parsed_object_pool_clear(repo->parsed_objects);
+	if (repo->parsed_objects)
+		parsed_object_pool_clear(repo->parsed_objects);
 	FREE_AND_NULL(repo->parsed_objects);
 
 	repo_settings_clear(repo);
-- 
2.55.0.1067.gf7fc94a55c

Re: [PATCH 1/2] repository: make repo_clear() idempotent

[Jeff King]

On Wed, Sep 02, 2026 at 01:55:27AM -0400, Jeff King wrote:

> Arguably this should not be a pointer at all, but the pool code is
> weirdly asymmetric. It offers only "new" which allocates a struct, but
> only "clear" to clean it up (but not deallocate). Might be worth fixing,
> but out of scope for this series.

I took a quick stab at this, and it gets ugly. There is no mutual
recursion between the parsed_object_pool and repository struct
definitions, but we do end up in a header include loop:

  - repository.h would need object.h (to include the pool struct)

  - object.h includes hash.h for object_id, etc

  - hash.h (sometimes) includes repository.h so it can define
    the_hash_algo when USE_THE_REPOSITORY_VARIABLE is defined

We could break the cycle if we had a separate the-repository.h which
looked like this:

  struct repository;
  extern struct repository *the_repository;

and then included that from hash.h. But then callers which want to use
the_hash_algo would need to include repository.h themselves. It is
just a macro looking at the_repository->hash_algo, so they need the
actual repository definition. About 9 files need to start including
repository.h themselves to make it work. Though a few of them _ought_ to
be including it anyway; they are not using the_hash_algo at all, but
just lucky that hash.h happens to bring repository.h when
USE_THE_REPOSITORY_VARIABLE is set.

An alternative would be to define the_hash_algo as its own pointer,
like:

diff --git a/hash.h b/hash.h
index cf94ad5700..9e21ac6480 100644
--- a/hash.h
+++ b/hash.h
@@ -269,8 +269,7 @@ enum get_oid_result {
 };
 
 #ifdef USE_THE_REPOSITORY_VARIABLE
-# include "repository.h"
-# define the_hash_algo the_repository->hash_algo
+extern struct git_hash_algo *the_hash_algo;
 #endif
 
 /* A suitably aligned type for stack allocations of hash contexts. */
diff --git a/repository.c b/repository.c
index db4f9d006e..f70c4deecf 100644
--- a/repository.c
+++ b/repository.c
@@ -30,6 +30,7 @@ extern struct repository *the_repository;
 /* The main repository */
 static struct repository the_repo;
 struct repository *the_repository = &the_repo;
+struct the_hash_algo = &the_repo->hash_algo;
 
 /*
  * An escape hatch: if we hit a bug in the production code that fails


That makes the_hash_algo just work without most code caring about
repositories at all. But of course it reveals yet more spots which are
relying on hash.h mentioning the_repository. :-/

I'm not sure how much it's worth untangling all of this, but probably
not enough just to remove pointer indirection from repo->parsed_objects.

-Peff

Re: [PATCH 1/2] repository: make repo_clear() idempotent

[Jeff King]

On Wed, Sep 02, 2026 at 02:29:40AM -0400, Jeff King wrote:

> I'm not sure how much it's worth untangling all of this, but probably
> not enough just to remove pointer indirection from repo->parsed_objects.

BTW, another curiosity: parsed_objects contains a pointer back to the
repo that contains it! What could it possibly depend on in the repo
itself?

As far as I can tell, the answer is nothing. We only ever access p->repo
in order to get to p->repo->parsed_objects, which will always be the
same as our original "p". There are some internal functions within
object.c which could be simplified by passing around the
parsed_object_pool directly.  But we also call lookup_commit() and a few
other public functions, all of which take a repository struct. Even
though they only use it to look at the parsed_objects field!

Structurally speaking these should be operating on a parsed_object_pool,
since that's all they need. But from the caller's point of view that is
just an implementation detail, and it is easier to pass in the whole
repository.

So we'd probably need to provide functions that operate directly on the
pool like:

  struct commit *lookup_commit_via_pool(struct parsed_object_pool *p,
                                        const struct object_id *oid);

and then maintain wrappers like:

  struct commit *lookup_commit(struct repository *r,
                               const struct object_id *oid)
  {
	return lookup_commit_via_pool(r->parsed_objects, oid);
  }

to avoid rewriting every caller with r->parsed_objects themselves.

The patch below illustrates the minimal change to drop the repo pointer
from parsed_object_pool. I think it more accurately represents the
actual dependencies of the data structures, but it's a fair bit of churn
for a minor amount of clarity. Probably not worth it.

---
 alloc.c  |  9 ++++--
 alloc.h  |  1 +
 commit.c | 20 +++++++++----
 commit.h |  3 ++
 object.c | 55 ++++++++++++++++++++---------------
 object.h |  3 +-
 6 files changed, 60 insertions(+), 31 deletions(-)

diff --git a/alloc.c b/alloc.c
index 533a045c2a..e7fb90734b 100644
--- a/alloc.c
+++ b/alloc.c
@@ -121,9 +121,14 @@ void init_commit_node(struct commit *c)
 	c->index = alloc_commit_index();
 }
 
-void *alloc_commit_node(struct repository *r)
+void *alloc_commit_node_via_pool(struct parsed_object_pool *p)
 {
-	struct commit *c = alloc_node(r->parsed_objects->commit_state, sizeof(struct commit));
+	struct commit *c = alloc_node(p->commit_state, sizeof(struct commit));
 	init_commit_node(c);
 	return c;
 }
+
+void *alloc_commit_node(struct repository *r)
+{
+	return alloc_commit_node_via_pool(r->parsed_objects);
+}
diff --git a/alloc.h b/alloc.h
index 87a47a9709..70c9c726c1 100644
--- a/alloc.h
+++ b/alloc.h
@@ -11,6 +11,7 @@ void *alloc_blob_node(struct repository *r);
 void *alloc_tree_node(struct repository *r);
 void init_commit_node(struct commit *c);
 void *alloc_commit_node(struct repository *r);
+void *alloc_commit_node_via_pool(struct parsed_object_pool *p);
 void *alloc_tag_node(struct repository *r);
 void *alloc_object_node(struct repository *r);
 
diff --git a/commit.c b/commit.c
index ad26f0b40a..407e11f00b 100644
--- a/commit.c
+++ b/commit.c
@@ -98,14 +98,19 @@ struct commit *lookup_commit_object(struct repository *r,
 
 }
 
-struct commit *lookup_commit(struct repository *r, const struct object_id *oid)
+struct commit *lookup_commit_via_pool(struct parsed_object_pool *p, const struct object_id *oid)
 {
-	struct object *obj = lookup_object(r, oid);
+	struct object *obj = lookup_object_via_pool(p, oid);
 	if (!obj)
-		return create_object(r, oid, alloc_commit_node(r));
+		return create_object_via_pool(p, oid, alloc_commit_node_via_pool(p));
 	return object_as_type(obj, OBJ_COMMIT, 0);
 }
 
+struct commit *lookup_commit(struct repository *r, const struct object_id *oid)
+{
+	return lookup_commit_via_pool(r->parsed_objects, oid);
+}
+
 struct commit *lookup_commit_reference_by_name(const char *name)
 {
 	return lookup_commit_reference_by_name_gently(name, 0);
@@ -206,9 +211,9 @@ int commit_graft_pos(struct repository *r, const struct object_id *oid)
 		       commit_graft_oid_access);
 }
 
-void unparse_commit(struct repository *r, const struct object_id *oid)
+void unparse_commit_via_pool(struct parsed_object_pool *p, const struct object_id *oid)
 {
-	struct commit *c = lookup_commit(r, oid);
+	struct commit *c = lookup_commit_via_pool(p, oid);
 
 	if (!c->object.parsed)
 		return;
@@ -217,6 +222,11 @@ void unparse_commit(struct repository *r, const struct object_id *oid)
 	c->object.parsed = 0;
 }
 
+void unparse_commit(struct repository *repo, const struct object_id *oid)
+{
+	unparse_commit_via_pool(repo->parsed_objects, oid);
+}
+
 int register_commit_graft(struct repository *r, struct commit_graft *graft,
 			  int ignore_dups)
 {
diff --git a/commit.h b/commit.h
index 1061ed791b..972db23558 100644
--- a/commit.h
+++ b/commit.h
@@ -76,6 +76,8 @@ struct commit *lookup_commit_object(struct repository *r, const struct object_id
  * "oid" is not in the object cache.
  */
 struct commit *lookup_commit(struct repository *r, const struct object_id *oid);
+struct commit *lookup_commit_via_pool(struct parsed_object_pool *pool,
+				      const struct object_id *oid);
 struct commit *lookup_commit_reference(struct repository *r,
 				       const struct object_id *oid);
 struct commit *lookup_commit_reference_gently(struct repository *r,
@@ -104,6 +106,7 @@ static inline int repo_parse_commit(struct repository *r, struct commit *item)
 }
 
 void unparse_commit(struct repository *r, const struct object_id *oid);
+void unparse_commit_via_pool(struct parsed_object_pool *p, const struct object_id *oid);
 
 static inline int repo_parse_commit_no_graph(struct repository *r,
 					     struct commit *commit)
diff --git a/object.c b/object.c
index 97f7fc0e87..c6b0815240 100644
--- a/object.c
+++ b/object.c
@@ -89,20 +89,20 @@ static void insert_obj_hash(struct object *obj, struct object **hash, unsigned i
  * Look up the record for the given sha1 in the hash map stored in
  * obj_hash.  Return NULL if it was not found.
  */
-struct object *lookup_object(struct repository *r, const struct object_id *oid)
+struct object *lookup_object_via_pool(struct parsed_object_pool *p, const struct object_id *oid)
 {
 	unsigned int i, first;
 	struct object *obj;
 
-	if (!r->parsed_objects->obj_hash)
+	if (!p->obj_hash)
 		return NULL;
 
-	first = i = hash_obj(oid, r->parsed_objects->obj_hash_size);
-	while ((obj = r->parsed_objects->obj_hash[i]) != NULL) {
+	first = i = hash_obj(oid, p->obj_hash_size);
+	while ((obj = p->obj_hash[i]) != NULL) {
 		if (oideq(oid, &obj->oid))
 			break;
 		i++;
-		if (i == r->parsed_objects->obj_hash_size)
+		if (i == p->obj_hash_size)
 			i = 0;
 	}
 	if (obj && i != first) {
@@ -111,57 +111,66 @@ struct object *lookup_object(struct repository *r, const struct object_id *oid)
 		 * that we do not need to walk the hash table the next
 		 * time we look for it.
 		 */
-		SWAP(r->parsed_objects->obj_hash[i],
-		     r->parsed_objects->obj_hash[first]);
+		SWAP(p->obj_hash[i],
+		     p->obj_hash[first]);
 	}
 	return obj;
 }
 
+struct object *lookup_object(struct repository *r, const struct object_id *oid)
+{
+	return lookup_object_via_pool(r->parsed_objects, oid);
+}
+
 /*
  * Increase the size of the hash map stored in obj_hash to the next
  * power of 2 (but at least 32).  Copy the existing values to the new
  * hash map.
  */
-static void grow_object_hash(struct repository *r)
+static void grow_object_hash(struct parsed_object_pool *p)
 {
 	int i;
 	/*
 	 * Note that this size must always be power-of-2 to match hash_obj
 	 * above.
 	 */
-	int new_hash_size = r->parsed_objects->obj_hash_size < 32 ? 32 : 2 * r->parsed_objects->obj_hash_size;
+	int new_hash_size = p->obj_hash_size < 32 ? 32 : 2 * p->obj_hash_size;
 	struct object **new_hash;
 
 	CALLOC_ARRAY(new_hash, new_hash_size);
-	for (i = 0; i < r->parsed_objects->obj_hash_size; i++) {
-		struct object *obj = r->parsed_objects->obj_hash[i];
+	for (i = 0; i < p->obj_hash_size; i++) {
+		struct object *obj = p->obj_hash[i];
 
 		if (!obj)
 			continue;
 		insert_obj_hash(obj, new_hash, new_hash_size);
 	}
-	free(r->parsed_objects->obj_hash);
-	r->parsed_objects->obj_hash = new_hash;
-	r->parsed_objects->obj_hash_size = new_hash_size;
+	free(p->obj_hash);
+	p->obj_hash = new_hash;
+	p->obj_hash_size = new_hash_size;
 }
 
-void *create_object(struct repository *r, const struct object_id *oid, void *o)
+void *create_object_via_pool(struct parsed_object_pool *p, const struct object_id *oid, void *o)
 {
 	struct object *obj = o;
 
 	obj->parsed = 0;
 	obj->flags = 0;
 	oidcpy(&obj->oid, oid);
 
-	if (r->parsed_objects->obj_hash_size - 1 <= r->parsed_objects->nr_objs * 2)
-		grow_object_hash(r);
+	if (p->obj_hash_size - 1 <= p->nr_objs * 2)
+		grow_object_hash(p);
 
-	insert_obj_hash(obj, r->parsed_objects->obj_hash,
-			r->parsed_objects->obj_hash_size);
-	r->parsed_objects->nr_objs++;
+	insert_obj_hash(obj, p->obj_hash, p->obj_hash_size);
+	p->nr_objs++;
 	return obj;
 }
 
+void *create_object(struct repository *r, const struct object_id *oid, void *o)
+{
+	return create_object_via_pool(r->parsed_objects, oid, o);
+}
+
 void *object_as_type(struct object *obj, enum object_type type, int quiet)
 {
 	if (obj->type == type)
@@ -551,12 +560,12 @@ void repo_clear_commit_marks(struct repository *r, unsigned int flags)
 	}
 }
 
-struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)
+struct parsed_object_pool *parsed_object_pool_new(struct repository *repo
+						  UNUSED)
 {
 	struct parsed_object_pool *o = xmalloc(sizeof(*o));
 	memset(o, 0, sizeof(*o));
 
-	o->repo = repo;
 	o->blob_state = alloc_state_alloc();
 	o->tree_state = alloc_state_alloc();
 	o->commit_state = alloc_state_alloc();
@@ -573,7 +582,7 @@ struct parsed_object_pool *parsed_object_pool_new(struct repository *repo)
 void parsed_object_pool_reset_commit_grafts(struct parsed_object_pool *o)
 {
 	for (int i = 0; i < o->grafts_nr; i++) {
-		unparse_commit(o->repo, &o->grafts[i]->oid);
+		unparse_commit_via_pool(o, &o->grafts[i]->oid);
 		free(o->grafts[i]);
 	}
 	o->grafts_nr = 0;
diff --git a/object.h b/object.h
index 8fb03ff90a..2bab336913 100644
--- a/object.h
+++ b/object.h
@@ -7,7 +7,6 @@ struct buffer_slab;
 struct repository;
 
 struct parsed_object_pool {
-	struct repository *repo;
 	struct object **obj_hash;
 	int nr_objs, obj_hash_size;
 
@@ -191,8 +190,10 @@ struct object *get_indexed_object(const struct repository *repo,
  * by calling parse_object() on them.
  */
 struct object *lookup_object(struct repository *r, const struct object_id *oid);
+struct object *lookup_object_via_pool(struct parsed_object_pool *p, const struct object_id *oid);
 
 void *create_object(struct repository *r, const struct object_id *oid, void *obj);
+void *create_object_via_pool(struct parsed_object_pool *p, const struct object_id *oid, void *obj);
 
 void *object_as_type(struct object *obj, enum object_type type, int quiet);
 

Re: [PATCH 1/2] repository: make repo_clear() idempotent

[Patrick Steinhardt]

On Wed, Sep 02, 2026 at 02:49:07AM -0400, Jeff King wrote:
> On Wed, Sep 02, 2026 at 02:29:40AM -0400, Jeff King wrote:
> 
> > I'm not sure how much it's worth untangling all of this, but probably
> > not enough just to remove pointer indirection from repo->parsed_objects.
> 
> BTW, another curiosity: parsed_objects contains a pointer back to the
> repo that contains it! What could it possibly depend on in the repo
> itself?
> 
> As far as I can tell, the answer is nothing. We only ever access p->repo
> in order to get to p->repo->parsed_objects, which will always be the
> same as our original "p". There are some internal functions within
> object.c which could be simplified by passing around the
> parsed_object_pool directly.  But we also call lookup_commit() and a few
> other public functions, all of which take a repository struct. Even
> though they only use it to look at the parsed_objects field!
> 
> Structurally speaking these should be operating on a parsed_object_pool,
> since that's all they need. But from the caller's point of view that is
> just an implementation detail, and it is easier to pass in the whole
> repository.

I was at one point wondering whether the parsed object pool should
really be an implementation detail of the object database -- parsing
objects should not have to depend on the repository, but it really
should only interact with the object database. I hacked together a
series, but it grew _huge_ because I was of course also trying to bubble
up the changes into all subsystems that do parse objects rigth now. So I
discarded that idea eventually.

But I think making the parsed object pool become more self-contained is
a step into the right direction.

> So we'd probably need to provide functions that operate directly on the
> pool like:
> 
>   struct commit *lookup_commit_via_pool(struct parsed_object_pool *p,
>                                         const struct object_id *oid);
> 
> and then maintain wrappers like:
> 
>   struct commit *lookup_commit(struct repository *r,
>                                const struct object_id *oid)
>   {
> 	return lookup_commit_via_pool(r->parsed_objects, oid);
>   }
> 
> to avoid rewriting every caller with r->parsed_objects themselves.

I dunno. If we want to make this switch I'd say that we should go all or
nothing. Otherwise, if we retain both interfaces, I don't really feel
like it gains us anything at all.

> The patch below illustrates the minimal change to drop the repo pointer
> from parsed_object_pool. I think it more accurately represents the
> actual dependencies of the data structures, but it's a fair bit of churn
> for a minor amount of clarity. Probably not worth it.

I think there is value in it. While deglobalizing our state we tend to
just pass the repository explicitly into the subsystems, which is a good
step. But I think it really should only be the first step, where the
next step would be to reduce the state we pass around. So ideally,
subsystems should really only receive as input what they actually need.
This would eventually ensure that our subsystems are more self-contained 
and that they can be used more flexibly.

Thanks!

Patrick

Re: [PATCH 1/2] repository: make repo_clear() idempotent

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I was at one point wondering whether the parsed object pool should
> really be an implementation detail of the object database -- parsing
> objects should not have to depend on the repository, ...

We need to be a bit careful here, as the above directly contradicts
our earlier design choice to have things like hash algorithm as
properties of a repository instance.

> This would eventually ensure that our subsystems are more self-contained 
> and that they can be used more flexibly.

I do agree with this line of thinking, though.

Thanks.

Re: [PATCH 1/2] repository: make repo_clear() idempotent

[Patrick Steinhardt]

On Wed, Sep 02, 2026 at 09:29:48AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > I was at one point wondering whether the parsed object pool should
> > really be an implementation detail of the object database -- parsing
> > objects should not have to depend on the repository, ...
> 
> We need to be a bit careful here, as the above directly contradicts
> our earlier design choice to have things like hash algorithm as
> properties of a repository instance.

Yeah, true. The question though is whether it really makes sense to
always propagate down the full repository, or whether we should instead
propagate only what matters. We do of course require the hash algorithm
(and potentially the compatibility hash algorithm) in these subsystems,
but propagating these two pieces of information might be the saner
approach compared to always making the full repository available.

In any case, I eventually discarded the above idea anyway.

Patrick

[PATCH 2/2] submodule--helper: free URL when repository setup fails

[Jeff King]

If repo setup fails, we'll return an error without freeing the allocated
url string, leaking the memory. The test suite does trigger this error,
but never with the leak. We only allocate a url if submodule_from_path()
returned something, but our tests use other situations, like totally
nonexistent submodules.

We can cover this case by asking about a submodule that exists but which
has not been initialized. The new test fails with SANITIZE=leak.

The smallest fix would just be a call to free(url), but I think it's a
little nicer to set up a dedicated out-path for cleanup here. The
previous commit made it safe to call repo_clear() even if
repo_submodule_init() fails.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/submodule--helper.c             | 10 +++++++---
 t/t7426-submodule-get-default-remote.sh | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e7cd3225fa..469e3dbcc9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -80,6 +80,7 @@ static int get_default_remote_submodule(const char *module_path, char **default_
 	struct repository subrepo;
 	const char *remote_name = NULL;
 	char *url = NULL;
+	int ret = 0;
 
 	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), module_path);
 	if (sub && sub->url) {
@@ -96,9 +97,11 @@ static int get_default_remote_submodule(const char *module_path, char **default_
 	}
 
 	if (repo_submodule_init(&subrepo, the_repository, module_path,
-				null_oid(the_hash_algo)) < 0)
-		return die_message(_("could not get a repository handle for submodule '%s'"),
+				null_oid(the_hash_algo)) < 0) {
+		ret = die_message(_("could not get a repository handle for submodule '%s'"),
 				   module_path);
+		goto out;
+	}
 
 	/* Look up by URL first */
 	if (url)
@@ -108,10 +111,11 @@ static int get_default_remote_submodule(const char *module_path, char **default_
 
 	*default_remote = xstrdup(remote_name);
 
+out:
 	repo_clear(&subrepo);
 	free(url);
 
-	return 0;
+	return ret;
 }
 
 static int module_get_default_remote(int argc, const char **argv, const char *prefix,
diff --git a/t/t7426-submodule-get-default-remote.sh b/t/t7426-submodule-get-default-remote.sh
index b842af9a2d..0379c9f044 100755
--- a/t/t7426-submodule-get-default-remote.sh
+++ b/t/t7426-submodule-get-default-remote.sh
@@ -60,6 +60,23 @@ test_expect_success 'get-default-remote fails with non-submodule path' '
 	)
 '
 
+test_expect_success 'get-default-remote fails with uninitialized submodule' '
+	test_when_finished "
+		git -C super config -f .gitmodules --remove-section submodule.uninitialized &&
+		git -C super update-index --force-remove uninitialized
+	" &&
+	(
+		cd super &&
+		git config -f .gitmodules submodule.uninitialized.path uninitialized &&
+		git config -f .gitmodules submodule.uninitialized.url ../sub &&
+		head=$(git -C ../sub rev-parse HEAD) &&
+		git update-index --add --cacheinfo 160000,$head,uninitialized &&
+		test_must_fail git submodule--helper get-default-remote \
+			uninitialized 2>err &&
+		test_grep "could not get a repository handle" err
+	)
+'
+
 test_expect_success 'get-default-remote fails without path argument' '
 	(
 		cd super &&
-- 
2.55.0.1067.gf7fc94a55c

Re: [PATCH 2/2] submodule--helper: free URL when repository setup fails

[Patrick Steinhardt]

On Wed, Sep 02, 2026 at 01:57:30AM -0400, Jeff King wrote:
> If repo setup fails, we'll return an error without freeing the allocated
> url string, leaking the memory. The test suite does trigger this error,
> but never with the leak. We only allocate a url if submodule_from_path()
> returned something, but our tests use other situations, like totally
> nonexistent submodules.
> 
> We can cover this case by asking about a submodule that exists but which
> has not been initialized. The new test fails with SANITIZE=leak.
> 
> The smallest fix would just be a call to free(url), but I think it's a
> little nicer to set up a dedicated out-path for cleanup here. The
> previous commit made it safe to call repo_clear() even if
> repo_submodule_init() fails.

Agreed.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/submodule--helper.c             | 10 +++++++---
>  t/t7426-submodule-get-default-remote.sh | 17 +++++++++++++++++
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index e7cd3225fa..469e3dbcc9 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -80,6 +80,7 @@ static int get_default_remote_submodule(const char *module_path, char **default_
>  	struct repository subrepo;
>  	const char *remote_name = NULL;
>  	char *url = NULL;
> +	int ret = 0;
>  
>  	sub = submodule_from_path(the_repository, null_oid(the_hash_algo), module_path);
>  	if (sub && sub->url) {

Nit, feel free to ignore: do we want to keep the value uninitialized
and...

> @@ -96,9 +97,11 @@ static int get_default_remote_submodule(const char *module_path, char **default_
>  	}
>  
>  	if (repo_submodule_init(&subrepo, the_repository, module_path,
> -				null_oid(the_hash_algo)) < 0)
> -		return die_message(_("could not get a repository handle for submodule '%s'"),
> +				null_oid(the_hash_algo)) < 0) {
> +		ret = die_message(_("could not get a repository handle for submodule '%s'"),
>  				   module_path);
> +		goto out;
> +	}
>  
>  	/* Look up by URL first */
>  	if (url)
> @@ -108,10 +111,11 @@ static int get_default_remote_submodule(const char *module_path, char **default_
>  
>  	*default_remote = xstrdup(remote_name);
>  

... set it to 0 here? Many compilers would warn in case the value was
uninitialized, which ensures that the return value is being explicitly
set before every `goto out`.

> +out:
>  	repo_clear(&subrepo);
>  	free(url);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int module_get_default_remote(int argc, const char **argv, const char *prefix,
> diff --git a/t/t7426-submodule-get-default-remote.sh b/t/t7426-submodule-get-default-remote.sh
> index b842af9a2d..0379c9f044 100755
> --- a/t/t7426-submodule-get-default-remote.sh
> +++ b/t/t7426-submodule-get-default-remote.sh
> @@ -60,6 +60,23 @@ test_expect_success 'get-default-remote fails with non-submodule path' '
>  	)
>  '
>  
> +test_expect_success 'get-default-remote fails with uninitialized submodule' '
> +	test_when_finished "
> +		git -C super config -f .gitmodules --remove-section submodule.uninitialized &&
> +		git -C super update-index --force-remove uninitialized
> +	" &&

I was about to say we could use `test_config` instead, but you're of
course not modifying the normal ".git/config" file but ".gitmodules".

> +	(
> +		cd super &&
> +		git config -f .gitmodules submodule.uninitialized.path uninitialized &&
> +		git config -f .gitmodules submodule.uninitialized.url ../sub &&
> +		head=$(git -C ../sub rev-parse HEAD) &&
> +		git update-index --add --cacheinfo 160000,$head,uninitialized &&
> +		test_must_fail git submodule--helper get-default-remote \
> +			uninitialized 2>err &&
> +		test_grep "could not get a repository handle" err
> +	)
> +'

Thanks!

Patrick