Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Jan 27, 2014, 00:49 UTC
- Message-ID
- <CACsJy8AwBXtYTbQhcAfYgwuEGrzbMG9SDo8JOpSVF945O7VPyQ@mail.gmail.com>
- In-Reply-To
- <1390781250-20389-2-git-send-email-martinerikwerner@gmail.com>
On Mon, Jan 27, 2014 at 7:07 AM, Martin Erik Werner <martinerikwerner@gmail.com> wrote:
Show 30 quoted lines
> 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;Show 5 quoted lines
> + 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.
Show 19 quoted lines
> + 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.
Show 31 quoted lines
> } 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