[PATCH] prefix_path(): Unconditionally free result of prefix_path

[PATCH] prefix_path(): Unconditionally free result of prefix_path

[Stefan Beller]

prefix_path() always returns a newly allocated string since
d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
2008-01-28)

Additionally the const is dropped from the pointers, so the call to
free doesn't need a cast.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Notes:
    Thanks for all the suggestions!
    They are incorporated into this version of the patch.

 builtin/checkout-index.c | 10 ++++------
 builtin/update-index.c   |  5 ++---
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 9ca2da1..8028c37 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -241,7 +241,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	/* Check out named files first */
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
-		const char *p;
+		char *p;
 
 		if (all)
 			die("git checkout-index: don't mix '--all' and explicit filenames");
@@ -249,8 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			die("git checkout-index: don't mix '--stdin' and explicit filenames");
 		p = prefix_path(prefix, prefix_length, arg);
 		checkout_file(p, prefix);
-		if (p < arg || p > arg + strlen(arg))
-			free((char *)p);
+		free(p);
 	}
 
 	if (read_from_stdin) {
@@ -260,7 +259,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			die("git checkout-index: don't mix '--all' and '--stdin'");
 
 		while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
-			const char *p;
+			char *p;
 			if (line_termination && buf.buf[0] == '"') {
 				strbuf_reset(&nbuf);
 				if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -269,8 +268,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			}
 			p = prefix_path(prefix, prefix_length, buf.buf);
 			checkout_file(p, prefix);
-			if (p < buf.buf || p > buf.buf + buf.len)
-				free((char *)p);
+			free(p);
 		}
 		strbuf_release(&nbuf);
 		strbuf_release(&buf);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6271b54..a92eed2 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -532,10 +532,9 @@ static int do_unresolve(int ac, const char **av,
 
 	for (i = 1; i < ac; i++) {
 		const char *arg = av[i];
-		const char *p = prefix_path(prefix, prefix_length, arg);
+		char *p = prefix_path(prefix, prefix_length, arg);
 		err |= unresolve_one(p);
-		if (p < arg || p > arg + strlen(arg))
-			free((char *)p);
+		free(p);
 	}
 	return err;
 }
-- 
2.4.0.rc3.16.g0ab00b9

Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path

[Eric Sunshine]

On Mon, May 4, 2015 at 3:11 PM, Stefan Beller <sbeller@google.com> wrote:
> prefix_path(): Unconditionally free result of prefix_path

Slightly redundant mention of "prefix_path". Also, prevailing custom
is to drop capitalization.

> prefix_path() always returns a newly allocated string since
> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
> 2008-01-28)

I'd probably turn this sentence fragment into a proper sentence:

    As of d089eba (...), prefix_path() always returns a newly
    allocated string, so free its result unconditionally.

> Additionally the const is dropped from the pointers, so the call to
> free doesn't need a cast.

Imperative mood:

    Additionally, drop the const from variables to which the
    prefix_path() result is assigned so they can be free()'d
    without having to cast-away constness.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     Thanks for all the suggestions!
>     They are incorporated into this version of the patch.

Thanks, this version looks much better.

