Skip to content

Conversation

druckdev
Copy link
Contributor

@druckdev druckdev commented Aug 26, 2025

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

CC: Johannes Schindelin johannes.schindelin@gmx.de

@druckdev druckdev marked this pull request as draft August 26, 2025 14:42
@druckdev
Copy link
Contributor Author

/preview

Copy link

Preview email sent as pull.2041.git.git.1756335079502.gitgitgadget@gmail.com

@druckdev
Copy link
Contributor Author

/preview

Copy link

Preview email sent as pull.2041.git.git.1756336187268.gitgitgadget@gmail.com

@bowleggedboot
Copy link

bowleggedboot commented Aug 28, 2025 via email

@bowleggedboot
Copy link

bowleggedboot commented Aug 28, 2025 via email

@bowleggedboot
Copy link

bowleggedboot commented Aug 28, 2025 via email

Due to security concerns the commit e8d0608 (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>
@druckdev druckdev force-pushed the pr2041-graceful-submodule-links branch from 61e6b6a to 96e5770 Compare August 28, 2025 11:20
@druckdev
Copy link
Contributor Author

/preview

Copy link

Preview email sent as pull.2041.git.git.1756382645725.gitgitgadget@gmail.com

@druckdev
Copy link
Contributor Author

/preview

Copy link

Preview email sent as pull.2041.git.git.1756384478802.gitgitgadget@gmail.com

@druckdev
Copy link
Contributor Author

/submit

Copy link

Submitted as pull.2041.git.git.1756385026051.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-2041/druckdev/pr2041-graceful-submodule-links-v1

To fetch this version to local tag pr-git-2041/druckdev/pr2041-graceful-submodule-links-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-2041/druckdev/pr2041-graceful-submodule-links-v1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants