Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()

["René Scharfe" <l.s.r@web.de>]

Am 18.07.2014 12:49, schrieb Duy Nguyen:
> On Fri, Jul 18, 2014 at 6:03 AM, Karsten Blees <karsten.blees@gmail.com> wrote:
>> Am 17.07.2014 19:05, schrieb René Scharfe:
>>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
>> [...]
>>> "These routines have traditionally been used by programs to save the
>>> name of a working directory for the purpose of returning to it. A much
>>> faster and less error-prone method of accomplishing this is to open the
>>> current directory (.) and use the fchdir(2) function to return."
>>>
>>
>> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not
>> use realpath() directly (which would also be thread-safe)?
> 
> But realpath still needs a given buffer (of PATH_MAX at least again).
> Unless you pass a NULL pointer as "resolved_name", then Linux can
> allocate the buffer but that's implementation specific [1]. I guess we
> can make a wrapper "git_getcwd" that returns a new buffer. The default
> implementation returns one of PATH_MAX chars, certain platforms like
> Linux can use realpath (or something else) to return a longer path?
> 
> [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html

realpath() can be used to implement the whole of real_path_internal(),
IIUC, so there would be no need to change the current directory anymore.

The realpath(3) manpage for Linux mentions that behaviour of realpath()
with resolved_path being NULL is not standardized by POSIX.1-2001 but
by POSIX.1-2008.  At least Linux, OpenBSD and FreeBSD seem to support
that, based on their manpages.  Here's the standard doc:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html

>> For non-XSI-compliant platforms, we could keep the current implementation.
>> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
>> lockfile.c to all path components.
>>
>>
>> If I may bother you with the Windows point of view:
>>
>> There is no fchdir(), and I'm pretty sure open(".") won't work either.
>>
>> fchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo).
>> realpath() is pretty much what GetFinalPathNameByHandle() does. However,
>> both of these APIs require Windows Vista.
>>
>> Opening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS,
>> which AFAICT MSVCRT.dll's open() implementation does _not_ do (could be
>> emulated in our mingw_open() wrapper, though).
>>
>> ...lots of work for little benefit, I would think.
>>
> 
> We could wrap this "get cwd, cd back" pattern as well. So "save_cwd"
> returns an opaque pointer, "go_back" takes the pointer, (f)chdir back
> and free the pointer if needed. On platforms that support fchdir, it
> can be used, else we fall back to chdir. I think there are only four
> places that follow this pattern, here, setup.c (.git discovery), git.c
> (restore_env) and unix-socket.c. Enough call sites to make it worth
> the effort?

On a cursory look I didn't find any more potential users.  Something
like this?  Applying it to setup.c looks difficult to impossible,
though.

---
 Makefile          |  8 ++++++++
 abspath.c         | 10 ++++++----
 cache.h           |  1 +
 git.c             |  9 +++++----
 save-cwd-fchdir.c | 25 +++++++++++++++++++++++++
 save-cwd.c        | 22 ++++++++++++++++++++++
 save-cwd.h        |  3 +++
 unix-socket.c     | 11 ++++++-----
 8 files changed, 76 insertions(+), 13 deletions(-)
 create mode 100644 save-cwd-fchdir.c
 create mode 100644 save-cwd.c
 create mode 100644 save-cwd.h

diff --git a/Makefile b/Makefile
index 840057c..ecf3129 100644
--- a/Makefile
+++ b/Makefile
@@ -32,6 +32,8 @@ all::
 #
 # Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.
 #
+# Define HAVE_FCHDIR if you have fchdir(2).
+#
 # Define NO_CURL if you do not have libcurl installed.  git-http-fetch and
 # git-http-push are not built, and you cannot use http:// and https://
 # transports (neither smart nor dumb).
@@ -1118,6 +1120,12 @@ ifdef HAVE_ALLOCA_H
 	BASIC_CFLAGS += -DHAVE_ALLOCA_H
 endif
 
+ifdef HAVE_FCHDIR
+	COMPAT_OBJS += save-cwd-fchdir.o
+else
+	COMPAT_OBJS += save-cwd.o
+endif
+
 ifdef NO_CURL
 	BASIC_CFLAGS += -DNO_CURL
 	REMOTE_CURL_PRIMARY =
diff --git a/abspath.c b/abspath.c
index ca33558..f49bacc 100644
--- a/abspath.c
+++ b/abspath.c
@@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 	 * here so that we can chdir() back to it at the end of the
 	 * function:
 	 */
-	char cwd[1024] = "";
+	struct saved_cwd *cwd = NULL;
 
 	int buf_index = 1;
 
