{"thread":{"id":"15092","subject":"[PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test","startedAt":"2008-08-19T18:46:30Z","lastAt":"2008-08-22T00:16:30Z","messageCount":8,"participants":["Jim Meyering","Alex Riesen","Brandon Casey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"87730","messageId":"87k5ecx2pl.fsf@rho.meyering.net","threadId":"15092","inReplyTo":null,"subject":"[PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test","fromName":"Jim Meyering","fromEmail":"meyering@redhat.com","sentAt":"2008-08-19T18:46:30Z","receivedAt":"2008-08-19T18:46:30Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"We removed a handful of these useless if-before-free tests\nseveral months ago. This change removes a new one that snuck back in.\n\nSigned-off-by: Jim Meyering \n---\nThere are four in regex.c, too, but that's imported code,\nso probably not worth modifying in git:\n\n compat/regex.c: if (var) free (var)\n compat/regex.c: if (preg->buffer != NULL)\n free (preg->buffer)\n compat/regex.c: if (preg->fastmap != NULL)\n free (preg->fastmap)\n compat/regex.c: if (preg->translate != NULL)\n free (preg->translate)\n\n remote.c | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex f61a3ab..105668f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -579,8 +579,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \tstruct refspec *refspec;\n\n \trefspec = parse_refspec_internal(1, fetch_refspec, 1, 1);\n-\tif (refspec)\n-\t\tfree(refspec);\n+\tfree(refspec);\n \treturn !!refspec;\n }\n\n--\n1.6.0.9.gae2e487\n"},{"id":"87850","messageId":"20080820194002.GC16626@blimp.local","threadId":"15092","inReplyTo":"87k5ecx2pl.fsf@rho.meyering.net","subject":"Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-08-20T19:40:02Z","receivedAt":"2008-08-20T19:40:02Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Jim Meyering, Tue, Aug 19, 2008 20:46:30 +0200:\n> We removed a handful of these useless if-before-free tests\n> several months ago. This change removes a new one that snuck back in.\n\nSomething used to crash in free when passed NULL. It's one of the\nreasons why some cross-platform projects have xfree which does the\ncheck (sometimes depending on platform). Admittedly, it is a long time\nsince I saw such a platform (some old SunOS it was, I believe).\n"},{"id":"87866","messageId":"878wursag2.fsf@rho.meyering.net","threadId":"15092","inReplyTo":"20080820194002.GC16626@blimp.local","subject":"Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2008-08-20T20:22:53Z","receivedAt":"2008-08-20T20:22:53Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Alex Riesen wrote:\n> Jim Meyering, Tue, Aug 19, 2008 20:46:30 +0200:\n>> We removed a handful of these useless if-before-free tests\n>> several months ago. This change removes a new one that snuck back in.\n>\n> Something used to crash in free when passed NULL. It's one of the\n> reasons why some cross-platform projects have xfree which does the\n> check (sometimes depending on platform). Admittedly, it is a long time\n> since I saw such a platform (some old SunOS it was, I believe).\n\nRight. This was discussed at length back in February:\n\n http://thread.gmane.org/gmane.comp.version-control.git/74187\n"},{"id":"87905","messageId":"pI8EXbuWhkm4pM0AaaTAPQQYBeFSffOSzCiM4NfNHKomivEcfUfShA@cipher.nrlssc.navy.mil","threadId":"15092","inReplyTo":"87k5ecx2pl.fsf@rho.meyering.net","subject":"Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-08-20T23:38:44Z","receivedAt":"2008-08-20T23:38:44Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Jim Meyering wrote:\n> We removed a handful of these useless if-before-free tests\n> several months ago. This change removes a new one that snuck back in.\n\n> diff --git a/remote.c b/remote.c\n> index f61a3ab..105668f 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -579,8 +579,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n> \tstruct refspec *refspec;\n> \n> \trefspec = parse_refspec_internal(1, fetch_refspec, 1, 1);\n> -\tif (refspec)\n> -\t\tfree(refspec);\n> +\tfree(refspec);\n> \treturn !!refspec;\n> }\n\nMaybe we should also begin the process of not leaking memory here...\n\ndiff --git a/remote.c b/remote.c\nindex 7f2897b..984ad1b 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -449,6 +449,20 @@ static int verify_refname(char *name, int is_glob)\n\treturn result;\n }\n \n+void free_refspecs(struct refspec *refspec, int nr_refspec)\n+{\n+\tint i;\n+\n+\tif (!refspec)\n+\t\treturn;\n+\n+\tfor (i = 0; i < nr_refspec; i++) {\n+\t\tfree(refspec[i].src);\n+\t\tfree(refspec[i].dst);\n+\t}\n+\tfree(refspec);\n+}\n+\n static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)\n {\n \tint i;\n@@ -567,7 +581,12 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \n invalid:\n \tif (verify) {\n-\t\tfree(rs);\n+\t\t/*\n+\t\t * nr_refspec must be greater than zero and i must be valid\n+\t\t * since it is only possible to reach this point from within\n+\t\t * the for loop above.\n+\t\t */\n+\t\tfree_refspecs(rs, i+1);\n \t\treturn NULL;\n \t}\n \tdie(\"Invalid refspec '%s'\", refspec[i]);\n@@ -579,8 +598,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \tstruct refspec *refspec;\n \n \trefspec = parse_refspec_internal(1, fetch_refspec, 1, 1);\n-\tif (refspec)\n-\t\tfree(refspec);\n+\tfree_refspecs(refspec, 1);\n \treturn !!refspec;\n }\n \ndiff --git a/remote.h b/remote.h\nindex 091b1d0..2601f6e 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -78,6 +78,7 @@ void ref_remove_duplicates(struct ref *ref_map);\n int valid_fetch_refspec(const char *refspec);\n struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);\n+void free_refspecs(struct refspec *refspec, int nr_refspec);\n \n int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t int nr_refspec, const char **refspec, int all);\n"},{"id":"87908","messageId":"7vr68jmdd2.fsf@gitster.siamese.dyndns.org","threadId":"15092","inReplyTo":"pI8EXbuWhkm4pM0AaaTAPQQYBeFSffOSzCiM4NfNHKomivEcfUfShA@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-21T00:16:25Z","receivedAt":"2008-08-21T00:16:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Maybe we should also begin the process of not leaking memory here...\n>\n> diff --git a/remote.c b/remote.c\n> index 7f2897b..984ad1b 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -449,6 +449,20 @@ static int verify_refname(char *name, int is_glob)\n> \treturn result;\n> }\n> \n> +void free_refspecs(struct refspec *refspec, int nr_refspec)\n> +{\n> +\tint i;\n> +\n> +\tif (!refspec)\n> +\t\treturn;\n> +\n> +\tfor (i = 0; i < nr_refspec; i++) {\n> +\t\tfree(refspec[i].src);\n> +\t\tfree(refspec[i].dst);\n> +\t}\n> +\tfree(refspec);\n> +}\n\nAre you sure all the codepaths that stuff refspec[].{src,dst} give\nfreeable pointer? E.g. if anybody splits a originally single string\n\"refs/heads/foo:refs/remotes/origin/foo\" into two by replacing the colon\nwith NUL and pointing the halves, and/or such string came from argv[]\nwithout strdup(), I'd imagine free() would not like you very much.\n\nI didn't look, though.\n"},{"id":"87916","messageId":"aS-Yl0OfC9_Ks79Dz2qAKZMrGX1F6_mTNtSFuB45LyarCC5Zmb8agA@cipher.nrlssc.navy.mil","threadId":"15092","inReplyTo":"7vr68jmdd2.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-08-21T00:33:11Z","receivedAt":"2008-08-21T00:33:11Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> Maybe we should also begin the process of not leaking memory here...\n>>\n>> diff --git a/remote.c b/remote.c\n>> index 7f2897b..984ad1b 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -449,6 +449,20 @@ static int verify_refname(char *name, int is_glob)\n>> \treturn result;\n>> }\n>> \n>> +void free_refspecs(struct refspec *refspec, int nr_refspec)\n>> +{\n>> +\tint i;\n>> +\n>> +\tif (!refspec)\n>> +\t\treturn;\n>> +\n>> +\tfor (i = 0; i < nr_refspec; i++) {\n>> +\t\tfree(refspec[i].src);\n>> +\t\tfree(refspec[i].dst);\n>> +\t}\n>> +\tfree(refspec);\n>> +}\n> \n> Are you sure all the codepaths that stuff refspec[].{src,dst} give\n> freeable pointer?\n\nremote.c:parse_refspec_internal() always does. This function is the\nproducer of the refspec that is being passed to free_refspecs() in the two\nplaces where the patch called it.\n\nThe code paths for each additionally use of free_refspecs would have to\ncheck that it is safe. Perhaps a comment is in order.\n\nIf you don't think we're ready for free_refspecs we can still call free()\nmanually in the two places I called free_refspecs.\n\n-brandon\n"},{"id":"87922","messageId":"7vpro3ks15.fsf@gitster.siamese.dyndns.org","threadId":"15092","inReplyTo":"aS-Yl0OfC9_Ks79Dz2qAKZMrGX1F6_mTNtSFuB45LyarCC5Zmb8agA@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] * remote.c (valid_fetch_refspec): remove useless if-before-free test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-21T02:42:30Z","receivedAt":"2008-08-21T02:42:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n>> Are you sure all the codepaths that stuff refspec[].{src,dst} give\n>> freeable pointer?\n>\n> remote.c:parse_refspec_internal() always does. This function is the\n> producer of the refspec that is being passed to free_refspecs() in the two\n> places where the patch called it.\n>\n> The code paths for each additionally use of free_refspecs would have to\n> check that it is safe. Perhaps a comment is in order.\n>\n> If you don't think we're ready for free_refspecs we can still call free()\n> manually in the two places I called free_refspecs.\n\nThanks.\n\nA generic helper like this is preferable, as long as (1) you made sure\nexisting callsites are safe, and (2) the helper is clearly commented\nagainst misuse by future callsites.\n\nI personally do not think it would be a bad idea to even make a rule that\nrefspec[].{src,dst} _must_ be freeable pointers. After all, we may have\nto deal with repositories with insane number of refs (not in millions but\ncertainly in thousands), but it would be insane to have a remote with\ninsane number of refspecs (even in hundreds).\n"},{"id":"88067","messageId":"wFl399-gCL5aWJVzIQInmJ7pEp0R-MPJKSzJw3vAxISbcHFg2DaDTA@cipher.nrlssc.navy.mil","threadId":"15092","inReplyTo":"7vpro3ks15.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] remote.c: add a function for deleting a refspec array and use it (twice)","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-08-22T00:16:30Z","receivedAt":"2008-08-22T00:16:30Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"A number of call sites allocate memory for a refspec array, populate\nits members with heap memory, and then free only the refspec pointer\nwhile leaking the memory allocated for the member elements. Provide\na function for freeing the elements of a refspec array and the array\nitself.\n\nCaution to callers: code paths must be checked to ensure that the\nrefspec members \"src\" and \"dst\" can be passed to free.\n\nSigned-off-by: Brandon Casey \n---\n remote.c | 29 +++++++++++++++++++++++++++--\n remote.h | 1 +\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 105668f..3ef09a4 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -449,6 +449,26 @@ static int verify_refname(char *name, int is_glob)\n \treturn result;\n }\n \n+/*\n+ * This function frees a refspec array.\n+ * Warning: code paths should be checked to ensure that the src\n+ * and dst pointers are always freeable pointers as well\n+ * as the refspec pointer itself.\n+ */\n+void free_refspecs(struct refspec *refspec, int nr_refspec)\n+{\n+\tint i;\n+\n+\tif (!refspec)\n+\t\treturn;\n+\n+\tfor (i = 0; i < nr_refspec; i++) {\n+\t\tfree(refspec[i].src);\n+\t\tfree(refspec[i].dst);\n+\t}\n+\tfree(refspec);\n+}\n+\n static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)\n {\n \tint i;\n@@ -567,7 +587,12 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \n invalid:\n \tif (verify) {\n-\t\tfree(rs);\n+\t\t/*\n+\t\t * nr_refspec must be greater than zero and i must be valid\n+\t\t * since it is only possible to reach this point from within\n+\t\t * the for loop above.\n+\t\t */\n+\t\tfree_refspecs(rs, i+1);\n \t\treturn NULL;\n \t}\n \tdie(\"Invalid refspec '%s'\", refspec[i]);\n@@ -579,7 +604,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \tstruct refspec *refspec;\n \n \trefspec = parse_refspec_internal(1, fetch_refspec, 1, 1);\n-\tfree(refspec);\n+\tfree_refspecs(refspec, 1);\n \treturn !!refspec;\n }\n \ndiff --git a/remote.h b/remote.h\nindex 091b1d0..2601f6e 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -78,6 +78,7 @@ void ref_remove_duplicates(struct ref *ref_map);\n int valid_fetch_refspec(const char *refspec);\n struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);\n+void free_refspecs(struct refspec *refspec, int nr_refspec);\n \n int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t int nr_refspec, const char **refspec, int all);\n-- \n1.6.0.21.g35a2e\n"}]}