[PATCH v3 0/4] setup: Don't dereference in-tree symlinks for absolute paths

> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
> > In order to manipulate symliks in the
> > work tree using absolute paths, symlinks should only be dereferenced
> > outside the work tree.
> 
> I agree 100% with this reasoning (modulo s/symliks/symlinks/).
> 
> As to the implementation, it looks a bit overly complicated,
> though.  I haven't tried writing the same myself, but 
> 
>  * I suspect that strbuf would help simplifying the allocation and
>    deallocation;
> 
>  * Also I suspect that use of string-list to split and then join is
>    making the code unnecessarily complex. In Python/Perl, that would
>    be a normal approach, but in C, it would be a lot simpler if you
>    prepare a writable temporary in wtpart[], walk from left to right
>    finding '/' and replacing it temporarily with NUL to terminate in
>    order to check with real_path(), restore the NUL back to '/' to
>    check deeper, and rinse and repeat.
> 
>    Having said that, I am not absolutely sure if the repeated
>    calls to real_path() are doing the right thing, though ;-)
> 
> > diff --git a/setup.c b/setup.c
> > index 5432a31..0789a96 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
> >     const char *orig = path;
> >     char *sanitized;
> >     if (is_absolute_path(orig)) {
> > -           const char *temp = real_path(path);
> > -           sanitized = xmalloc(len + strlen(temp) + 1);
> > -           strcpy(sanitized, temp);
> > +           int i, match;
> > +           size_t wtpartlen;
> > +           char *npath, *wtpart;
> > +           struct string_list list = STRING_LIST_INIT_DUP;
> > +           const char *work_tree = get_git_work_tree();
> > +           if (!work_tree)
> > +                   return NULL;
> > +           npath = xmalloc(strlen(path) + 1);
> >             if (remaining_prefix)
> >                     *remaining_prefix = 0;
> > +           if (normalize_path_copy_len(npath, path, remaining_prefix)) {
> > +                   free(npath);
> > +                   return NULL;
> > +           }
> > +
> > +           string_list_split(&list, npath, '/', -1);
> > +           wtpart = xmalloc(strlen(npath) + 1);
> > +           i = 0;
> > +           match = 0;
> > +           strcpy(wtpart, list.items[i++].string);
> > +           strcat(wtpart, "/");
> > +           if (strcmp(real_path(wtpart), work_tree) == 0) {
> > +                   match = 1;
> > +           } else {
> > +                   while (i < list.nr) {
> > +                           strcat(wtpart, list.items[i++].string);
> > +                           if (strcmp(real_path(wtpart), work_tree) == 0) {
> > +                                   match = 1;
> > +                                   break;
> > +                           }
> > +                           strcat(wtpart, "/");
> > +                   }
> > +           }
> > +           string_list_clear(&list, 0);
> > +           if (!match) {
> > +                   free(npath);
> > +                   free(wtpart);
> > +                   return NULL;
> > +           }
> > +
> > +           wtpartlen = strlen(wtpart);
> > +           sanitized = xmalloc(strlen(npath) - wtpartlen);
> > +           strcpy(sanitized, npath + wtpartlen + 1);
> > +           free(npath);
> > +           free(wtpart);
> >     } else {
> >             sanitized = xmalloc(len + strlen(path) + 1);
> >             if (len)
> > @@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
> >             strcpy(sanitized + len, path);
> >             if (remaining_prefix)
> >                     *remaining_prefix = len;
> > -   }
> > -   if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
> > -           goto error_out;
> > -   if (is_absolute_path(orig)) {
> > -           size_t root_len, len, total;
> > -           const char *work_tree = get_git_work_tree();
> > -           if (!work_tree)
> > -                   goto error_out;
> > -           len = strlen(work_tree);
> > -           root_len = offset_1st_component(work_tree);
> > -           total = strlen(sanitized) + 1;
> > -           if (strncmp(sanitized, work_tree, len) ||
> > -               (len > root_len && sanitized[len] != '\0' && sanitized[len] != '/')) {
> > -           error_out:
> > +           if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {
> >                     free(sanitized);
> >                     return NULL;
> >             }
> > -           if (sanitized[len] == '/')
> > -                   len++;
> > -           memmove(sanitized, sanitized + len, total - len);
> >     }
> >     return sanitized;
> >  }
> > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> > index 3a0677a..03a12ac 100755
> > --- a/t/t0060-path-utils.sh
> > +++ b/t/t0060-path-utils.sh
> > @@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
> >     test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
> >  '
> >  
> > -test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '
> > +test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '
> >  
> >     ln -s target symlink &&
> >     test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"

> All this new code looks long enough to be a separate function with a
> meaningful name. And we could travese through each path component in
> npath without wtpart (replacing '/' with '\0' to terminate the string
> temporarily for real_path()). But it's up to you. Whichever way is
> easier to read to you.