[PATCH/RFC] submodule: gracefully handle links in module paths

From: Julian Prein via GitGitGadget <gitgitgadget@gmail.com>
Date: Aug 28, 2025, 12:43 UTC
Message-ID: <pull.2041.git.git.1756385026051.gitgitgadget@gmail.com>

From: Julian Prein <julian@druck.dev>

Due to security concerns the commit e8d060894448 (submodule: require the submodule path to contain directories only, 2024-03-26) introduced some checks and hard `exit(128)` calls for the case that a submodule path contains symbolic links. This change was motivated by CVE-2024-32002, which was discovered shortly before. Consequently, git currently aborts actions of which some could be continued by not processing the faulty module.

As an example, one of these actions that may be aborted is showing a diff with diff.submodule set to `log` or `diff`: Let's assume a repository where a submodule was moved and its old path replaced with a symlink pointing to the new location. A git-show on an older commit from before the move that also touches the module will now have only partial output. If the submodule is the first file to list, there will be no diff at all after the commit message. Other examples for actions that are aborted after already being started are fetching and pushing with their respective recurseSubmodules turned on.

Handle these cases more gracefully by returning an error value instead of dying, if possible. This was done only in functions that already supported returning an error.

With this change the mentioned git-show will display the full diff and show a "(commits not present)" for the submodule - the same is done currently when a submodule is simply moved. The fetch and push will complete their action while skipping the faulty module and printing an error message.

Signed-off-by: Julian Prein <julian@druck.dev>
---
    submodule: gracefully handle links in module paths
    
    Hello,
    
    I'm submitting this patch primarily to fix the issue described with
    diff.submodule. I maintain a repository in which this kind of move
    occurred, and I have diff.submodule set to log. With current git I can't
    properly display some older commits. Changing submodule_to_gitdir would
    be sufficient to address this, but I noticed more places doing the same
    thing (exit(128) even though an error value could be returned), so I
    changed these as well.
    
    Since this is security-relevant, and I'm not very familiar with the
    codebase, I probably lack the foresight to foresee all consequences of
    this change. For this reason, I'm submitting this as an RFC and without
    any tests for now. If you deem this patch reasonable, I'd be happy to
    write tests and add them to a future version.
    
    Thanks,
    
    Julian

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2041%2Fdruckdev%2Fpr2041-graceful-submodule-links-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2041/druckdev/pr2041-graceful-submodule-links-v1
Pull-Request: https://github.com/git/git/pull/2041

 submodule.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/submodule.c b/submodule.c
index fff3c75570..843ec48339 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1127,7 +1127,7 @@ static int push_submodule(const char *path,
 			  int dry_run)
 {
 	if (validate_submodule_path(path) < 0)
-		exit(128);
+		return 0;
 
 	if (for_each_remote_ref_submodule(path, has_remote, NULL) > 0) {
 		struct child_process cp = CHILD_PROCESS_INIT;
@@ -1514,10 +1514,19 @@ static struct fetch_task *fetch_task_create(struct submodule_parallel_fetch *spf
 {
 	struct fetch_task *task;
 
-	CALLOC_ARRAY(task, 1);
+	if (validate_submodule_path(path) < 0) {
+		// Add submodule path to the list of submodules with errors.
+		// validate_submodule_path already printed an error message, but
+		// it would be confusing if the submodule wouldn't be listed
+		// together with other erroneous modules at the end.
+		//
+		// NEEDSWORK: name instead of path would be nice
+		spf->result = 1;
+		strbuf_addf(&spf->submodules_with_errors, "\t%s\n", path);
+		return NULL;
+	}
 
-	if (validate_submodule_path(path) < 0)
-		exit(128);
+	CALLOC_ARRAY(task, 1);
 
 	task->sub = submodule_from_path(spf->r, treeish_name, path);
 
@@ -1999,8 +2008,12 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
 	struct strbuf buf = STRBUF_INIT;
 	int ret = 0;
 
-	if (validate_submodule_path(path) < 0)
-		exit(128);
+	if (validate_submodule_path(path) < 0) {
+		if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
+			die(NULL);
+		ret = -1;
+		goto out;
+	}
 
 	if (!file_exists(path) || is_empty_dir(path))
 		return 0;
@@ -2555,7 +2568,7 @@ int submodule_to_gitdir(struct repository *repo,
 	int ret = 0;
 
 	if (validate_submodule_path(submodule) < 0)
-		exit(128);
+		return -1;
 
 	strbuf_reset(buf);
 	strbuf_addstr(buf, submodule);

base-commit: f814da676ae46aac5be0a98b99373a76dee6cedb

-- 
gitgitgadget