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

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

[Nguyễn Thái Ngọc Duy]

This array 'cwd' is used to store the result from getcwd() and chdir()
back. PATH_MAX is the right constant for the job. On systems with
longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,
e.g. "git init". Make it static too to reduce stack usage.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 abspath.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/abspath.c b/abspath.c
index ca33558..c0c868f 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] = "";
+	static char cwd[PATH_MAX];
 
 	int buf_index = 1;
 
@@ -49,6 +49,8 @@ static const char *real_path_internal(const char *path, int die_on_error)
 	char *last_elem = NULL;
 	struct stat st;
 
+	*cwd = '\0';
+
 	/* We've already done it */
 	if (path == buf || path == next_buf)
 		return path;
-- 
1.9.1.346.ga2b5940

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

[René Scharfe]

Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
> This array 'cwd' is used to store the result from getcwd() and chdir()
> back. PATH_MAX is the right constant for the job.

PATH_MAX may be better than 1024, but there can't really be a correct
constant.  The actual limit depends on the file system.

> On systems with
> longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,
> e.g. "git init". 

Out of curiosity, I just created a path with over 130000 characters on
Linux.  It's not actually usable but it shows that 4096 is not a real
limit on Linux.  Here's how I created that insane path:

	a=1234567890
	x=$a$a$a$a$a
	y=$x$x$x$x$x
	cd /tmp
	while true
	do
		mkdir $y || break
		cd $y || break
	done
	pwd >/tmp/y
	cd /tmp
	wc -c <y

Another experiment with the program listed below shows that getcwd() on
Linux works fine with such paths if the provided buffer is large
enough, even though PATH_MAX is 4096:

	#include <limits.h>
	#include <stdio.h>
	#include <string.h>
	#include <unistd.h>
	int main(int argc, char **argv)
	{
		char cwd[200000];
		printf("PATH_MAX = %d\n", PATH_MAX);
		if (getcwd(cwd, sizeof(cwd)))
			printf("strlen(getcwd()) = %zu\n", strlen(cwd));
		return 0;
	}

> Make it static too to reduce stack usage.

Why is that needed?  Is recursion involved?  (Didn't find any in the
function itself after a very brief look.)


There is get_current_dir_name(), a GNU extension that allocates the
necessary memory.  Perhaps we can use it instead and provide a
compatibility implementation based on getcwd() for platforms that don't
have it?

But then there's also this advice from the getcwd(3) manpage on OpenBSD:

"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."

So, how about something like this?

---
 abspath.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/abspath.c b/abspath.c
index ca33558..7fff13a 100644
--- a/abspath.c
+++ b/abspath.c
@@ -38,10 +38,10 @@ static const char *real_path_internal(const char *path, int die_on_error)
 
 	/*
 	 * If we have to temporarily chdir(), store the original CWD
-	 * here so that we can chdir() back to it at the end of the
+	 * here so that we can fchdir() back to it at the end of the
 	 * function:
 	 */
-	char cwd[1024] = "";
+	int cwd_fd = -1;
 
 	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_fd < 0)
+				cwd_fd = open(".", O_RDONLY);
+			if (cwd_fd < 0) {
 				if (die_on_error)
 					die_errno("Could not get current working directory");
 				else
@@ -142,8 +144,11 @@ 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_fd >= 0) {
+		if (fchdir(cwd_fd))
+			die_errno("Could not change back to the original working directory");
+		close(cwd_fd);
+	}
 
 	return retval;
 }
-- 
2.0.2

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> "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."
>
> So, how about something like this?

Yeah, I've always wanted to see us use fchdir() for coming back
(another thing I wanted to see is to use openat() and friends).

I do not offhand recall if the run of chdir() in gitdir discovery
code has a similar "now we are done, let's go back to the original"
use of chdir() there, but if we do, we should fix it, too.

Looks sensible from a cursory read.

>
> ---
>  abspath.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index ca33558..7fff13a 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -38,10 +38,10 @@ static const char *real_path_internal(const char *path, int die_on_error)
>  
>  	/*
>  	 * If we have to temporarily chdir(), store the original CWD
> -	 * here so that we can chdir() back to it at the end of the
> +	 * here so that we can fchdir() back to it at the end of the
>  	 * function:
>  	 */
> -	char cwd[1024] = "";
> +	int cwd_fd = -1;
>  
>  	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_fd < 0)
> +				cwd_fd = open(".", O_RDONLY);
> +			if (cwd_fd < 0) {
>  				if (die_on_error)
>  					die_errno("Could not get current working directory");
>  				else
> @@ -142,8 +144,11 @@ 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_fd >= 0) {
> +		if (fchdir(cwd_fd))
> +			die_errno("Could not change back to the original working directory");
> +		close(cwd_fd);
> +	}
>  
>  	return retval;
>  }

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

