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

[René Scharfe <l.s.r@xxxxxx>]

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


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html