[PATCH] checkout-index.c: Unconditionally free memory

[PATCH] checkout-index.c: Unconditionally free memory

[Stefan Beller]

It's safe to free the char pointer `p` unconditionally.

The pointer is assigned just 2 lines earlier as a return from
prefix_path, which allocates new memory for its return value.

Then it is used in checkout_file, which passes the pointer on to
cache_name_pos and write_tempfile_record, both of which do not store
the pointer in any permanent record.

So the condition on when to free the pointer is just "always".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/checkout-index.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 9ca2da1..e28dc35 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -249,8 +249,8 @@ 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((char *)p);
 	}
 
 	if (read_from_stdin) {
-- 
2.4.0.rc3.16.g0ab00b9

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Eric Sunshine]

On Fri, May 1, 2015 at 3:28 PM, Stefan Beller <sbeller@google.com> wrote:
> It's safe to free the char pointer `p` unconditionally.
>
> The pointer is assigned just 2 lines earlier as a return from
> prefix_path, which allocates new memory for its return value.
>
> Then it is used in checkout_file, which passes the pointer on to
> cache_name_pos and write_tempfile_record, both of which do not store
> the pointer in any permanent record.
>
> So the condition on when to free the pointer is just "always".

Why doesn't the 'p' in the 'while' loop just below deserve the same treatment?

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..e28dc35 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -249,8 +249,8 @@ 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((char *)p);
>         }
>
>         if (read_from_stdin) {
> --
> 2.4.0.rc3.16.g0ab00b9
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Eric Sunshine]

On Fri, May 1, 2015 at 3:33 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, May 1, 2015 at 3:28 PM, Stefan Beller <sbeller@google.com> wrote:
>> It's safe to free the char pointer `p` unconditionally.
>>
>> The pointer is assigned just 2 lines earlier as a return from
>> prefix_path, which allocates new memory for its return value.
>>
>> Then it is used in checkout_file, which passes the pointer on to
>> cache_name_pos and write_tempfile_record, both of which do not store
>> the pointer in any permanent record.
>>
>> So the condition on when to free the pointer is just "always".
>
> Why doesn't the 'p' in the 'while' loop just below deserve the same treatment?

Ditto update-index.c:do_unresolved().

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Jeff King]

On Fri, May 01, 2015 at 12:28:27PM -0700, Stefan Beller wrote:

> It's safe to free the char pointer `p` unconditionally.
> 
> The pointer is assigned just 2 lines earlier as a return from
> prefix_path, which allocates new memory for its return value.
> 
> Then it is used in checkout_file, which passes the pointer on to
> cache_name_pos and write_tempfile_record, both of which do not store
> the pointer in any permanent record.
> 
> So the condition on when to free the pointer is just "always".

That of course makes me wonder why somebody would write this in the
first place. :)

It looks like it comes from be65e7d (Fix users of prefix_path() to
free() only when necessary, 2006-05-07), which claims that prefix_path
sometimes does not allocate. When did that change? Looks like maybe
d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
2008-01-28), but it certainly is the case now.

Probably all of the other sites touched by be65e7d could use the same
treatment.

-Peff

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Stefan Beller]

On Fri, May 1, 2015 at 12:50 PM, Jeff King <peff@peff.net> wrote:
> On Fri, May 01, 2015 at 12:28:27PM -0700, Stefan Beller wrote:
>
>> It's safe to free the char pointer `p` unconditionally.
>>
>> The pointer is assigned just 2 lines earlier as a return from
>> prefix_path, which allocates new memory for its return value.
>>
>> Then it is used in checkout_file, which passes the pointer on to
>> cache_name_pos and write_tempfile_record, both of which do not store
>> the pointer in any permanent record.
>>
>> So the condition on when to free the pointer is just "always".
>
> That of course makes me wonder why somebody would write this in the
> first place. :)
>
> It looks like it comes from be65e7d (Fix users of prefix_path() to
> free() only when necessary, 2006-05-07), which claims that prefix_path
> sometimes does not allocate. When did that change? Looks like maybe
> d089eba (setup: sanitize absolute and funny paths in get_pathspec(),
> 2008-01-28), but it certainly is the case now.
>
> Probably all of the other sites touched by be65e7d could use the same
> treatment.

I looked around, just as Eric suggested as well and found those too.
I don't think I'll track down the history of when this change became possible,
but I'd rather point to (f5114a40c0d0, 2011-08-04, git-check-attr:
Normalize paths),
where the result of prefix_path is freed unconditionally already.

>
> -Peff

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Jeff King]

On Fri, May 01, 2015 at 03:33:13PM -0700, Stefan Beller wrote:

> > Probably all of the other sites touched by be65e7d could use the same
> > treatment.
> 
> I looked around, just as Eric suggested as well and found those too.
> I don't think I'll track down the history of when this change became possible,
> but I'd rather point to (f5114a40c0d0, 2011-08-04, git-check-attr:
> Normalize paths),
> where the result of prefix_path is freed unconditionally already.

Ah, right. I noticed there were two extras in be65e7d that Eric hadn't
mentioned, but I didn't actually check to see if they were already
fixed. It looks like they are (further evidence that this is a good
thing to be doing :) ).

-Peff

[PATCH] checkout-index.c: Unconditionally free memory

[Stefan Beller]

It's safe to free the char pointer `p` unconditionally.

The pointer is assigned just 2 lines earlier as a return from
prefix_path, which allocates new memory for its return value.

Then it is used in checkout_file, which passes the pointer on to
cache_name_pos and write_tempfile_record, both of which do not store
the pointer in any permanent record.
So the condition on when to free the pointer is just "always".