FWIW, with or without addressing the very minor nits above:

Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..8028c37 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -241,7 +241,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>         /* Check out named files first */
>         for (i = 0; i < argc; i++) {
>                 const char *arg = argv[i];
> -               const char *p;
> +               char *p;
>
>                 if (all)
>                         die("git checkout-index: don't mix '--all' and explicit filenames");
> @@ -249,8 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>                         die("git checkout-index: don't mix '--stdin' and explicit filenames");
>                 p = prefix_path(prefix, prefix_length, arg);
>                 checkout_file(p, prefix);
> -               if (p < arg || p > arg + strlen(arg))
> -                       free((char *)p);
> +               free(p);
>         }
>
>         if (read_from_stdin) {
> @@ -260,7 +259,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>                         die("git checkout-index: don't mix '--all' and '--stdin'");
>
>                 while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
> -                       const char *p;
> +                       char *p;
>                         if (line_termination && buf.buf[0] == '"') {
>                                 strbuf_reset(&nbuf);
>                                 if (unquote_c_style(&nbuf, buf.buf, NULL))
> @@ -269,8 +268,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>                         }
>                         p = prefix_path(prefix, prefix_length, buf.buf);
>                         checkout_file(p, prefix);
> -                       if (p < buf.buf || p > buf.buf + buf.len)
> -                               free((char *)p);
> +                       free(p);
>                 }
>                 strbuf_release(&nbuf);
>                 strbuf_release(&buf);
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 6271b54..a92eed2 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -532,10 +532,9 @@ static int do_unresolve(int ac, const char **av,
>
>         for (i = 1; i < ac; i++) {
>                 const char *arg = av[i];
> -               const char *p = prefix_path(prefix, prefix_length, arg);
> +               char *p = prefix_path(prefix, prefix_length, arg);
>                 err |= unresolve_one(p);
> -               if (p < arg || p > arg + strlen(arg))
> -                       free((char *)p);
> +               free(p);
>         }
>         return err;
>  }
> --
> 2.4.0.rc3.16.g0ab00b9

Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, May 4, 2015 at 3:11 PM, Stefan Beller <sbeller@google.com> wrote:
>> prefix_path(): Unconditionally free result of prefix_path
>
> Slightly redundant mention of "prefix_path". Also, prevailing custom
> is to drop capitalization.
>
>> prefix_path() always returns a newly allocated string since
>> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
>> 2008-01-28)
>
> I'd probably turn this sentence fragment into a proper sentence:
>
>     As of d089eba (...), prefix_path() always returns a newly
>     allocated string, so free its result unconditionally.
>
>> Additionally the const is dropped from the pointers, so the call to
>> free doesn't need a cast.
>
> Imperative mood:
>
>     Additionally, drop the const from variables to which the
>     prefix_path() result is assigned so they can be free()'d
>     without having to cast-away constness.
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>>     Thanks for all the suggestions!
>>     They are incorporated into this version of the patch.
>
> Thanks, this version looks much better.
>
> FWIW, with or without addressing the very minor nits above:
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Thanks, both.

To save a round-trip, I'll munge the log message myself stealing
Eric's suggestions.

Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path

[Jeff King]

On Mon, May 04, 2015 at 12:11:54PM -0700, Stefan Beller wrote:

> prefix_path() always returns a newly allocated string since
> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
> 2008-01-28)
> 
> Additionally the const is dropped from the pointers, so the call to
> free doesn't need a cast.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
> Notes:
>     Thanks for all the suggestions!
>     They are incorporated into this version of the patch.
> 
>  builtin/checkout-index.c | 10 ++++------
>  builtin/update-index.c   |  5 ++---
>  2 files changed, 6 insertions(+), 9 deletions(-)

Should we also squash in these sites? I think they are adequately
covered under the proposed log message.

Found by grepping for prefix_path calls. The only remainders are:

  1. in blame, we assign the result to a const char that may also point
     straight into to argv, but we never actually free either way

  2. test-path-utils does not free at all, but we probably don't care
     either way

