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

2014-02-03 Thread Martin Erik Werner 
On Mon, Feb 03, 2014 at 11:15:57AM +0700, Duy Nguyen wrote:
 On Sun, Feb 2, 2014 at 11:35 PM, Martin Erik Werner
 martinerikwer...@gmail.com wrote:
  diff --git a/setup.c b/setup.c
  index a2e60ab..230505c 100644
  --- a/setup.c
  +++ b/setup.c
  @@ -86,11 +86,23 @@ 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);
  +   char *npath;
  +
  +   npath = xmalloc(strlen(path) + 1);
  if (remaining_prefix)
  *remaining_prefix = 0;
  +   if (normalize_path_copy_len(npath, path, remaining_prefix)) 
  {
  +   free(npath);
  +   return NULL;
  +   }
  +   if (abspath_part_inside_repo(npath)) {
  +   free(npath);
  +   return NULL;
  +   }
  +
  +   sanitized = xmalloc(strlen(npath) + 1);
  +   strcpy(sanitized, npath);
  +   free(npath);
 
 We could replace these three lines with sanitized = npath;. But it's
 not a big deal imo. The rest of the series looks good.
 
 Reviewed-by: Duy Nguyen pclo...@gmail.com
 
  } else {
  sanitized = xmalloc(len + strlen(path) + 1);
  if (len)
 -- 
 Duy

Thank you for reviewing! And thanks Torsten and Junio Likewise. (And
thanks Richard for initial triggering and brief discussion of the bug :)

Hmm, yeah I don't really know what to prefer out of a. Two mallocs with
only a minimal one returned or 2. Single, potentially non-minimal, malloc
returned, if it makes little difference, for simplicity the latter seems nicer.

Then it seems like one could get rid of npath completely:

diff --git a/setup.c b/setup.c
index 230505c..dd120cd 100644
--- a/setup.c
+++ b/setup.c
@@ -88,21 +88,17 @@ char *prefix_path_gently(const char *prefix, int len,
if (is_absolute_path(orig)) {
char *npath;
 
-   npath = xmalloc(strlen(path) + 1);
+   sanitized = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
-   if (normalize_path_copy_len(npath, path, remaining_prefix)) {
-   free(npath);
+   if (normalize_path_copy_len(sanitized, path, remaining_prefix)) 
{
+   free(sanitized);
return NULL;
}
-   if (abspath_part_inside_repo(npath)) {
-   free(npath);
+   if (abspath_part_inside_repo(sanitized)) {
+   free(sanitized);
return NULL;
}
-
-   sanitized = xmalloc(strlen(npath) + 1);
-   strcpy(sanitized, npath);
-   free(npath);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)

at the cost of 'sanitized' always being the length of path, regardless
if it's shorter, or even a NUL string.

--
Martin Erik Werner martinerikwer...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-02-03 Thread Junio C Hamano 
Martin Erik Werner martinerikwer...@gmail.com writes:

 Then it seems like one could get rid of npath completely:

Yes.  And you need to remove its definition as well to avoid unused
variable warning.

Will queue with an obvious fix-up.

Thanks.


 diff --git a/setup.c b/setup.c
 index 230505c..dd120cd 100644
 --- a/setup.c
 +++ b/setup.c
 @@ -88,21 +88,17 @@ char *prefix_path_gently(const char *prefix, int len,
   if (is_absolute_path(orig)) {
   char *npath;
  
 - npath = xmalloc(strlen(path) + 1);
 + sanitized = xmalloc(strlen(path) + 1);
   if (remaining_prefix)
   *remaining_prefix = 0;
 - if (normalize_path_copy_len(npath, path, remaining_prefix)) {
 - free(npath);
 + if (normalize_path_copy_len(sanitized, path, remaining_prefix)) 
 {
 + free(sanitized);
   return NULL;
   }
 - if (abspath_part_inside_repo(npath)) {
 - free(npath);
 + if (abspath_part_inside_repo(sanitized)) {
 + free(sanitized);
   return NULL;
   }
 -
 - sanitized = xmalloc(strlen(npath) + 1);
 - strcpy(sanitized, npath);
 - free(npath);
   } else {
   sanitized = xmalloc(len + strlen(path) + 1);
   if (len)

 at the cost of 'sanitized' always being the length of path, regardless
 if it's shorter, or even a NUL string.

 --
 Martin Erik Werner martinerikwer...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths

2014-02-02 Thread Martin Erik Werner 
The 'prefix_path_gently' function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree. In order to manipulate symlinks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.

Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the in-repo part (without dereferencing
any symlinks inside the work tree).

For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.

Fixes t0060-82.

Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
 setup.c   | 36 
 t/t0060-path-utils.sh |  2 +-
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/setup.c b/setup.c
index a2e60ab..230505c 100644
--- a/setup.c
+++ b/setup.c
@@ -86,11 +86,23 @@ 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);
+   char *npath;
+
+   npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+   if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+   free(npath);
+   return NULL;
+   }
+   if (abspath_part_inside_repo(npath)) {
+   free(npath);
+   return NULL;
+   }
+
+   sanitized = xmalloc(strlen(npath) + 1);
+   strcpy(sanitized, npath);
+   free(npath);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -98,26 +110,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 c0a14f6..f04b82d 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 absolute paths to work 
tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work 
tree symlinks' '
ln -s target symlink 
test $(test-path-utils prefix_path prefix $(pwd)/symlink) = 
symlink
 '
-- 
1.8.5.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-02-02 Thread Duy Nguyen 
On Sun, Feb 2, 2014 at 11:35 PM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
 diff --git a/setup.c b/setup.c
 index a2e60ab..230505c 100644
 --- a/setup.c
 +++ b/setup.c
 @@ -86,11 +86,23 @@ 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);
 +   char *npath;
 +
 +   npath = xmalloc(strlen(path) + 1);
 if (remaining_prefix)
 *remaining_prefix = 0;
 +   if (normalize_path_copy_len(npath, path, remaining_prefix)) {
 +   free(npath);
 +   return NULL;
 +   }
 +   if (abspath_part_inside_repo(npath)) {
 +   free(npath);
 +   return NULL;
 +   }
 +
 +   sanitized = xmalloc(strlen(npath) + 1);
 +   strcpy(sanitized, npath);
 +   free(npath);

We could replace these three lines with sanitized = npath;. But it's
not a big deal imo. The rest of the series looks good.

Reviewed-by: Duy Nguyen pclo...@gmail.com

 } else {
 sanitized = xmalloc(len + strlen(path) + 1);
 if (len)
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html