Re: [PATCH] setup: don't die if realpath(3) fails on getcwd(3)
From: Junio C Hamano <hidden>
Date: 2022-05-20 18:39:28
Kevin Locke [off-list ref] 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).
quoted hunk
Signed-off-by: Kevin Locke <redacted> --- 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