From: Linus Torvalds <torvalds@osdl.org>
Date: Wed, 16 Nov 2005 00:18:26 GMT
Subject: Re: [PATCH 1/3] C implementation of the 'git' program, take two.
Message-ID: <Pine.LNX.4.64.0511151603510.11232@g5.osdl.org>
In-Reply-To: <20051115233125.3153B5BF76@nox.op5.se>



On Wed, 16 Nov 2005, Andreas Ericsson wrote:
> +
> +	/* allow relative paths, but run with exact */
> +	if (chdir(exec_path)) {
> +		printf("git: '%s': %s\n", exec_path, strerror(errno));
> +		exit (1);
> +	}
> +
> +	getcwd(git_command, sizeof(git_command));
> +	chdir(wd);

Argh. This is pretty horrible way to turn a path into an absolute one. 
Especially since you didn't even test whether the original "wd" was 
successful.

Why don't you just do

	if (exec_path[0] != '/') {
		.. prepend "cwd/" to exec_path ..

since as far as I can tell you don't actually care whether it's a 
simplified path or not (you can remove "./" at the beginning just to make 
it cleaner, if you wish. In fact, you can remove "../" at the beginning 
too (but only the beginning) since getcwd() shouldn't have any symlink 
components).

The reason to avoid "chdir(relative) + chdir(back)" is that it totally 
unnecessarily breaks under some extreme cases. For example, if the 
exec_path is already absolute, and we just happen to be in a really deep 
subdirectory, then the getcwd() could have failed due to the PATH_MAX 
limitations.

Also, depending on getcwd() will not work if any parent directory is 
unreadable or non-executable (well, under Linux it will, as long as it's 
executable, since getcwd() is actually a system call. Not in UNIX in 
general, though). Again, that means that unless you _have_ to know what 
the cwd is, you should try to avoid relying on it.

Now, there are Linux-specific tricks that can avoid some of the problems 
if you want to, but they are very much hacks:

	if (filename[0] != '/') {
		fd = open(filename, O_DIRECTORY);
		if (fd >= 0) {
			snprintf(link_name, sizeof(link_name), "/proc/self/fd/%d", fd);
			if (!readlink(link_name ...)) {
				.. there it is ..
			}
			close(fd);
		}
	..

and the nicer thign to do is to just not try to be clever.

		Linus