Looking at the history this behavior must be fixed since at least
(f5114a40c0d0, 2011-08-04, git-check-attr: Normalize paths), where
the result of prefix_path is freed unconditionally.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/checkout-index.c | 6 ++----
 builtin/update-index.c   | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 9ca2da1..5325f92 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -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((char *)p);
 	}
 
 	if (read_from_stdin) {
@@ -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((char *)p);
 		}
 		strbuf_release(&nbuf);
 		strbuf_release(&buf);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6271b54..584efa5 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -534,8 +534,7 @@ static int do_unresolve(int ac, const char **av,
 		const char *arg = av[i];
 		const char *p = prefix_path(prefix, prefix_length, arg);
 		err |= unresolve_one(p);
-		if (p < arg || p > arg + strlen(arg))
-			free((char *)p);
+		free((char *)p);
 	}
 	return err;
 }
-- 
2.4.0.rc3.16.g0ab00b9

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Jeff King]

On Fri, May 01, 2015 at 03:35:37PM -0700, Stefan Beller wrote:

> Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory

Looks like the patch has expanded beyond checkout-index.c. Maybe:

  unconditionally free result of prefix_path

would be more descriptive? I usually like the "area:" prefix, but I
think here the common thread is not an area, but that they are return
values from prefix_path.

> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..5325f92 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -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((char *)p);

Can we just drop the "const" from the declaration of "p"? Then we don't
have to do this funny cast here. It looks like the same applies to the
other callsites, and even the other uses of prefix_path in
update-index.c.

-Peff

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, May 01, 2015 at 03:35:37PM -0700, Stefan Beller wrote:
>
>> Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory
>
> Looks like the patch has expanded beyond checkout-index.c. Maybe:
>
>   unconditionally free result of prefix_path
>
> would be more descriptive? I usually like the "area:" prefix, but I
> think here the common thread is not an area, but that they are return
> values from prefix_path.

Sure, the prefix could even be "prefix_path(): $message", I would
think.

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Jeff King]

On Sat, May 02, 2015 at 07:15:08PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, May 01, 2015 at 03:35:37PM -0700, Stefan Beller wrote:
> >
> >> Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory
> >
> > Looks like the patch has expanded beyond checkout-index.c. Maybe:
> >
> >   unconditionally free result of prefix_path
> >
> > would be more descriptive? I usually like the "area:" prefix, but I
> > think here the common thread is not an area, but that they are return
> > values from prefix_path.
> 
> Sure, the prefix could even be "prefix_path(): $message", I would
> think.

I almost suggested that, but it not a change to prefix_path at all, but
rather to its callers. That may be getting nit-picky, though. :)

-Peff

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Sure, the prefix could even be "prefix_path(): $message", I would
>> think.
>
> I almost suggested that, but it not a change to prefix_path at all, but
> rather to its callers. That may be getting nit-picky, though. :)

Using "prefix_path(): free returned memory in the callers" as the
title, with the body that explains that an earlier fix to make that
a requirement but forgot to do so for a few that are fixed in this
commit, would be sensible, no?  It is not about how prefix_path() is
implemented but its returned value, which is still about that
function.

Re: [PATCH] checkout-index.c: Unconditionally free memory

[Eric Sunshine]

On Fri, May 1, 2015 at 6:35 PM, Stefan Beller <sbeller@google.com> wrote:
> It's safe to free the char pointer `p` unconditionally.
>
> The pointer is assigned just 2 lines earlier as a return from
> prefix_path, which allocates new memory for its return value.
>
> Then it is used in checkout_file, which passes the pointer on to
> cache_name_pos and write_tempfile_record, both of which do not store
> the pointer in any permanent record.
> So the condition on when to free the pointer is just "always".

In addition to Peff's comment about the Subject: being incorrect for
this updated patch, the commit message itself is specific to
checkout-index.c by mentioning only cache_name_pos() and
write_tempfile_record(), neither of which are applicable to
update-index.c.

In fact, the commit message, by talking about 'p' and this or that
called function, is probably too detailed. It should be more than
sufficient merely to observe that the string returned by prefix_path()
is always newly allocated (and, parenthetically, that that result is
never stored away for later use), therefore freeing it unconditionally
is the correct thing to do.

> Looking at the history this behavior must be fixed since at least
> (f5114a40c0d0, 2011-08-04, git-check-attr: Normalize paths), where
> the result of prefix_path is freed unconditionally.

This is a strange justification. How does the reader know that the
author of that change was disposing of the result of prefix_path()
properly? Rather than increasing the reader's confidence that your
change is correct, this sort of hand-wavy argument decreases
confidence.

The change which made prefix_path() always return a newly allocated
string was d089eba (setup: sanitize absolute and funny paths in
get_pathspec(), 2008-01-28), so why not cite that instead?

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
> index 9ca2da1..5325f92 100644
> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -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((char *)p);
>         }
>
>         if (read_from_stdin) {
> @@ -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((char *)p);
>                 }
>                 strbuf_release(&nbuf);
>                 strbuf_release(&buf);
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 6271b54..584efa5 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -534,8 +534,7 @@ static int do_unresolve(int ac, const char **av,
>                 const char *arg = av[i];
>                 const char *p = prefix_path(prefix, prefix_length, arg);
>                 err |= unresolve_one(p);
> -               if (p < arg || p > arg + strlen(arg))
> -                       free((char *)p);
> +               free((char *)p);
>         }
>         return err;
>  }
> --
> 2.4.0.rc3.16.g0ab00b9