Re: [RFC] setup: fail if .git is not a file or directory

[Junio C Hamano <gitster@pobox.com>]

Tian Yuchen <a3205153416@gmail.com> writes:

> Currently, `setup_git_directory_gently_1()` checks if `.git` is a
> regular file (handling submodules/worktrees) or a directory. If it is
> neither (e.g., a FIFO), the code hits a NEEDSWORK comment and simply
> ignores the entity, continuing the discovery process in the parent
> directory.

Thanks for noticing the needswork comment.

> This behavior can be very dangerous. If a user is inside a subdirectory
> containing a melformed/broken `.git` entity, the Git will traverse up,
> attach to a parent repository and might execute destructive commands.

Is it?  If a malicious party can replace your .git file in your
submodule with a device node or a fifo, it means they can write into
your working tree of your top level superproject, so they have other
and better things to use to attack you if they wanted to.  I would
say 'very dangerous' is an overstatement.  If "git add" in a place
you thought was in your submodule ends up adding the file to the
superproject because of filesystem corruption making ".git" in your
submodule to something strange, you'd have larger problem anyway---
would you trust your commits and trees recorded in such a corrupt
filesystem?

Having said all that.

Failing instead of ignoring may make it easier for the end-users to
notice anomalies and take corrective action, if this non-file
non-directory filesystem entity was created by mistake or by
file-system corruption.  It would be an unnecessary breakage to
those who deliberately named a fifo they use ".git", fully knowing
well that it is a reserved name that "git add" and friends happily
ignore, but I somehow find it unlikely.

> But I still have questions:
> 1. Is failing hard the desired behavior here? Should skipping it and
>    continuing discovery be an option for the user, which might seem
>    more fault-tolerant?
> 2. Should we die() immediately here, or return GIT_DIR_INVALID_GITFILE
>    and let the caller decide?

Determining if it makes sense to do so is 80% of the work needed to
resolve a NEEDSWORK comment.  The actual implementation is often
much simpler than the work needed for it.  ;-)

> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
> ---
>  setup.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 3a6a048620..a1b56de67a 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1581,7 +1581,17 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  		if (!gitdirenv) {
>  			if (die_on_error ||
>  			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> -				/* NEEDSWORK: fail if .git is not file nor dir */
> +				struct stat st;
> +				if (!lstat(dir->buf, &st) &&
> +					!S_ISREG(st.st_mode) &&
> +					!S_ISDIR(st.st_mode)){
> +
> +					if (die_on_error)
> +						die(_("Invalid %s: not a regular file or directory"), dir->buf);
> +					else
> +						return GIT_DIR_INVALID_GITFILE;
> +				}

I would have expected that the new code would come after the
existing "if the thing is a .git directory, it is a happy outcome"
code.  In this code path, where we found ".git" that is not a file,
the most likely reason is because we found a healthy git directory,
so it is a sane thing to avoid extra lstat() in that normal
codepath.  Only after we see that the ".git" we have is neither a
"gitdir:" file nor a git directory, we know we are dealing with a
rare anomaly that we can afford to waste extra cycles.

>  				if (is_git_directory(dir->buf)) {
>  					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>  					gitdir_path = xstrdup(dir->buf);