[Karsten Blees]

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)?

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.

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

[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

> 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?
-- 
Duy

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

[René Scharfe]

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

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

[Duy Nguyen]

On Fri, Jul 18, 2014 at 10:08 PM, René Scharfe <l.s.r@web.de> wrote:
>> 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?

I think it looks nice.

> Applying it to setup.c looks difficult to impossible, though.

Yeah, setup.c needs cwd as a string because it does something else to
cwd besides going back. Too bad.
-- 
Duy

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

[Karsten Blees]

Am 18.07.2014 12:49, schrieb Duy Nguyen:
> 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?
> 

real_path(): here we actually don't want to cd / cd back, we just do
this because getcwd reolves symlinks.

setup.c: AFAICT this sets up the work dir and stays there (no cd back).

git.c: Only does a 'preliminary' repo setup so that the alias mechanism
reads the correct config files. Then it reverts the setup if the
resulting git command doesn't need it. Perhaps it would be better (and
faster) to teach the alias code to read the right config file in the
first place?

unix-socket.c: This looks pretty broken. The cd / cd back logic is only
ever used if the socket path is too long. In this case, after cd'ing to
the parent directory of the socket, unix_stream_listen tries to unlink
the *original* socket path, instead of the remaining socket basename in
sockaddr_un.sun_path. I.e. the subsequent bind() will fail on second
invocation of the credential daemon.

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

[René Scharfe]

Am 20.07.2014 02:29, schrieb Karsten Blees:
> unix-socket.c: This looks pretty broken. The cd / cd back logic is only
> ever used if the socket path is too long. In this case, after cd'ing to
> the parent directory of the socket, unix_stream_listen tries to unlink
> the *original* socket path, instead of the remaining socket basename in
> sockaddr_un.sun_path. I.e. the subsequent bind() will fail on second
> invocation of the credential daemon.

-- >8 --
Subject: [PATCH] unix-socket: remove stale socket before calling chdir()

unix_stream_listen() is given a path.  It calls unix_sockaddr_init(),
which in turn can call chdir().  After that a relative path doesn't
mean the same as before.  Any use of the original path should thus
happen before that call.  For that reason, unlink the given path
(to get rid of a possibly existing stale socket) right at the
beginning of the function.

Noticed-by: Karsten Blees <karsten.blees@gmail.com>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 unix-socket.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/unix-socket.c b/unix-socket.c
index 01f119f..91bd6b8 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -99,11 +99,12 @@ int unix_stream_listen(const char *path)
 	struct sockaddr_un sa;
 	struct unix_sockaddr_context ctx;
 
+	unlink(path);
+
 	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
 		return -1;
 	fd = unix_stream_socket();
 
-	unlink(path);
 	if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
 		goto fail;
 
-- 
2.0.2

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

[Jeff King]

On Sun, Jul 20, 2014 at 10:00:41AM +0200, René Scharfe wrote:

> -- >8 --
> Subject: [PATCH] unix-socket: remove stale socket before calling chdir()
> 
> unix_stream_listen() is given a path.  It calls unix_sockaddr_init(),
> which in turn can call chdir().  After that a relative path doesn't
> mean the same as before.  Any use of the original path should thus
> happen before that call.  For that reason, unlink the given path
> (to get rid of a possibly existing stale socket) right at the
> beginning of the function.

Thanks, I think this ordering problem was just missed in 1eb10f4
(unix-socket: handle long socket pathnames, 2012-01-09).

Your solution looks OK, though I think also just using:

  unlink(sa.sun_path);

would work, too (that is the path we are feeding to bind(), whether we
have chdir'd or not, so perhaps it is a little more obviously correct?).
I'm OK with either.

> diff --git a/unix-socket.c b/unix-socket.c
> index 01f119f..91bd6b8 100644
> --- a/unix-socket.c
> +++ b/unix-socket.c
> @@ -99,11 +99,12 @@ int unix_stream_listen(const char *path)
>  	struct sockaddr_un sa;
>  	struct unix_sockaddr_context ctx;
>  
> +	unlink(path);
> +
>  	if (unix_sockaddr_init(&sa, path, &ctx) < 0)
>  		return -1;
>  	fd = unix_stream_socket();
>  
> -	unlink(path);

I briefly wondered if this should be unlinking only when we get EEXIST,
but I don't think it is worth caring about. The only caller is
credential-cache, and it always wants to unconditionally replace
whatever is there (it will already have tried to contact any existing
socket).

-Peff

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

[René Scharfe]

Am 18.07.2014 01:03, schrieb Karsten Blees:
> 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)?

