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

From: Martin Erik Werner <martinerikwerner@gmail.com>
Date: Feb 3, 2014, 13:17 UTC
Message-ID: <20140203131700.GA15607@mule>
In-Reply-To: <CACsJy8DX8bh2cAx+a_cJafAOYB7Ly=y28jAGo1L8NEmkWaZv=Q@mail.gmail.com>

On Mon, Feb 03, 2014 at 11:15:57AM +0700, Duy Nguyen wrote:

Show 41 quoted lines

> On Sun, Feb 2, 2014 at 11:35 PM, Martin Erik Werner
> <martinerikwerner@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 <pclouds@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 <martinerikwerner@gmail.com>