@@ -80,7 +80,9 @@ static const char *real_path_internal(const char *path, int die_on_error)
 		}
 
 		if (*buf) {
-			if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+			if (!cwd)
+				cwd = save_cwd();
+			if (!cwd) {
 				if (die_on_error)
 					die_errno("Could not get current working directory");
 				else
@@ -142,8 +144,8 @@ static const char *real_path_internal(const char *path, int die_on_error)
 	retval = buf;
 error_out:
 	free(last_elem);
-	if (*cwd && chdir(cwd))
-		die_errno("Could not change back to '%s'", cwd);
+	if (cwd && restore_cwd(cwd))
+		die_errno("Could not change back to the original working directory");
 
 	return retval;
 }
diff --git a/cache.h b/cache.h
index 92fc9f1..5b4e9cd 100644
--- a/cache.h
+++ b/cache.h
@@ -7,6 +7,7 @@
 #include "advice.h"
 #include "gettext.h"
 #include "convert.h"
+#include "save-cwd.h"
 
 #include SHA1_HEADER
 #ifndef git_SHA_CTX
diff --git a/git.c b/git.c
index 5b6c761..946cc82 100644
--- a/git.c
+++ b/git.c
@@ -20,7 +20,7 @@ const char git_more_info_string[] =
 
 static struct startup_info git_startup_info;
 static int use_pager = -1;
-static char orig_cwd[PATH_MAX];
+static struct saved_cwd *orig_cwd;
 static const char *env_names[] = {
 	GIT_DIR_ENVIRONMENT,
 	GIT_WORK_TREE_ENVIRONMENT,
@@ -36,7 +36,8 @@ static void save_env(void)
 	if (saved_environment)
 		return;
 	saved_environment = 1;
-	if (!getcwd(orig_cwd, sizeof(orig_cwd)))
+	orig_cwd = save_cwd();
+	if (!orig_cwd)
 		die_errno("cannot getcwd");
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		orig_env[i] = getenv(env_names[i]);
@@ -48,8 +49,8 @@ static void save_env(void)
 static void restore_env(void)
 {
 	int i;
-	if (*orig_cwd && chdir(orig_cwd))
-		die_errno("could not move to %s", orig_cwd);
+	if (orig_cwd && restore_cwd(orig_cwd))
+		die_errno("could not move to the original working directory");
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
 		if (orig_env[i])
 			setenv(env_names[i], orig_env[i], 1);
diff --git a/save-cwd-fchdir.c b/save-cwd-fchdir.c
new file mode 100644
index 0000000..5327932
--- /dev/null
+++ b/save-cwd-fchdir.c
@@ -0,0 +1,25 @@
+#include "cache.h"
+
+struct saved_cwd {
+	int cwd_fd;
+};
+
+struct saved_cwd *save_cwd(void)
+{
+	struct saved_cwd *cwd = xmalloc(sizeof(*cwd));
+	cwd->cwd_fd = open(".", O_RDONLY);
+	if (cwd->cwd_fd < 0) {
+		free(cwd);
+		return NULL;
+	}
+	return cwd;
+}
+
+int restore_cwd(struct saved_cwd *cwd)
+{
+	int rc = fchdir(cwd->cwd_fd);
+	if (!rc)
+		rc = close(cwd->cwd_fd);
+	free(cwd);
+	return rc;
+}
diff --git a/save-cwd.c b/save-cwd.c
new file mode 100644
index 0000000..1864e9f
--- /dev/null
+++ b/save-cwd.c
@@ -0,0 +1,22 @@
+#include "cache.h"
+
+struct saved_cwd {
+	char cwd[PATH_MAX];
+};
+
+struct saved_cwd *save_cwd(void)
+{
+	struct saved_cwd *cwd = xmalloc(sizeof(*cwd));
+	if (!getcwd(cwd->cwd, sizeof(cwd->cwd))) {
+		free(cwd);
+		return NULL;
+	}
+	return cwd;
+}
+
+int restore_cwd(struct saved_cwd *cwd)
+{
+	int rc = chdir(cwd->cwd);
+	free(cwd);
+	return rc;
+}
diff --git a/save-cwd.h b/save-cwd.h
new file mode 100644
index 0000000..1087ed3
--- /dev/null
+++ b/save-cwd.h
@@ -0,0 +1,3 @@
+struct saved_cwd;
+struct saved_cwd *save_cwd(void);
+int restore_cwd(struct saved_cwd *);
diff --git a/unix-socket.c b/unix-socket.c
index 01f119f..65d92f2 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -18,19 +18,19 @@ static int chdir_len(const char *orig, int len)
 }
 
 struct unix_sockaddr_context {
-	char orig_dir[PATH_MAX];
+	struct saved_cwd *orig_dir;
 };
 
 static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)
 {
-	if (!ctx->orig_dir[0])
+	if (!ctx->orig_dir)
 		return;
 	/*
 	 * If we fail, we can't just return an error, since we have
 	 * moved the cwd of the whole process, which could confuse calling
 	 * code.  We are better off to just die.
 	 */
-	if (chdir(ctx->orig_dir) < 0)
+	if (restore_cwd(ctx->orig_dir))
 		die("unable to restore original working directory");
 }
 
@@ -39,7 +39,7 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 {
 	int size = strlen(path) + 1;
 
-	ctx->orig_dir[0] = '\0';
+	ctx->orig_dir = NULL;
 	if (size > sizeof(sa->sun_path)) {
 		const char *slash = find_last_dir_sep(path);
 		const char *dir;
@@ -57,7 +57,8 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 			return -1;
 		}
 
-		if (!getcwd(ctx->orig_dir, sizeof(ctx->orig_dir))) {
+		ctx->orig_dir = save_cwd();
+		if (!ctx->orig_dir) {
 			errno = ENAMETOOLONG;
 			return -1;
 		}
-- 
2.0.2