diff --git a/builtin/update-index.c b/builtin/update-index.c
index a92eed2..0665b31 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -870,14 +870,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		case PARSE_OPT_DONE:
 		{
 			const char *path = ctx.argv[0];
-			const char *p;
+			char *p;
 
 			setup_work_tree();
 			p = prefix_path(prefix, prefix_length, path);
 			update_one(p);
 			if (set_executable_bit)
 				chmod_path(set_executable_bit, p);
-			free((char *)p);
+			free(p);
 			ctx.argc--;
 			ctx.argv++;
 			break;
@@ -908,7 +908,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 		setup_work_tree();
 		while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
-			const char *p;
+			char *p;
 			if (line_termination && buf.buf[0] == '"') {
 				strbuf_reset(&nbuf);
 				if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -919,7 +919,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			update_one(p);
 			if (set_executable_bit)
 				chmod_path(set_executable_bit, p);
-			free((char *)p);
+			free(p);
 		}
 		strbuf_release(&nbuf);
 		strbuf_release(&buf);

Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path

[Stefan Beller]

On Mon, May 4, 2015 at 8:21 PM, Jeff King <peff@peff.net> wrote:
> On Mon, May 04, 2015 at 12:11:54PM -0700, Stefan Beller wrote:
>
>> prefix_path() always returns a newly allocated string since
>> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
>> 2008-01-28)
>>
>> Additionally the const is dropped from the pointers, so the call to
>> free doesn't need a cast.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>>     Thanks for all the suggestions!
>>     They are incorporated into this version of the patch.
>>
>>  builtin/checkout-index.c | 10 ++++------
>>  builtin/update-index.c   |  5 ++---
>>  2 files changed, 6 insertions(+), 9 deletions(-)
>
> Should we also squash in these sites? I think they are adequately
> covered under the proposed log message.

That sounds good to me.

>
> Found by grepping for prefix_path calls. The only remainders are:
>
>   1. in blame, we assign the result to a const char that may also point
>      straight into to argv, but we never actually free either way
>
>   2. test-path-utils does not free at all, but we probably don't care
>      either way
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index a92eed2..0665b31 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -870,14 +870,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>                 case PARSE_OPT_DONE:
>                 {
>                         const char *path = ctx.argv[0];
> -                       const char *p;
> +                       char *p;
>
>                         setup_work_tree();
>                         p = prefix_path(prefix, prefix_length, path);
>                         update_one(p);
>                         if (set_executable_bit)
>                                 chmod_path(set_executable_bit, p);
> -                       free((char *)p);
> +                       free(p);
>                         ctx.argc--;
>                         ctx.argv++;
>                         break;
> @@ -908,7 +908,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>
>                 setup_work_tree();
>                 while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
> -                       const char *p;
> +                       char *p;
>                         if (line_termination && buf.buf[0] == '"') {
>                                 strbuf_reset(&nbuf);
>                                 if (unquote_c_style(&nbuf, buf.buf, NULL))
> @@ -919,7 +919,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>                         update_one(p);
>                         if (set_executable_bit)
>                                 chmod_path(set_executable_bit, p);
> -                       free((char *)p);
> +                       free(p);
>                 }
>                 strbuf_release(&nbuf);
>                 strbuf_release(&buf);

Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Should we also squash in these sites? I think they are adequately
>> covered under the proposed log message.
>
> That sounds good to me.
>>
>> Found by grepping for prefix_path calls. The only remainders are:
>>
>>   1. in blame, we assign the result to a const char that may also point
>>      straight into to argv, but we never actually free either way

The return value from add_prefix() that is what prefix_path()
returned eventually becomes scoreboard.path that needs to be kept
during the lifetime of the process, and I think there isn't much
point doing the "free() immediately before exiting".

>>   2. test-path-utils does not free at all, but we probably don't care
>>      either way

Anyway, here is what I'd queue for now.

-- >8 --
From: Stefan Beller <sbeller@google.com>
Date: Mon, 4 May 2015 12:11:54 -0700
Subject: [PATCH] prefix_path(): unconditionally free results in the callers

As of d089ebaa (setup: sanitize absolute and funny paths in
get_pathspec(), 2008-01-28), prefix_path() always returns a
newly allocated string, so callers should free its result.

Additionally, drop the const from variables to which the result of
the prefix_path() is assigned, so they can be free()'d without
having to cast-away the constness.

Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout-index.c | 10 ++++------
 builtin/update-index.c   | 13 ++++++-------
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 9ca2da1..8028c37 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -241,7 +241,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	/* Check out named files first */
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
-		const char *p;
+		char *p;
 
 		if (all)
 			die("git checkout-index: don't mix '--all' and explicit filenames");
@@ -249,8 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			die("git checkout-index: don't mix '--stdin' and explicit filenames");
 		p = prefix_path(prefix, prefix_length, arg);
 		checkout_file(p, prefix);
-		if (p < arg || p > arg + strlen(arg))
-			free((char *)p);
+		free(p);
 	}
 
 	if (read_from_stdin) {
@@ -260,7 +259,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			die("git checkout-index: don't mix '--all' and '--stdin'");
 
 		while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
-			const char *p;
+			char *p;
 			if (line_termination && buf.buf[0] == '"') {
 				strbuf_reset(&nbuf);
 				if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -269,8 +268,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 			}
 			p = prefix_path(prefix, prefix_length, buf.buf);
 			checkout_file(p, prefix);
-			if (p < buf.buf || p > buf.buf + buf.len)
-				free((char *)p);
+			free(p);
 		}
 		strbuf_release(&nbuf);
 		strbuf_release(&buf);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6271b54..0665b31 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -532,10 +532,9 @@ static int do_unresolve(int ac, const char **av,
 
 	for (i = 1; i < ac; i++) {
 		const char *arg = av[i];
-		const char *p = prefix_path(prefix, prefix_length, arg);
+		char *p = prefix_path(prefix, prefix_length, arg);
 		err |= unresolve_one(p);
-		if (p < arg || p > arg + strlen(arg))
-			free((char *)p);
+		free(p);
 	}
 	return err;
 }
