{"thread":{"id":"64486","subject":"[PATCH] submodule add: sanity check existing .gitmodules","startedAt":"2025-11-16T07:03:00Z","lastAt":"2025-11-25T14:33:14Z","messageCount":3,"participants":["Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530768","messageId":"xmqqv7jacvdq.fsf@gitster.g","threadId":"64486","inReplyTo":null,"subject":"[PATCH] submodule add: sanity check existing .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-16T07:02:57Z","receivedAt":"2025-11-16T07:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git submodule add\" tries to find if a submodule with the same name\nalready exists at a different path, by looking up an entry in the\n.gitmodules file. If the entry in the file is incomplete, e.g.,\nwhen the submodule..something variable is defined but there is\nno definition of submodule..path variable, it accessing the\nmissing .path member of the submodule structure and triggers a\nsegfault.\n\nA brief audit was done to make sure that the code does not assume\nmembers other than those that are absolutely certain to exist: a\nsubmodule obtained by submodule_from_name() should have .name\nmember, while a submodule obtained by submodule_from_path() should\nalso have .path as well as .name member, and we cannot assume\nanything else. Luckily, the module_add() codepath was the only\nproblematic one. It is fairly recent code that comes from 1fa06ced\n(submodule: prevent overwriting .gitmodules on path reuse,\n2025-07-24).\n\nA helper used by update_submodule() seems to assume that its call to\nsubmodule_from_path() always yields a submodule object without a\nfailure, which seems to rely on the caller's making sure it is the\ncase. Leave an assert() with a NEEDSWORK comment there for future\ndevelopers to make sure the assumption actually holds.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 12 ++++++++++--\n t/t7400-submodule-basic.sh | 19 +++++++++++++++++++\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 07a1935cbe..1a1043cdab 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1913,6 +1913,13 @@ static int determine_submodule_update_strategy(struct repository *r,\n \tconst char *val;\n \tint ret;\n \n+\t/*\n+\t * NEEDSWORK: audit and ensure that update_submodule() has right\n+\t * to assume that submodule_from_path() above will always succeed.\n+\t */\n+\tif (!sub)\n+\t\tBUG(\"update_submodule assumes a submodule exists at path (%s)\",\n+\t\t path);\n \tkey = xstrfmt(\"submodule.%s.update\", sub->name);\n \n \tif (update) {\n@@ -3537,14 +3544,15 @@ static int module_add(int argc, const char **argv, const char *prefix,\n \t\t}\n \t}\n \n-\tif(!add_data.sm_name)\n+\tif (!add_data.sm_name)\n \t\tadd_data.sm_name = add_data.sm_path;\n \n \texisting = submodule_from_name(the_repository,\n \t\t\t\t\tnull_oid(the_hash_algo),\n \t\t\t\t\tadd_data.sm_name);\n \n-\tif (existing && strcmp(existing->path, add_data.sm_path)) {\n+\tif (existing && existing->path &&\n+\t strcmp(existing->path, add_data.sm_path)) {\n \t\tif (!force) {\n \t\t\tdie(_(\"submodule name '%s' already used for path '%s'\"),\n \t\t\t add_data.sm_name, existing->path);\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex fd3e7e355e..9ade97e432 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -48,6 +48,25 @@ test_expect_success 'submodule deinit works on empty repository' '\n \tgit submodule deinit --all\n '\n \n+test_expect_success 'submodule add with incomplete .gitmodules' '\n+\ttest_when_finished \"rm -f expect actual\" &&\n+\ttest_when_finished \"git config remove-section submodule.one\" &&\n+\ttest_when_finished \"git rm -f one .gitmodules\" &&\n+\tgit init one &&\n+\tgit -C one commit --allow-empty -m one-initial &&\n+\tgit config -f .gitmodules submodule.one.ignore all &&\n+\n+\tgit submodule add ./one &&\n+\n+\tfor var in ignore path url\n+\tdo\n+\t\tgit config -f .gitmodules --get \"submodule.one.$var\" ||\n+\t\treturn 1\n+\tdone >actual &&\n+\ttest_write_lines all one ./one >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup - initial commit' '\n \t>t &&\n \tgit add t &&\n-- \n2.52.0-rc2-455-g230fcf2819\n\n"},{"id":"531253","messageId":"CABPp-BES6HBGxXKC9sfBHu_5oBEDYD+aDquHtoDSZtZdaqOMBQ@mail.gmail.com","threadId":"64486","inReplyTo":"xmqqv7jacvdq.fsf@gitster.g","subject":"Re: [PATCH] submodule add: sanity check existing .gitmodules","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-11-25T06:49:10Z","receivedAt":"2025-11-25T06:49:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Nov 15, 2025 at 11:03 PM Junio C Hamano wrote:\n>\n> \"git submodule add\" tries to find if a submodule with the same name\n> already exists at a different path, by looking up an entry in the\n> .gitmodules file. If the entry in the file is incomplete, e.g.,\n> when the submodule..something variable is defined but there is\n> no definition of submodule..path variable, it accessing the\n\naccessing => tries to access\n (or accessing => accesses)\n\n> missing .path member of the submodule structure and triggers a\n> segfault.\n>\n> A brief audit was done to make sure that the code does not assume\n> members other than those that are absolutely certain to exist: a\n> submodule obtained by submodule_from_name() should have .name\n> member, while a submodule obtained by submodule_from_path() should\n> also have .path as well as .name member, and we cannot assume\n> anything else. Luckily, the module_add() codepath was the only\n> problematic one. It is fairly recent code that comes from 1fa06ced\n> (submodule: prevent overwriting .gitmodules on path reuse,\n> 2025-07-24).\n>\n> A helper used by update_submodule() seems to assume that its call to\n> submodule_from_path() always yields a submodule object without a\n> failure, which seems to rely on the caller's making sure it is the\n\ncaller's => caller ?\n\n> case. Leave an assert() with a NEEDSWORK comment there for future\n> developers to make sure the assumption actually holds.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"531260","messageId":"xmqqfra2tc60.fsf@gitster.g","threadId":"64486","inReplyTo":"CABPp-BES6HBGxXKC9sfBHu_5oBEDYD+aDquHtoDSZtZdaqOMBQ@mail.gmail.com","subject":"Re: [PATCH] submodule add: sanity check existing .gitmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-25T14:33:11Z","receivedAt":"2025-11-25T14:33:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> no definition of submodule..path variable, it accessing the\n>\n> accessing => tries to access\n> (or accessing => accesses)\n\n>> A helper used by update_submodule() seems to assume that its call to\n>> submodule_from_path() always yields a submodule object without a\n>> failure, which seems to rely on the caller's making sure it is the\n>\n> caller's => caller ?\n\nThanks.\n"}]}