{"thread":{"id":"64052","subject":"[PATCH/RFC] submodule: gracefully handle links in module paths","startedAt":"2025-08-28T12:43:48Z","lastAt":"2025-08-28T12:43:48Z","messageCount":1,"participants":["Julian Prein via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"525100","messageId":"pull.2041.git.git.1756385026051.gitgitgadget@gmail.com","threadId":"64052","inReplyTo":null,"subject":"[PATCH/RFC] submodule: gracefully handle links in module paths","fromName":"Julian Prein via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-08-28T12:43:46Z","receivedAt":"2025-08-28T12:43:48Z","isPatch":true,"sender":{"key":"julian@druckdev.xyz","avatar":"https://avatars.githubusercontent.com/u/63563978?v=4"},"body":"From: Julian Prein <julian@druck.dev>\n\nDue to security concerns the commit e8d060894448 (submodule: require the\nsubmodule path to contain directories only, 2024-03-26) introduced some\nchecks and hard `exit(128)` calls for the case that a submodule path\ncontains symbolic links. This change was motivated by CVE-2024-32002,\nwhich was discovered shortly before. Consequently, git currently aborts\nactions of which some could be continued by not processing the faulty\nmodule.\n\nAs an example, one of these actions that may be aborted is showing a\ndiff with diff.submodule set to `log` or `diff`: Let's assume a\nrepository where a submodule was moved and its old path replaced with a\nsymlink pointing to the new location. A git-show on an older commit from\nbefore the move that also touches the module will now have only partial\noutput. If the submodule is the first file to list, there will be no\ndiff at all after the commit message. Other examples for actions that\nare aborted after already being started are fetching and pushing with\ntheir respective recurseSubmodules turned on.\n\nHandle these cases more gracefully by returning an error value instead\nof dying, if possible. This was done only in functions that already\nsupported returning an error.\n\nWith this change the mentioned git-show will display the full diff and\nshow a \"(commits not present)\" for the submodule - the same is done\ncurrently when a submodule is simply moved. The fetch and push will\ncomplete their action while skipping the faulty module and printing an\nerror message.\n\nSigned-off-by: Julian Prein <julian@druck.dev>\n---\n    submodule: gracefully handle links in module paths\n    \n    Hello,\n    \n    I'm submitting this patch primarily to fix the issue described with\n    diff.submodule. I maintain a repository in which this kind of move\n    occurred, and I have diff.submodule set to log. With current git I can't\n    properly display some older commits. Changing submodule_to_gitdir would\n    be sufficient to address this, but I noticed more places doing the same\n    thing (exit(128) even though an error value could be returned), so I\n    changed these as well.\n    \n    Since this is security-relevant, and I'm not very familiar with the\n    codebase, I probably lack the foresight to foresee all consequences of\n    this change. For this reason, I'm submitting this as an RFC and without\n    any tests for now. If you deem this patch reasonable, I'd be happy to\n    write tests and add them to a future version.\n    \n    Thanks,\n    \n    Julian\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2041%2Fdruckdev%2Fpr2041-graceful-submodule-links-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2041/druckdev/pr2041-graceful-submodule-links-v1\nPull-Request: https://github.com/git/git/pull/2041\n\n submodule.c | 27 ++++++++++++++++++++-------\n 1 file changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex fff3c75570..843ec48339 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1127,7 +1127,7 @@ static int push_submodule(const char *path,\n \t\t\t  int dry_run)\n {\n \tif (validate_submodule_path(path) < 0)\n-\t\texit(128);\n+\t\treturn 0;\n \n \tif (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1514,10 +1514,19 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf\n {\n \tstruct fetch_task *task;\n \n-\tCALLOC_ARRAY(task, 1);\n+\tif (validate_submodule_path(path) < 0) {\n+\t\t// Add submodule path to the list of submodules with errors.\n+\t\t// validate_submodule_path already printed an error message, but\n+\t\t// it would be confusing if the submodule wouldn't be listed\n+\t\t// together with other erroneous modules at the end.\n+\t\t//\n+\t\t// NEEDSWORK: name instead of path would be nice\n+\t\tspf->result = 1;\n+\t\tstrbuf_addf(&spf->submodules_with_errors, \"\\t%s\\n\", path);\n+\t\treturn NULL;\n+\t}\n \n-\tif (validate_submodule_path(path) < 0)\n-\t\texit(128);\n+\tCALLOC_ARRAY(task, 1);\n \n \ttask->sub = submodule_from_path(spf->r, treeish_name, path);\n \n@@ -1999,8 +2008,12 @@ int bad_to_remove_submodule(const char *path, unsigned flags)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (validate_submodule_path(path) < 0)\n-\t\texit(128);\n+\tif (validate_submodule_path(path) < 0) {\n+\t\tif (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)\n+\t\t\tdie(NULL);\n+\t\tret = -1;\n+\t\tgoto out;\n+\t}\n \n \tif (!file_exists(path) || is_empty_dir(path))\n \t\treturn 0;\n@@ -2555,7 +2568,7 @@ int submodule_to_gitdir(struct repository *repo,\n \tint ret = 0;\n \n \tif (validate_submodule_path(submodule) < 0)\n-\t\texit(128);\n+\t\treturn -1;\n \n \tstrbuf_reset(buf);\n \tstrbuf_addstr(buf, submodule);\n\nbase-commit: f814da676ae46aac5be0a98b99373a76dee6cedb\n-- \ngitgitgadget\n"}]}