{"thread":{"id":"46894","subject":"[PATCH] branch: reset instead of release a strbuf","startedAt":"2017-10-03T21:14:18Z","lastAt":"2017-10-04T05:27:42Z","messageCount":9,"participants":["Stefan Beller","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"329606","messageId":"20171003211409.16037-1-sbeller@google.com","threadId":"46894","inReplyTo":null,"subject":"[PATCH] branch: reset instead of release a strbuf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-03T21:14:09Z","receivedAt":"2017-10-03T21:14:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Our documentation advises to not re-use a strbuf, after strbuf_release\nhas been called on it. Use the proper reset instead.\n\nSigned-off-by: Stefan Beller \n---\n\nMaybe one of the #leftoverbits is to remove the re-init call in release\nand see what breaks? (And then fixing up more of such cases as presented\nin this patch)\n\nThanks,\nStefan\n\n builtin/branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b998e16d0c..9758012390 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (!head_rev)\n \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n \t}\n-\tfor (i = 0; i < argc; i++, strbuf_release(&bname)) {\n+\tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n \n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"329609","messageId":"20171003214646.GZ19555@aiede.mtv.corp.google.com","threadId":"46894","inReplyTo":"20171003211409.16037-1-sbeller@google.com","subject":"Re: [PATCH] branch: reset instead of release a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-03T21:46:46Z","receivedAt":"2017-10-03T21:46:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Our documentation advises to not re-use a strbuf, after strbuf_release\n> has been called on it. Use the proper reset instead.\n\nI'm super surprised at this documentation. strbuf_release maintains\nthe invariant that a strbuf is always usable (i.e., that we do not have\nto fear use-after-free problems).\n\nOn second thought, though, strbuf_release is a more expensive operation\nthan strbuf_reset --- constantly free()-ing and re-malloc()ing is\nunnecessary churn in malloc data structures. So maybe that is the\nmotivation here?\n\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Maybe one of the #leftoverbits is to remove the re-init call in release\n> and see what breaks? (And then fixing up more of such cases as presented\n> in this patch)\n\nAs mentioned above: please no. That would be problematic both for\nease of development and for the risk of security bugs.\n\n> builtin/branch.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index b998e16d0c..9758012390 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> \t\tif (!head_rev)\n> \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n> \t}\n> -\tfor (i = 0; i < argc; i++, strbuf_release(&bname)) {\n> +\tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n> \t\tchar *target = NULL;\n> \t\tint flags = 0;\n\nShould there be a strbuf_release (or UNLEAK if you are very very sure)\ncall at the end of the function to replace this?\n\nWith that change (but not without it),\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"329612","messageId":"20171003221740.26325-1-sbeller@google.com","threadId":"46894","inReplyTo":"20171003214646.GZ19555@aiede.mtv.corp.google.com","subject":"[PATCH] branch: reset instead of release a strbuf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-03T22:17:40Z","receivedAt":"2017-10-03T22:17:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Our documentation advises to not re-use a strbuf, after strbuf_release\nhas been called on it. Use the proper reset instead.\n\nCurrently 'strbuf_release' releases and re-initializes the strbuf, so it\nis safe, but slow. 'strbuf_reset' only resets the internal length variable,\nsuch that this could also be accounted for as a micro-optimization.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n builtin/branch.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b998e16d0c..71ed1c7036 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\tif (!head_rev)\n \t\t\tdie(_(\"Couldn't look up commit object for HEAD\"));\n \t}\n-\tfor (i = 0; i < argc; i++, strbuf_release(&bname)) {\n+\tfor (i = 0; i < argc; i++, strbuf_reset(&bname)) {\n \t\tchar *target = NULL;\n \t\tint flags = 0;\n \n@@ -282,8 +282,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t}\n \n \tfree(name);\n+\tstrbuf_release(&bname);\n \n-\treturn(ret);\n+\treturn ret;\n }\n \n static int calc_maxwidth(struct ref_array *refs, int remote_bonus)\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"329613","messageId":"20171003222414.GC19555@aiede.mtv.corp.google.com","threadId":"46894","inReplyTo":"20171003221740.26325-1-sbeller@google.com","subject":"Re: [PATCH] branch: reset instead of release a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-03T22:24:14Z","receivedAt":"2017-10-03T22:24:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> Our documentation advises to not re-use a strbuf, after strbuf_release\n> has been called on it. Use the proper reset instead.\n>\n> Reviewed-by: Jonathan Nieder \n\nThis is indeed\nReviewed-by: Jonathan Nieder \n\nThank you.\n\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/branch.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n\nHere's a patch to address the surprising strbuf.h advice.\n\n-- >8 --\nSubject: strbuf: do not encourage init-after-release\n\nstrbuf_release already leaves the strbuf in a valid, initialized\nstate, so there is not need to call strbuf_init after it.\n\nMoreover, this is not likely to change in the future: strbuf_release\nleaving the strbuf in a valid state has been easy to maintain and has\nbeen very helpful for Git's robustness and simplicity (e.g.,\npreventing use-after-free vulnerabilities).\n\nIt is still not advisable to call strbuf_release until done using a\nstrbuf because it is wasteful, so keep that part of the advice.\n\nReported-by: Stefan Beller \nSigned-off-by: Jonathan Nieder \n---\n strbuf.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 7496cb8ec5..6e175c3694 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -83,7 +83,7 @@ extern void strbuf_init(struct strbuf *, size_t);\n \n /**\n * Release a string buffer and the memory it used. You should not use the\n- * string buffer after using this function, unless you initialize it again.\n+ * string buffer after using this function.\n */\n extern void strbuf_release(struct strbuf *);\n \n-- \n2.14.2.920.gcf0c67979c\n\n"},{"id":"329623","messageId":"20171003234919.qsr54ncmw6cihowi@sigill.intra.peff.net","threadId":"46894","inReplyTo":"20171003222414.GC19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH] branch: reset instead of release a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-03T23:49:19Z","receivedAt":"2017-10-03T23:49:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2017 at 03:24:14PM -0700, Jonathan Nieder wrote:\n\n> Here's a patch to address the surprising strbuf.h advice.\n> \n> -- >8 --\n> Subject: strbuf: do not encourage init-after-release\n> \n> strbuf_release already leaves the strbuf in a valid, initialized\n> state, so there is not need to call strbuf_init after it.\n> \n> Moreover, this is not likely to change in the future: strbuf_release\n> leaving the strbuf in a valid state has been easy to maintain and has\n> been very helpful for Git's robustness and simplicity (e.g.,\n> preventing use-after-free vulnerabilities).\n\nThanks for picking this up. Like you, I was quite surprised when I saw\nStefan's original patch.\n\n> diff --git a/strbuf.h b/strbuf.h\n> index 7496cb8ec5..6e175c3694 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -83,7 +83,7 @@ extern void strbuf_init(struct strbuf *, size_t);\n> \n> /**\n> * Release a string buffer and the memory it used. You should not use the\n> - * string buffer after using this function, unless you initialize it again.\n> + * string buffer after using this function.\n> */\n> extern void strbuf_release(struct strbuf *);\n\nI think it's actually OK to use the string buffer after this function.\nIt's just an empty string.\n\nPerhaps we should be more explicit: this releases any resources and\nresets to a pristine, empty state. I suspect strbuf_detach() probably\nshould make the same claim.\n\nEarlier you mentioned:\n\n> It is still not advisable to call strbuf_release until done using a\n> strbuf because it is wasteful, so keep that part of the advice.\n\nIs this what you meant? If so, I think we should probably be more\nexplicit in giving people a hint to use strbuf_reset() for efficiency.\n\n-Peff\n"},{"id":"329633","messageId":"xmqqvajvfydm.fsf@gitster.mtv.corp.google.com","threadId":"46894","inReplyTo":"20171003234919.qsr54ncmw6cihowi@sigill.intra.peff.net","subject":"Re: [PATCH] branch: reset instead of release a strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T02:19:33Z","receivedAt":"2017-10-04T02:19:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> /**\n>> * Release a string buffer and the memory it used. You should not use the\n>> - * string buffer after using this function, unless you initialize it again.\n>> + * string buffer after using this function.\n>> */\n>> extern void strbuf_release(struct strbuf *);\n>\n> I think it's actually OK to use the string buffer after this function.\n> It's just an empty string.\n>\n> Perhaps we should be more explicit: this releases any resources and\n> resets to a pristine, empty state. I suspect strbuf_detach() probably\n> should make the same claim.\n>\n> Earlier you mentioned:\n>\n>> It is still not advisable to call strbuf_release until done using a\n>> strbuf because it is wasteful, so keep that part of the advice.\n>\n> Is this what you meant? If so, I think we should probably be more\n> explicit in giving people a hint to use strbuf_reset() for efficiency.\n\nYes, \"should not use\" above is simply misleading. Either drop it\naltogether, or say something like\n\n\tIf you find yourself reusing the same strbuf in a loop and\n\tcalling strbuf_release() each iteration, you may want to\n\tconsider if it makes more sense to use strbuf_reset()\n\tinstead in each iteration and calling strbuf_release() at\n\tthe end.\n\nperhaps.\n"},{"id":"329636","messageId":"20171004023954.GK19555@aiede.mtv.corp.google.com","threadId":"46894","inReplyTo":"20171003234919.qsr54ncmw6cihowi@sigill.intra.peff.net","subject":"[PATCH v2] strbuf doc: reuse after strbuf_release is fine","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-10-04T02:39:54Z","receivedAt":"2017-10-04T02:40:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"strbuf_release leaves the strbuf in a valid, initialized state, so\nthere is no need to call strbuf_init after it.\n\nMoreover, this is not likely to change in the future: strbuf_release\nleaving the strbuf in a valid state has been easy to maintain and has\nbeen very helpful for Git's robustness and simplicity (e.g.,\npreventing use-after-free vulnerabilities).\n\nDocument the semantics so the next generation of Git developers can\nbecome familiar with them without reading the implementation. It is\nstill not advisable to call strbuf_release too often because it is\nwasteful, so add a note pointing to strbuf_reset for that.\n\nThe same semantics apply to strbuf_detach. Add a similar note to its\ndocstring to make that clear.\n\nImproved-by: Jeff King \nSigned-off-by: Jonathan Nieder \n---\nJeff King wrote:\n\n> I think it's actually OK to use the string buffer after this function.\n> It's just an empty string.\n>\n> Perhaps we should be more explicit: this releases any resources and\n> resets to a pristine, empty state. I suspect strbuf_detach() probably\n> should make the same claim.\n\nLike this?\n\nThanks,\nJonathan\n\n strbuf.h | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 7496cb8ec5..249df86711 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -82,8 +82,12 @@ extern char strbuf_slopbuf[];\n extern void strbuf_init(struct strbuf *, size_t);\n \n /**\n- * Release a string buffer and the memory it used. You should not use the\n- * string buffer after using this function, unless you initialize it again.\n+ * Release a string buffer and the memory it used. After this call, the\n+ * strbuf points to an empty string that does not need to be free()ed, as\n+ * if it had been set to `STRBUF_INIT` and never modified.\n+ *\n+ * To clear a strbuf in preparation for further use without the overhead\n+ * of free()ing and malloc()ing again, use strbuf_reset() instead.\n */\n extern void strbuf_release(struct strbuf *);\n \n@@ -91,6 +95,9 @@ extern void strbuf_release(struct strbuf *);\n * Detach the string from the strbuf and returns it; you now own the\n * storage the string occupies and it is your responsibility from then on\n * to release it with `free(3)` when you are done with it.\n+ *\n+ * The strbuf that previously held the string is reset to `STRBUF_INIT` so\n+ * it can be reused after calling this function.\n */\n extern char *strbuf_detach(struct strbuf *, size_t *);\n \n-- \n2.14.2.920.gcf0c67979c\n\n"},{"id":"329647","messageId":"xmqq376zecdj.fsf@gitster.mtv.corp.google.com","threadId":"46894","inReplyTo":"20171004023954.GK19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] strbuf doc: reuse after strbuf_release is fine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T05:00:08Z","receivedAt":"2017-10-04T05:00:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> strbuf_release leaves the strbuf in a valid, initialized state, so\n> there is no need to call strbuf_init after it.\n>\n> Moreover, this is not likely to change in the future: strbuf_release\n> leaving the strbuf in a valid state has been easy to maintain and has\n> been very helpful for Git's robustness and simplicity (e.g.,\n> preventing use-after-free vulnerabilities).\n>\n> Document the semantics so the next generation of Git developers can\n> become familiar with them without reading the implementation. It is\n> still not advisable to call strbuf_release too often because it is\n> wasteful, so add a note pointing to strbuf_reset for that.\n>\n> The same semantics apply to strbuf_detach. Add a similar note to its\n> docstring to make that clear.\n>\n> Improved-by: Jeff King \n> Signed-off-by: Jonathan Nieder \n> ---\n> Jeff King wrote:\n>\n>> I think it's actually OK to use the string buffer after this function.\n>> It's just an empty string.\n>>\n>> Perhaps we should be more explicit: this releases any resources and\n>> resets to a pristine, empty state. I suspect strbuf_detach() probably\n>> should make the same claim.\n>\n> Like this?\n\nLooks good to me.\n\n\n>\n> Thanks,\n> Jonathan\n>\n> strbuf.h | 11 +++++++++--\n> 1 file changed, 9 insertions(+), 2 deletions(-)\n>\n> diff --git a/strbuf.h b/strbuf.h\n> index 7496cb8ec5..249df86711 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -82,8 +82,12 @@ extern char strbuf_slopbuf[];\n> extern void strbuf_init(struct strbuf *, size_t);\n> \n> /**\n> - * Release a string buffer and the memory it used. You should not use the\n> - * string buffer after using this function, unless you initialize it again.\n> + * Release a string buffer and the memory it used. After this call, the\n> + * strbuf points to an empty string that does not need to be free()ed, as\n> + * if it had been set to `STRBUF_INIT` and never modified.\n> + *\n> + * To clear a strbuf in preparation for further use without the overhead\n> + * of free()ing and malloc()ing again, use strbuf_reset() instead.\n> */\n> extern void strbuf_release(struct strbuf *);\n> \n> @@ -91,6 +95,9 @@ extern void strbuf_release(struct strbuf *);\n> * Detach the string from the strbuf and returns it; you now own the\n> * storage the string occupies and it is your responsibility from then on\n> * to release it with `free(3)` when you are done with it.\n> + *\n> + * The strbuf that previously held the string is reset to `STRBUF_INIT` so\n> + * it can be reused after calling this function.\n> */\n> extern char *strbuf_detach(struct strbuf *, size_t *);\n"},{"id":"329651","messageId":"20171004052736.i3vykavpbcfcztfr@sigill.intra.peff.net","threadId":"46894","inReplyTo":"20171004023954.GK19555@aiede.mtv.corp.google.com","subject":"Re: [PATCH v2] strbuf doc: reuse after strbuf_release is fine","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-04T05:27:36Z","receivedAt":"2017-10-04T05:27:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 03, 2017 at 07:39:54PM -0700, Jonathan Nieder wrote:\n\n> > I think it's actually OK to use the string buffer after this function.\n> > It's just an empty string.\n> >\n> > Perhaps we should be more explicit: this releases any resources and\n> > resets to a pristine, empty state. I suspect strbuf_detach() probably\n> > should make the same claim.\n> \n> Like this?\n\nYes, perfect. Thanks!\n\n-Peff\n"}]}