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

[Patrick Steinhardt <ps@pks.im>]

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