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@xxxxxxxx>
> ---
> 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