Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths

[Duy Nguyen <pclouds@gmail.com>]

On Mon, Jan 27, 2014 at 7:07 AM, Martin Erik Werner
<martinerikwerner@gmail.com> wrote:
> 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 {

Could we remove this part and let the while loop handle the first path
component too? The only difference I see is if this code matches, we
have a trailing slash, while the "while" loop does not have a trailing
slash in wtpart.

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

This "+ 1" is to ignore '/', isn't it? If so we should not do if match
is set 1 outside "while" loop.

> +               free(npath);
> +               free(wtpart);

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.

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