Re: [PATCH v3 1/4] real_path: resolve symlinks by hand

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 12, 2016, 22:19 UTC
Message-ID: <xmqqd1gw94f1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1481566615-75299-2-git-send-email-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 9 quoted lines

> +/* removes the last path component from 'path' except if 'path' is root */
> +static void strip_last_component(struct strbuf *path)
> +{
> +	size_t offset = offset_1st_component(path->buf);
> +	size_t len = path->len;
> +
> +	/* Find start of the last component */
> +	while (offset < len && !is_dir_sep(path->buf[len - 1]))
> +		len--;

If somebody at a higher level in the callchain has already normalized path, this is not a problem, but this will behave "unexpectedly" when path ends with a dir_sep byte (or more).

E.g. for input path "foo/bar/", the above loop runs zero times and then ...

> +	/* Skip sequences of multiple path-separators */
> +	while (offset < len && is_dir_sep(path->buf[len - 1]))
> +		len--;

... the slash at the end is removed, leaving "foo/bar" in path.

Show 23 quoted lines

> +	strbuf_setlen(path, len);
> +}
> ...
> +/* get (and remove) the next component in 'remaining' and place it in 'next' */
> +static void get_next_component(struct strbuf *next, struct strbuf *remaining)
> +{
> +	char *start = NULL;
> +	char *end = NULL;
> +
> +	strbuf_reset(next);
> +
> +	/* look for the next component */
> +	/* Skip sequences of multiple path-separators */
> +	for (start = remaining->buf; is_dir_sep(*start); start++)
> +		; /* nothing */
> +	/* Find end of the path component */
> +	for (end = start; *end && !is_dir_sep(*end); end++)
> +		; /* nothing */
> +
> +	strbuf_add(next, start, end - start);
> +	/* remove the component from 'remaining' */
> +	strbuf_remove(remaining, 0, end - remaining->buf);
> +}

Unlike the strip_last_component(), I think this one is more carefully done and avoids getting fooled by //extra//slashes// at the beginning or at the end, which does help in the correctness of the loop we see below.

Show 33 quoted lines

> @@ -58,74 +88,112 @@ static const char *real_path_internal(const char *path, int die_on_error)
>  			goto error_out;
>  	}
>  
> +	strbuf_reset(&resolved);
> +
> +	if (is_absolute_path(path)) {
> +		/* absolute path; start with only root as being resolved */
> +		int offset = offset_1st_component(path);
> +		strbuf_add(&resolved, path, offset);
> +		strbuf_addstr(&remaining, path + offset);
> +	} else {
> +		/* relative path; can use CWD as the initial resolved path */
> +		if (strbuf_getcwd(&resolved)) {
> +			if (die_on_error)
> +				die_errno("unable to get current working directory");
> +			else
> +				goto error_out;
>  		}
> +		strbuf_addstr(&remaining, path);
> +	}
>  
> +	/* Iterate over the remaining path components */
> +	while (remaining.len > 0) {
> +		get_next_component(&next, &remaining);
> +
> +		if (next.len == 0) {
> +			continue; /* empty component */
> +		} else if (next.len == 1 && !strcmp(next.buf, ".")) {
> +			continue; /* '.' component */
> +		} else if (next.len == 2 && !strcmp(next.buf, "..")) {
> +			/* '..' component; strip the last path component */
> +			strip_last_component(&resolved);

Wouldn't this let "resolved" eventually run out of the path components to strip for a malformed input e.g. "/a/../../b"?

Show 7 quoted lines

> + ...
> +			/*
> +			 * if there are still remaining components to resolve
> +			 * then append them to symlink
> +			 */
> +			if (remaining.len) {
> +				strbuf_addch(&symlink, '/');

This can add duplicate dir_sep if readlink(2)'ed contents of the symbolic link already ends with a slash, but I think it (together with the fact that the code does nothing to normalize what is read from the symbolic link) probably does not matter, given the way how get_next_component() is implemented.

Show 20 quoted lines

> +				strbuf_addbuf(&symlink, &remaining);
> +			}
> +
> +			/*
> +			 * use the symlink as the remaining components that
> +			 * need to be resloved
> +			 */
> +			strbuf_swap(&symlink, &remaining);
> +		}
>  	}
>  
> +	retval = resolved.buf;
> +
>  error_out:
> +	strbuf_release(&remaining);
> +	strbuf_release(&next);
> +	strbuf_release(&symlink);
>  
>  	return retval;
>  }