# [PATCH] submodule add: sanity check existing .gitmodules 3 messages from 2025-11-16 to 2025-11-25. Participants: Junio C Hamano, Elijah Newren. Thread: https://gitlist.dev/t/64486 ## Junio C Hamano, 2025-11-16 07:02 Subject: [PATCH] submodule add: sanity check existing .gitmodules Message-ID: URL: https://gitlist.dev/e/xmqqv7jacvdq.fsf%40gitster.g ``` "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..something variable is defined but there is no definition of submodule..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 --- 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 ``` ## Elijah Newren, 2025-11-25 06:49 Subject: Re: [PATCH] submodule add: sanity check existing .gitmodules Message-ID: URL: https://gitlist.dev/e/CABPp-BES6HBGxXKC9sfBHu_5oBEDYD%2BaDquHtoDSZtZdaqOMBQ%40mail.gmail.com In-Reply-To: ``` On Sat, Nov 15, 2025 at 11:03 PM Junio C Hamano 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..something variable is defined but there is > no definition of submodule..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 ``` ## Junio C Hamano, 2025-11-25 14:33 Subject: Re: [PATCH] submodule add: sanity check existing .gitmodules Message-ID: URL: https://gitlist.dev/e/xmqqfra2tc60.fsf%40gitster.g In-Reply-To: ``` Elijah Newren writes: >> no definition of submodule..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. ```