[PATCH] submodule add: sanity check existing .gitmodules

[PATCH] submodule add: sanity check existing .gitmodules

[Junio C Hamano]

"git submodule add" tries to find if a submodule with the same name
already exists at a different path, by looking up an entry in the
.gitmodules file.  If the entry in the file is incomplete, e.g.,
when the submodule.<name>.something variable is defined but there is
no definition of submodule.<name>.path variable, it accessing the
missing .path member of the submodule structure and triggers a
segfault.

A brief audit was done to make sure that the code does not assume
members other than those that are absolutely certain to exist: a
submodule obtained by submodule_from_name() should have .name
member, while a submodule obtained by submodule_from_path() should
also have .path as well as .name member, and we cannot assume
anything else.  Luckily, the module_add() codepath was the only
problematic one.  It is fairly recent code that comes from 1fa06ced
(submodule: prevent overwriting .gitmodules on path reuse,
2025-07-24).

A helper used by update_submodule() seems to assume that its call to
submodule_from_path() always yields a submodule object without a
failure, which seems to rely on the caller's making sure it is the
case.  Leave an assert() with a NEEDSWORK comment there for future
developers to make sure the assumption actually holds.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 12 ++++++++++--
 t/t7400-submodule-basic.sh  | 19 +++++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 07a1935cbe..1a1043cdab 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1913,6 +1913,13 @@ static int determine_submodule_update_strategy(struct repository *r,
 	const char *val;
 	int ret;
 
+	/*
+	 * NEEDSWORK: audit and ensure that update_submodule() has right
+	 * to assume that submodule_from_path() above will always succeed.
+	 */
+	if (!sub)
+		BUG("update_submodule assumes a submodule exists at path (%s)",
+		    path);
 	key = xstrfmt("submodule.%s.update", sub->name);
 
 	if (update) {
@@ -3537,14 +3544,15 @@ static int module_add(int argc, const char **argv, const char *prefix,
 		}
 	}
 
-	if(!add_data.sm_name)
+	if (!add_data.sm_name)
 		add_data.sm_name = add_data.sm_path;
 
 	existing = submodule_from_name(the_repository,
 					null_oid(the_hash_algo),
 					add_data.sm_name);
 
-	if (existing && strcmp(existing->path, add_data.sm_path)) {
+	if (existing && existing->path &&
+	    strcmp(existing->path, add_data.sm_path)) {
 		if (!force) {
 			die(_("submodule name '%s' already used for path '%s'"),
 			    add_data.sm_name, existing->path);
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index fd3e7e355e..9ade97e432 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -48,6 +48,25 @@ test_expect_success 'submodule deinit works on empty repository' '
 	git submodule deinit --all
 '
 
+test_expect_success 'submodule add with incomplete .gitmodules' '
+	test_when_finished "rm -f expect actual" &&
+	test_when_finished "git config remove-section submodule.one" &&
+	test_when_finished "git rm -f one .gitmodules" &&
+	git init one &&
+	git -C one commit --allow-empty -m one-initial &&
+	git config -f .gitmodules submodule.one.ignore all &&
+
+	git submodule add ./one &&
+
+	for var in ignore path url
+	do
+		git config -f .gitmodules --get "submodule.one.$var" ||
+		return 1
+	done >actual &&
+	test_write_lines all one ./one >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'setup - initial commit' '
 	>t &&
 	git add t &&
-- 
2.52.0-rc2-455-g230fcf2819

Re: [PATCH] submodule add: sanity check existing .gitmodules

[Elijah Newren]

On Sat, Nov 15, 2025 at 11:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "git submodule add" tries to find if a submodule with the same name
> already exists at a different path, by looking up an entry in the
> .gitmodules file.  If the entry in the file is incomplete, e.g.,
> when the submodule.<name>.something variable is defined but there is
> no definition of submodule.<name>.path variable, it accessing the

accessing => tries to access
  (or accessing => accesses)

> missing .path member of the submodule structure and triggers a
> segfault.
>
> A brief audit was done to make sure that the code does not assume
> members other than those that are absolutely certain to exist: a
> submodule obtained by submodule_from_name() should have .name
> member, while a submodule obtained by submodule_from_path() should
> also have .path as well as .name member, and we cannot assume
> anything else.  Luckily, the module_add() codepath was the only
> problematic one.  It is fairly recent code that comes from 1fa06ced
> (submodule: prevent overwriting .gitmodules on path reuse,
> 2025-07-24).
>
> A helper used by update_submodule() seems to assume that its call to
> submodule_from_path() always yields a submodule object without a
> failure, which seems to rely on the caller's making sure it is the

caller's => caller ?

> case.  Leave an assert() with a NEEDSWORK comment there for future
> developers to make sure the assumption actually holds.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH] submodule add: sanity check existing .gitmodules

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> no definition of submodule.<name>.path variable, it accessing the
>
> accessing => tries to access
>   (or accessing => accesses)

>> A helper used by update_submodule() seems to assume that its call to
>> submodule_from_path() always yields a submodule object without a
>> failure, which seems to rely on the caller's making sure it is the
>
> caller's => caller ?

Thanks.