# [PATCH] checkout-index.c: Unconditionally free memory 12 messages from 2015-05-01 to 2015-05-03. Participants: Stefan Beller, Eric Sunshine, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/39214 ## Stefan Beller, 2015-05-01 19:28 Subject: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: <1430508507-14016-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1430508507-14016-1-git-send-email-sbeller%40google.com ``` 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 --- 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 ``` ## Eric Sunshine, 2015-05-01 19:33 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQr5wQ6nfy90%2BSWyFFv0Neq4oJiEtur4cBJNGHNKXhZrQ%40mail.gmail.com In-Reply-To: <1430508507-14016-1-git-send-email-sbeller@google.com> ``` On Fri, May 1, 2015 at 3:28 PM, 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". Why doesn't the 'p' in the 'while' loop just below deserve the same treatment? > Signed-off-by: Stefan Beller > --- > 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 ``` ## Eric Sunshine, 2015-05-01 19:41 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: URL: https://gitlist.dev/e/CAPig%2BcR7V%2B9JLsCiJ_kmUBJc8_1Wwn6%3DO7uxQSEbogOGFKj8FQ%40mail.gmail.com In-Reply-To: ``` On Fri, May 1, 2015 at 3:33 PM, Eric Sunshine wrote: > On Fri, May 1, 2015 at 3:28 PM, 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". > > Why doesn't the 'p' in the 'while' loop just below deserve the same treatment? Ditto update-index.c:do_unresolved(). ``` ## Jeff King, 2015-05-01 19:50 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: <20150501195017.GA3493@peff.net> URL: https://gitlist.dev/e/20150501195017.GA3493%40peff.net In-Reply-To: <1430508507-14016-1-git-send-email-sbeller@google.com> ``` 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 ``` ## Stefan Beller, 2015-05-01 22:33 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: URL: https://gitlist.dev/e/CAGZ79kYNR6JNU%3DvVL_Pp0axFX_cpu1Bi5x226GUbeFdum9wkZg%40mail.gmail.com In-Reply-To: <20150501195017.GA3493@peff.net> ``` On Fri, May 1, 2015 at 12:50 PM, Jeff King 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 ``` ## Stefan Beller, 2015-05-01 22:35 Subject: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: <1430519737-6224-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1430519737-6224-1-git-send-email-sbeller%40google.com In-Reply-To: <1430508507-14016-1-git-send-email-sbeller@google.com> ``` 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 --- 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 ``` ## Jeff King, 2015-05-01 22:39 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: <20150501223913.GA1179@peff.net> URL: https://gitlist.dev/e/20150501223913.GA1179%40peff.net In-Reply-To: ``` 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 ``` ## Jeff King, 2015-05-01 22:43 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: <20150501224334.GB1179@peff.net> URL: https://gitlist.dev/e/20150501224334.GB1179%40peff.net In-Reply-To: <1430519737-6224-1-git-send-email-sbeller@google.com> ``` 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 ``` ## Eric Sunshine, 2015-05-01 23:41 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRCKDD--Px-FvrV1h8Os7WjYKQXN%2B5kh1WzVDoud9NJmg%40mail.gmail.com In-Reply-To: <1430519737-6224-1-git-send-email-sbeller@google.com> ``` On Fri, May 1, 2015 at 6:35 PM, 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". 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 > --- > 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 ``` ## Junio C Hamano, 2015-05-03 02:15 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: URL: https://gitlist.dev/e/xmqqpp6iwgnn.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150501224334.GB1179@peff.net> ``` Jeff King 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. ``` ## Jeff King, 2015-05-03 02:30 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: <20150503023024.GA4269@peff.net> URL: https://gitlist.dev/e/20150503023024.GA4269%40peff.net In-Reply-To: ``` On Sat, May 02, 2015 at 07:15:08PM -0700, Junio C Hamano wrote: > Jeff King 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 ``` ## Junio C Hamano, 2015-05-03 03:03 Subject: Re: [PATCH] checkout-index.c: Unconditionally free memory Message-ID: URL: https://gitlist.dev/e/xmqqbni2wefo.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150503023024.GA4269@peff.net> ``` Jeff King 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. ```