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

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
[off-list ref] wrote:

quoted

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 <redacted>

quoted

        } 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 [off-list ref]