From: Junio C Hamano <gitster@pobox•com>
To: Martin Erik Werner <martinerikwerner@gmail•com>
Cc: tboegi@web•de, git@vger•kernel.org, richih@debian•org
Subject: Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths
Date: Mon, 27 Jan 2014 08:31:37 -0800 [thread overview]
Message-ID: <xmqqd2jdpes6.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1390781250-20389-2-git-send-email-martinerikwerner@gmail.com> (Martin Erik Werner's message of "Mon, 27 Jan 2014 01:07:30 +0100")
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"
next prev parent reply other threads:[~2014-01-27 16:31 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-01-15 12:48 git-mv with absolute path derefereces symlinks Martin Erik Werner
2014-01-26 14:22 ` [PATCH 0/2] in-tree symlink handling with absolute paths Martin Erik Werner
2014-01-26 14:22 ` [PATCH 1/2] t0060: Add test for manipulating symlinks via " Martin Erik Werner
2014-01-26 14:22 ` [PATCH 2/2] setup: Don't dereference in-tree symlinks for " Martin Erik Werner
2014-01-26 17:19 ` Torsten Bögershausen
2014-01-27 0:07 ` Martin Erik Werner
2014-01-27 0:07 ` [PATCH v2 " Martin Erik Werner
2014-01-27 0:49 ` Duy Nguyen
2014-01-27 16:31 ` Junio C Hamano [this message]
2014-01-31 20:21 ` [PATCH v3 0/4] " Martin Erik Werner
2014-02-02 1:59 ` [PATCH v4 0/4] Handling of " Martin Erik Werner
2014-02-02 1:59 ` [PATCH v4 1/4] t0060: Add test for manipulating symlinks via " Martin Erik Werner
2014-02-02 1:59 ` [PATCH v4 2/4] t0060: Add test for prefix_path when path == work tree Martin Erik Werner
2014-02-02 1:59 ` [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function Martin Erik Werner
2014-02-02 2:19 ` Duy Nguyen
2014-02-02 2:23 ` Duy Nguyen
2014-02-02 11:13 ` Martin Erik Werner
2014-02-02 11:21 ` David Kastrup
2014-02-02 11:37 ` Torsten Bögershausen
2014-02-02 12:09 ` Martin Erik Werner
2014-02-02 12:27 ` Torsten Bögershausen
2014-02-02 12:15 ` Duy Nguyen
2014-02-02 1:59 ` [PATCH v4 4/4] setup: Don't dereference in-tree symlinks for absolute paths Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 0/5] Handling of " Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 1/5] t0060: Add test for manipulating symlinks via " Martin Erik Werner
2014-02-03 18:50 ` Junio C Hamano
2014-02-03 19:52 ` Junio C Hamano
2014-02-03 20:12 ` Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 2/5] t0060: Add test for prefix_path when path == work tree Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 3/5] t0060: Add tests for prefix_path when path begins with " Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function Martin Erik Werner
2014-02-03 21:00 ` Junio C Hamano
2014-02-03 23:16 ` Martin Erik Werner
2014-02-04 18:09 ` Junio C Hamano
2014-02-04 18:32 ` Martin Erik Werner
2014-02-02 16:35 ` [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths Martin Erik Werner
2014-02-03 4:15 ` Duy Nguyen
2014-02-03 13:17 ` Martin Erik Werner
2014-02-04 0:05 ` Junio C Hamano
2014-02-04 14:25 ` [PATCH v6 0/6] Handling of " Martin Erik Werner
2014-02-04 14:25 ` [PATCH v6 1/6] t3004: Add test for ls-files on symlinks via " Martin Erik Werner
2014-02-04 14:25 ` [PATCH v6 2/6] t0060: Add test for prefix_path " Martin Erik Werner
2014-02-04 14:25 ` [PATCH v6 3/6] t0060: Add test for prefix_path when path == work tree Martin Erik Werner
2014-02-04 14:25 ` [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with " Martin Erik Werner
2014-02-04 20:00 ` Torsten Bögershausen
2014-02-04 20:07 ` Junio C Hamano
2014-02-04 14:25 ` [PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function Martin Erik Werner
2014-02-04 19:18 ` Junio C Hamano
2014-02-04 14:25 ` [PATCH v6 6/6] setup: Don't dereference in-tree symlinks for absolute paths Martin Erik Werner
2014-01-31 20:22 ` [PATCH v3 1/4] t0060: Add test for manipulating symlinks via " Martin Erik Werner
2014-01-31 20:22 ` [PATCH v3 2/4] t0060: Add test for prefix_path when path == work tree Martin Erik Werner
2014-01-31 20:22 ` [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function Martin Erik Werner
2014-01-31 22:37 ` Torsten Bögershausen
2014-02-01 1:31 ` Martin Erik Werner
2014-02-01 2:31 ` Duy Nguyen
2014-01-31 20:23 ` [PATCH v3 4/4] setup: Don't dereference in-tree symlinks for absolute paths Martin Erik Werner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqd2jdpes6.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=martinerikwerner@gmail$(echo .)com \
--cc=richih@debian$(echo .)org \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox