Re: [PATCH 2/2] submodule--helper: free URL when repository setup fails
From: Patrick Steinhardt <hidden>
Date: 2026-09-02 09:11:33
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Jeff King <redacted> --- 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...
quoted hunk ↗ jump to hunk
@@ -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`.
quoted hunk ↗ jump to hunk
+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