Re: [PATCH] setup: don't die if realpath(3) fails on getcwd(3)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 20, 2022, 18:38 UTC
- Message-ID
- <xmqqtu9kgibk.fsf@gitster.g>
- In-Reply-To
- <80eeba2b2a58af2a5497f398beb5c03447c41f61.1653003552.git.kevin@kevinlocke.name>
Kevin Locke <kevin@kevinlocke.name> writes:
Show 17 quoted lines
> 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).
Show 22 quoted lines
> 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?
Show 5 quoted lines
> tmp_original_cwd = NULL; > - startup_info->original_cwd = strbuf_detach(&tmp, NULL); > > /* > * Get our worktree; we only protect the current working directory