That's a good question; thanks for stepping back and looking at the 
bigger picture.  If there is widespread OS support for a functionality 
then we should use it and just provide a compatibility implementation 
for those platforms lacking it.  The downside is that compat code gets 
less testing.

Seeing that readlink() is left as a stub in compat/mingw.h that only 
errors out, would the equivalent function on Windows be PathCanonicalize 
(http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)?

> For non-XSI-compliant platforms, we could keep the current implementation.

OK, so realpath() for Linux and the BSDs, mingw_realpath() wrapping 
PathCanonicalize() for Windows and the current code for the rest?

> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
> lockfile.c to all path components.

Thread safety sounds good.  We'd also need something like 
normalize_path_copy() but without the conversion of backslashes to 
slashes, in order to get rid of "." and ".." path components and 
something like absolute_path() that doesn't die on error, no?

> 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.

On Windows, there *is* an absolute path length limit of 260 in the 
normal case and a bit more than 32000 for some functions using the \\?\ 
namespace.  So one could get away with using a constant-sized buffer for 
a "remember the place and return later" function here.

Also, _getcwd can be asked to allocate an appropriately-sized buffer for 
use, like GNU's get_current_dir_name, by specifying NULL as its first 
parameter (http://msdn.microsoft.com/en-us/library/sf98bd4y.aspx).

Not having to move around at all as mentioned above is still better, of 
course.

René

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

[Karsten Blees]

Am 18.07.2014 13:32, schrieb René Scharfe:
> Am 18.07.2014 01:03, schrieb Karsten Blees:
>> 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)?
> 
> That's a good question; thanks for stepping back and looking at the bigger picture.  If there is widespread OS support for a functionality then we should use it and just provide a compatibility implementation for those platforms lacking it.  The downside is that compat code gets less testing.
> 

I just noticed that in contrast to the POSIX realpath(), our real_path() doesn't require the last path component to exist. I don't know if this property is required by the calling code, though.

> Seeing that readlink()

You mean realpath()? We don't have a stub for that yet.

> is left as a stub in compat/mingw.h that only errors out, would the equivalent function on Windows be PathCanonicalize (http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)?
> 

PathCanonicalize() doesn't return an absolute path, the realpath() equivalent would be GetFullPathName() (doesn't resolve symlinks) or GetFinalPathNameByHandle() (requires Vista, resolves symlinks, requires the path to exist).

>> For non-XSI-compliant platforms, we could keep the current implementation.
> 
> OK, so realpath() for Linux and the BSDs, mingw_realpath() wrapping PathCanonicalize() for Windows and the current code for the rest?
> 
>> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
>> lockfile.c to all path components.
> 
> Thread safety sounds good.  We'd also need something like normalize_path_copy() but without the conversion of backslashes to slashes, in order to get rid of "." and ".." path components and something like absolute_path() that doesn't die on error, no?
> 

Windows can handle forward slashes, so normalize_path_copy works just fine.

>> 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.
> 
> On Windows, there *is* an absolute path length limit of 260 in the normal case and a bit more than 32000 for some functions using the \\?\ namespace. So one could get away with using a constant-sized buffer for a "remember the place and return later" function here.
> 

The current directory is pretty much the only exception to the \\?\ trick [1]. So a fixed buffer for getcwd() would actually be fine on Windows (although it would have to be 3 * PATH_MAX, as PATH_MAX wide chars will convert to at most 3 * PATH_MAX UTF-8 chars).

However, a POSIX conformant getcwd must fail with ERANGE if the buffer is too small. So a better alternative would be to add a strbuf_getcwd() that works similar to strbuf_readlink() (i.e. resize the buffer until its large enough).

Side note: the 'hard' 260 limit for the current directory also means that as long as we *simulate* realpath() via chdir()/getcwd(), long paths [1] don't work here.

> Also, _getcwd can be asked to allocate an appropriately-sized buffer for use, like GNU's get_current_dir_name, by specifying NULL as its first parameter (http://msdn.microsoft.com/en-us/library/sf98bd4y.aspx).
> 

We use nedmalloc in the Windows builds, so unfortuately we cannot free memory allocated by MSVCRT.dll.


[1] http://msdn.microsoft.com/en-us/library/windows/desktop/aa365530%28v=vs.85%29.aspx
[2] https://github.com/msysgit/git/commit/84393750

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

[René Scharfe]

