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

[Brandon Williams <bmwill@google.com>]

On 12/12, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > +/* 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.
> 

The way this is currently used I don't believe this scenario can happen
(since input to this shouldn't have trailing slashes), but if others
begin to use this function then yes, that is an implicit assumption.  I
think it may be an ok assumption though since this is only called on
"resolved" which is the ouput and needs to be normalized to begin with. To
fix this we could simply add the while loop that strips dir_sep at the
beginning as well as at the end, like so:

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

> > +	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.
> 
> > @@ -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"?
> 

As I understand it, that path is correct and would resolve to "/b".  The
strip_last_component function doesn't allow striping the "1st" component
or the "root" component.  So if resolved is "/" and we encounter a ".."
which requires striping of the last component, the result would be "/".

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

Yes, I think the way get_next_component() is written will account for
non-normalized symlink contents.  This way we only have to worry about
normalizing input in one location (maybe two with
strip_last_component()).

> > +				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;
> >  }
> 

-- 
Brandon Williams