Re: [PATCH v5 3/7] submodule: always validate gitdirs inside submodule_name_to_gitdir
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Dec 5, 2025, 12:17 UTC
- Message-ID
- <aTLNPKlDnsNzyZkC@pks.im>
- In-Reply-To
- <20251119211030.2008441-4-adrian.ratiu@collabora.com>
On Wed, Nov 19, 2025 at 11:10:26PM +0200, Adrian Ratiu wrote:
Show 28 quoted lines
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 2873b2780e..9914ca0786 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1780,23 +1776,6 @@ static int clone_submodule(const struct module_clone_data *clone_data,
> free(path);
> }
>
> - /*
> - * We already performed this check at the beginning of this function,
> - * before cloning the objects. This tries to detect racy behavior e.g.
> - * in parallel clones, where another process could easily have made the
> - * gitdir nested _after_ it was created.
> - *
> - * To prevent further harm coming from this unintentionally-nested
> - * gitdir, let's disable it by deleting the `HEAD` file.
> - */
> - if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0) {
> - char *head = xstrfmt("%s/HEAD", sm_gitdir);
> - unlink(head);
> - free(head);
> - die(_("refusing to create/use '%s' in another submodule's "
> - "git dir"), sm_gitdir);
> - }
> -
> connect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0);
>
> p = repo_submodule_path(the_repository, clone_data_path, "config");Hm. This one is a bit puzzling to me. This seems to explicitly be a check about a TOCTOU-style race, where a concurrent process might have created the parent repository after our initial validation of the path. We don't call `submodule_name_to_gitdir()` inbetween those two calls though, so why is this not a concern anymore with the unified API?
Show 35 quoted lines
> diff --git a/submodule.c b/submodule.c
> index 35c55155f7..8ef028f26b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2153,30 +2153,11 @@ int submodule_move_head(const char *path, const char *super_prefix,
>
> if (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {
> if (old_head) {
> - if (!submodule_uses_gitfile(path))
> - absorb_git_dir_into_superproject(path,
> - super_prefix);
> - else {
> - char *dotgit = xstrfmt("%s/.git", path);
> - char *git_dir = xstrdup(read_gitfile(dotgit));
> -
> - free(dotgit);
> - if (validate_submodule_git_dir(git_dir,
> - sub->name) < 0)
> - die(_("refusing to create/use '%s' in "
> - "another submodule's git dir"),
> - git_dir);
> - free(git_dir);
> - }
> + absorb_git_dir_into_superproject(path, super_prefix);
> } else {
> struct strbuf gitdir = STRBUF_INIT;
> submodule_name_to_gitdir(&gitdir, the_repository,
> sub->name);
> - if (validate_submodule_git_dir(gitdir.buf,
> - sub->name) < 0)
> - die(_("refusing to create/use '%s' in another "
> - "submodule's git dir"),
> - gitdir.buf);
> connect_work_tree_and_git_dir(path, gitdir.buf, 0);
> strbuf_release(&gitdir);The second case here makes sense to me, as we do call `submodule_name_to_gitdir()`. But in the first branch of the condition we retrieve the path directly, so we're not guarded by the validation anymore, are we?
Patrick