@@ -871,14 +870,14 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		case PARSE_OPT_DONE:
 		{
 			const char *path = ctx.argv[0];
-			const char *p;
+			char *p;
 
 			setup_work_tree();
 			p = prefix_path(prefix, prefix_length, path);
 			update_one(p);
 			if (set_executable_bit)
 				chmod_path(set_executable_bit, p);
-			free((char *)p);
+			free(p);
 			ctx.argc--;
 			ctx.argv++;
 			break;
@@ -909,7 +908,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 
 		setup_work_tree();
 		while (strbuf_getline(&buf, stdin, line_termination) != EOF) {
-			const char *p;
+			char *p;
 			if (line_termination && buf.buf[0] == '"') {
 				strbuf_reset(&nbuf);
 				if (unquote_c_style(&nbuf, buf.buf, NULL))
@@ -920,7 +919,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			update_one(p);
 			if (set_executable_bit)
 				chmod_path(set_executable_bit, p);
-			free((char *)p);
+			free(p);
 		}
 		strbuf_release(&nbuf);
 		strbuf_release(&buf);
-- 
2.4.0-311-gf1d9b8d

Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path

[Jeff King]

On Tue, May 05, 2015 at 10:36:38AM -0700, Junio C Hamano wrote:

> Stefan Beller <sbeller@google.com> writes:
> 
> >> Should we also squash in these sites? I think they are adequately
> >> covered under the proposed log message.
> >
> > That sounds good to me.
> >>
> >> Found by grepping for prefix_path calls. The only remainders are:
> >>
> >>   1. in blame, we assign the result to a const char that may also point
> >>      straight into to argv, but we never actually free either way
> 
> The return value from add_prefix() that is what prefix_path()
> returned eventually becomes scoreboard.path that needs to be kept
> during the lifetime of the process, and I think there isn't much
> point doing the "free() immediately before exiting".

Yeah, sorry, I meant to say that more explicitly, but clearly didn't. I
think it is fine as-is.

> Anyway, here is what I'd queue for now.

Looks good, thanks.

-Peff

Re: [PATCH] prefix_path(): Unconditionally free result of prefix_path

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> >>   1. in blame, we assign the result to a const char that may also point
>> >>      straight into to argv, but we never actually free either way
>> 
>> The return value from add_prefix() that is what prefix_path()
>> returned eventually becomes scoreboard.path that needs to be kept
>> during the lifetime of the process, and I think there isn't much
>> point doing the "free() immediately before exiting".
>
> Yeah, sorry, I meant to say that more explicitly, but clearly didn't. I
> think it is fine as-is.

Yeah, sorry, I didn't mean "You are wrong and here is why".  I was
merely agreeing with you.

Thanks for running grep over the codebase to check possible
remaining problems.