Re: [PATCH] setup: don't die if realpath(3) fails on getcwd(3)

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

Kevin Locke <kevin@kevinlocke.name> writes:

> Prior to Git 2.35.0, git could be run from an inaccessible working
> directory so long as the git repository specified by options and/or
> environment variables was accessible.  For example:
>
>     git init repo
>     mkdir -p a/b
>     cd a/b
>     chmod u-x ..
>     git -C "${PWD%/a/b}/repo" status
>
> If this example seems a bit contrived, consider running with the
> repository owner as a substitute UID (e.g. with runuser(1) or sudo(8))
> without ensuring the working directory is accessible by that user.
>
> The code added by e6f8861bd4 to preserve the working directory attempts
> to normalize the path using strbuf_realpath().  If that fails, as in the
> case above, it is treated as a fatal error.

Thanks.  As this thing is primarily for safety, I am inclined to say
that I'd prefer to see it error out when we cannot figure out the
necessary info to keep that safety promise to the users, than using
an unnormalized value as a stand-in and letting the logic that is
designed to be fed a normalized value do random (and possibly wrong)
things.  But see below.

> Fixes: e6f8861bd4 ("setup: introduce startup_info->original_cwd")

AFAIK, we do not use this kind of trailer in this project.  Casting
in stone the claim that this "fixes" would be embarrassing when it
turns out that it does not fix it (or even worse, breaks it).

> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> ---
>  setup.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index a7b36f3ffb..fb68caaae0 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -458,11 +458,13 @@ static void setup_original_cwd(void)
>  	 *     not startup_info->original_cwd.
>  	 */
>  
> -	/* Normalize the directory */
> -	strbuf_realpath(&tmp, tmp_original_cwd, 1);
> -	free((char*)tmp_original_cwd);
> +	/* Try to normalize the directory.  Fails if ancestor not readable. */
> +	if (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
> +		free((char*)tmp_original_cwd);
> +		startup_info->original_cwd = strbuf_detach(&tmp, NULL);
> +	} else
> +		startup_info->original_cwd = tmp_original_cwd;

I am OK to loosen the "we try not to remove the original cwd" logic
so that it does not kick in when we cannot figure out the original
cwd in the first place.  But if that is the case, then I'd rather
see "startrup_info->original_cwd set to NULL" as the signal that we
are in such a situation.

Elijah, what's your take on this change?

>  	tmp_original_cwd = NULL;
> -	startup_info->original_cwd = strbuf_detach(&tmp, NULL);
>  
>  	/*
>  	 * Get our worktree; we only protect the current working directory