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

[Brandon Williams <bmwill@google.com>]

On 12/09, Johannes Sixt wrote:
> Am 09.12.2016 um 00:58 schrieb Brandon Williams:
> >The current implementation of real_path uses chdir() in order to resolve
> >symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
> >process as a whole and not just an individual thread.  Instead perform
> >the symlink resolution by hand so that the calls to chdir() can be
> >removed, making real_path one step closer to being reentrant.
> >
> >Signed-off-by: Brandon Williams <bmwill@google.com>
> >---
> > abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++---------------------
> > 1 file changed, 122 insertions(+), 61 deletions(-)
> >
> >diff --git a/abspath.c b/abspath.c
> >index 2825de8..92f2a29 100644
> >--- a/abspath.c
> >+++ b/abspath.c
> >@@ -11,8 +11,38 @@ int is_directory(const char *path)
> > 	return (!stat(path, &st) && S_ISDIR(st.st_mode));
> > }
> >
> >+/* removes the last path component from 'path' except if 'path' is root */
> >+static void strip_last_component(struct strbuf *path)
> >+{
> >+	if (path->len > offset_1st_component(path->buf)) {
> >+		char *last_slash = find_last_dir_sep(path->buf);
> >+		strbuf_setlen(path, last_slash - path->buf);
> >+	}
> >+}
> 
> This implementation is not correct because when the input is "/foo",
> the result is "" when it should be "/". Also, can the input be a
> non-normalized path? When the input is "foo///bar", should the
> result be "foo" or would "foo//" be an acceptable result? I think it
> should be the former. find_last_dir_sep() returns the last of the
> three slashes, not the first one. Therefore, I've rewritten the
> function thusly:
> 
> static void strip_last_component(struct strbuf *path)
> {
> 	size_t offset = offset_1st_component(path->buf);
> 	size_t len = path->len;
> 	while (offset < len && !is_dir_sep(path->buf[len - 1]))
> 		len--;
> 	while (offset < len && is_dir_sep(path->buf[len - 1]))
> 		len--;
> 	strbuf_setlen(path, len);
> }
> 

Thanks for that catch.  So your rewrite takes the offset of the 1st
component and ensures that we don't cut that part off.  It first strips
all non directory separators and then all directory separators.  This
way "/foo////bar" becomes "/foo" and as you pointed out "/foo" would
become "/".  The offset would also take care of windows drive letters
and the like.  Looks good.  Thanks!

> >+		strbuf_addbuf(&resolved, &next);
> >+
> >+		if (lstat(resolved.buf, &st)) {
> >+			/* error out unless this was the last component */
> >+			if (!(errno == ENOENT && !remaining.len)) {
> 
> Perhaps it was easier to _write_ the condition in this way, but I
> would have an easier time to _read_ it when it is
> 
> 			if (errno != ENOENT || remaining.len) {
> 

Yes I did write it out weird, mostly because it made the most sense for
what I was trying to accomplish (add path components must exist, except
for the very last one).  I'm fine applying DeMorgan's since it looks a
little cleaner.

> >+
> >+			if (is_absolute_path(symlink.buf)) {
> >+				/* absolute symlink; set resolved to root */
> >+				int offset = offset_1st_component(symlink.buf);
> >+				strbuf_reset(&resolved);
> >+				strbuf_add(&resolved, symlink.buf, offset);
> >+				strbuf_remove(&symlink, 0, offset);
> 
> Good. I would have expected some kind of "goto repeat;" because when
> we encounter a symlink to an absolute path, most, if not all, work
> done so far is obsoleted. But I haven't thought too deeply about
> this.

It made the most sense to just reuse the same looping condition that
I already had in place.  Resetting the resolved string to be the root
component of the absolute symlink made it easy to "throw away" all the
old work to allow us to start from scratch again.

-- 
Brandon Williams