Am 20.07.2014 01:55, schrieb Karsten Blees:
> Am 18.07.2014 13:32, schrieb René Scharfe:
>> Am 18.07.2014 01:03, schrieb Karsten Blees:
>>> 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)?
>>
>> That's a good question; thanks for stepping back and looking at the
>> bigger picture.  If there is widespread OS support for a
>> functionality then we should use it and just provide a
>> compatibility implementation for those platforms lacking it.  The
>> downside is that compat code gets less testing.
>>
>
> I just noticed that in contrast to the POSIX realpath(), our
> real_path() doesn't require the last path component to exist. I don't
> know if this property is required by the calling code, though.

If it's important then removing the last component on ENOENT, calling 
realpath() again and appending it should handle that, right?

>> Seeing that readlink()
>
> You mean realpath()? We don't have a stub for that yet.
>
>> is left as a stub in compat/mingw.h that only errors out, would the
>> equivalent function on Windows be PathCanonicalize
>> (http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)?
>>
> PathCanonicalize() doesn't return an absolute path, the realpath()
> equivalent would be GetFullPathName() (doesn't resolve symlinks) or
> GetFinalPathNameByHandle() (requires Vista, resolves symlinks,
> requires the path to exist).

I meant readlink() and assumed its ENOSYS stub in compat/mingw.h means 
that git doesn't handle symlinks on Windows at all.

> However, a POSIX conformant getcwd must fail with ERANGE if the
> buffer is too small. So a better alternative would be to add a
> strbuf_getcwd() that works similar to strbuf_readlink() (i.e. resize
> the buffer until its large enough).

That's a good idea anyway, will send patches.

> Side note: the 'hard' 260 limit for the current directory also means
> that as long as we *simulate* realpath() via chdir()/getcwd(), long
> paths [1] don't work here.

Great, so that motivation for getting a chdir()-free implementation 
remains. :)

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This array 'cwd' is used to store the result from getcwd() and chdir()
> back. PATH_MAX is the right constant for the job. On systems with
> longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,
> e.g. "git init". Make it static too to reduce stack usage.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Thanks.  It seems that this 1024 has been with us since the
beginning of this piece of code.  I briefly wondered if there are
strange platform that will have PATH_MAX shorter than 1024 that will
be hurt by this change, but the result in cwd[] is used to grow the
final result bufs[] that is sized based on PATH_MAX anyway, so it
will not be an issue (besides, the absurdly short one seems to be
a different macro, MAX_PATH, on Windows).

Will queue.

>  abspath.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/abspath.c b/abspath.c
> index ca33558..c0c868f 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] = "";
> +	static char cwd[PATH_MAX];
>  
>  	int buf_index = 1;
>  
> @@ -49,6 +49,8 @@ static const char *real_path_internal(const char *path, int die_on_error)
>  	char *last_elem = NULL;
>  	struct stat st;
>  
> +	*cwd = '\0';
> +
>  	/* We've already done it */
>  	if (path == buf || path == next_buf)
>  		return path;

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

[Karsten Blees]

Am 17.07.2014 20:03, schrieb Junio C Hamano:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
> 
>> This array 'cwd' is used to store the result from getcwd() and chdir()
>> back. PATH_MAX is the right constant for the job. On systems with
>> longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,
>> e.g. "git init". Make it static too to reduce stack usage.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
> 
> Thanks.  It seems that this 1024 has been with us since the
> beginning of this piece of code.  I briefly wondered if there are
> strange platform that will have PATH_MAX shorter than 1024 that will
> be hurt by this change, but the result in cwd[] is used to grow the
> final result bufs[] that is sized based on PATH_MAX anyway, so it
> will not be an issue (besides, the absurdly short one seems to be
> a different macro, MAX_PATH, on Windows).
> 

Indeed, there's a strange platform where PATH_MAX is only 259. With
Unicode support, the current directory can be that many wide characters,
which means up to 3 * PATH_MAX UTF-8 bytes (if all of them are CJK).

I don't think this will be a problem, though, as the return buffer is
PATH_MAX-bounded as well.

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

[Karsten Blees]

Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
> e.g. "git init". Make it static too to reduce stack usage.

But wouldn't this increase overall memory usage? Stack memory
will be reused by subsequent code, while static memory cannot
be reused (but still increases the process working set).

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

[Junio C Hamano]

Karsten Blees <karsten.blees@gmail.com> writes:

> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
>> e.g. "git init". Make it static too to reduce stack usage.
>
> But wouldn't this increase overall memory usage? Stack memory
> will be reused by subsequent code, while static memory cannot
> be reused (but still increases the process working set).

Correct.  And this is a function that is not involved in deep
recursion, so avoiding stack allocation seems quite misguided.