Re: [PATCH 1/3] C implementation of the 'git' program, take two.

From: Linus Torvalds <torvalds@osdl.org>
Date: Nov 16, 2005, 00:18 UTC
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:

Show 9 quoted lines

> +
> +	/* 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