{"thread":{"id":"48626","subject":"[PATCH 1/2] sequencer.c: plug leaks in do_pick_commit","startedAt":"2018-06-01T20:01:54Z","lastAt":"2018-06-21T11:46:38Z","messageCount":11,"participants":["Stefan Beller","Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"349017","messageId":"20180601200146.114919-1-sbeller@google.com","threadId":"48626","inReplyTo":null,"subject":"[PATCH 1/2] sequencer.c: plug leaks in do_pick_commit","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-01T20:01:45Z","receivedAt":"2018-06-01T20:01:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Going to leave, we additionally free the author and commit message\nand make sure to call update_abort_safety_file().\n\nSigned-off-by: Stefan Beller \n---\n\nThis patch can apply on master.\n\nThis is a follow up from \nhttps://public-inbox.org/git/nycvar.QRO.7.76.6.1805311402210.82@tvgsbejvaqbjf.bet/\n\n\n\n sequencer.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cca968043ea..b98690ecd41 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1773,7 +1773,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n \t\t\t\t\t &head, &msgbuf, opts);\n \t\tif (res < 0)\n-\t\t\treturn res;\n+\t\t\tgoto leave;\n+\n \t\tres |= write_message(msgbuf.buf, msgbuf.len,\n \t\t\t\t git_path_merge_msg(), 0);\n \t} else {\n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"349018","messageId":"20180601200146.114919-2-sbeller@google.com","threadId":"48626","inReplyTo":"20180601200146.114919-1-sbeller@google.com","subject":"[PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-01T20:01:46Z","receivedAt":"2018-06-01T20:01:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n sequencer.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex b98690ecd41..aba03e9429a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -175,6 +175,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n \t\t\twarning(_(\"invalid commit message cleanup mode '%s'\"),\n \t\t\t\t s);\n \n+\t\tfree(s);\n \t\treturn status;\n \t}\n \n-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"349203","messageId":"xmqq8t7v6zpd.fsf@gitster-ct.c.googlers.com","threadId":"48626","inReplyTo":"20180601200146.114919-2-sbeller@google.com","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T02:41:02Z","receivedAt":"2018-06-04T02:41:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Signed-off-by: Stefan Beller \n> ---\n> sequencer.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index b98690ecd41..aba03e9429a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -175,6 +175,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n> \t\t\twarning(_(\"invalid commit message cleanup mode '%s'\"),\n> \t\t\t\t s);\n> \n> +\t\tfree(s);\n> \t\treturn status;\n> \t}\n\nShouldn't 's' now lose 'const'? After all, git_config_string()\ngives you an allocated memory so...\n"},{"id":"349210","messageId":"xmqqmuwb5i7k.fsf@gitster-ct.c.googlers.com","threadId":"48626","inReplyTo":"xmqq8t7v6zpd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T03:44:15Z","receivedAt":"2018-06-04T03:44:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> sequencer.c | 1 +\n>> 1 file changed, 1 insertion(+)\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index b98690ecd41..aba03e9429a 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -175,6 +175,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n>> \t\t\twarning(_(\"invalid commit message cleanup mode '%s'\"),\n>> \t\t\t\t s);\n>> \n>> +\t\tfree(s);\n>> \t\treturn status;\n>> \t}\n>\n> Shouldn't 's' now lose 'const'? After all, git_config_string()\n> gives you an allocated memory so...\n\nYikes. Should git_config_string() and git_config_pathname() take\n\"char **dst\" instead of \"const char **\" as their out-location\nparameter? They both assign a pointer to an allocated piece of\nmemory for the caller to own or dispose of, but because of\nconst-ness of the pointee their first parameter has, a caller like\nthis one must declare \"const char *s\" yet is forced to call free()\nnot to leak the value when it is done.\n\n"},{"id":"349211","messageId":"20180604035637.GA15408@sigill.intra.peff.net","threadId":"48626","inReplyTo":"xmqqmuwb5i7k.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-04T03:56:37Z","receivedAt":"2018-06-04T03:56:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 04, 2018 at 12:44:15PM +0900, Junio C Hamano wrote:\n\n> >> diff --git a/sequencer.c b/sequencer.c\n> >> index b98690ecd41..aba03e9429a 100644\n> >> --- a/sequencer.c\n> >> +++ b/sequencer.c\n> >> @@ -175,6 +175,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n> >> \t\t\twarning(_(\"invalid commit message cleanup mode '%s'\"),\n> >> \t\t\t\t s);\n> >> \n> >> +\t\tfree(s);\n> >> \t\treturn status;\n> >> \t}\n> >\n> > Shouldn't 's' now lose 'const'? After all, git_config_string()\n> > gives you an allocated memory so...\n> \n> Yikes. Should git_config_string() and git_config_pathname() take\n> \"char **dst\" instead of \"const char **\" as their out-location\n> parameter? They both assign a pointer to an allocated piece of\n> memory for the caller to own or dispose of, but because of\n> const-ness of the pointee their first parameter has, a caller like\n> this one must declare \"const char *s\" yet is forced to call free()\n> not to leak the value when it is done.\n\nI've looked into it before, but that causes its own wave of headaches.\nThe source of the problem is that we do:\n\n const char *some_var = \"default\";\n ...\n git_config_string(&some_var, ...);\n\nSo sometimes some_var needs to be freed and sometimes not (and every one\nof those uses is a potential leak, but it's OK because they're all\nprogram-lifetime globals anyway, and people don't _tend_ to set the same\noption over and over, leaking heap memory). And C being C, we can't\nconvert a pointer-to-pointer-to-const into a pointer-to-pointer without\nan explicit cast.\n\nDoing it \"right\" in C would probably involve two variables:\n\n const char *some_var = \"default\";\n const char *some_var_storage = NULL;\n\n int git_config_string_smart(const char **ptr, char **storage,\n const char *var, const char *value)\n {\n ...\n\tfree(*storage);\n\t*ptr = *storage = xstrdup(value);\n\treturn 0;\n }\n\n #define GIT_CONFIG_STRING(name, var, value) \\\n git_config_string_smart(&(name), &(name##_storage), var, value)\n\nOr something like that.\n\nWe could also get away from an out-parameter and use the return type,\nsince the single-pointer conversion is OK. But the primary value of\ngit_config_string() is that it lets you return the error code as a\none-liner.\n\n-Peff\n"},{"id":"349212","messageId":"xmqqin6z5g8e.fsf@gitster-ct.c.googlers.com","threadId":"48626","inReplyTo":"20180604035637.GA15408@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T04:26:57Z","receivedAt":"2018-06-04T04:27:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I've looked into it before, but that causes its own wave of headaches.\n> The source of the problem is that we do:\n>\n> const char *some_var = \"default\";\n> ...\n> git_config_string(&some_var, ...);\n\nYup, that is a valid pattern for \"run once and let exit(3) clean\nafter us\" programs.\n\n> Doing it \"right\" in C would probably involve two variables:\n>\n> const char *some_var = \"default\";\n> const char *some_var_storage = NULL;\n>\n> int git_config_string_smart(const char **ptr, char **storage,\n> const char *var, const char *value)\n> {\n> ...\n> \tfree(*storage);\n> \t*ptr = *storage = xstrdup(value);\n> \treturn 0;\n> }\n>\n> #define GIT_CONFIG_STRING(name, var, value) \\\n> git_config_string_smart(&(name), &(name##_storage), var, value)\n>\n> Or something like that.\n\nThe attitude the approach takes is that \"run once and let exit(3)\nclean after us\" programs *should* care. And at that point, maybe\n\n\tchar *some_var = xstrdup(\"default\");\n\tgit_config_string(&some_var, ...);\n\nthat takes \"char **\" and frees the current storage before assigning\nto it may be simpler than the two-variable approach.\n\nBut you're right. We cannot just unconst the type and be done with\nit---there are associated clean-up necessary if we were to do this.\n\nThanks.\n"},{"id":"349215","messageId":"20180604045122.GE14451@sigill.intra.peff.net","threadId":"48626","inReplyTo":"xmqqin6z5g8e.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-04T04:51:22Z","receivedAt":"2018-06-04T04:51:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 04, 2018 at 01:26:57PM +0900, Junio C Hamano wrote:\n\n> > Doing it \"right\" in C would probably involve two variables:\n> >\n> > const char *some_var = \"default\";\n> > const char *some_var_storage = NULL;\n> >\n> > int git_config_string_smart(const char **ptr, char **storage,\n> > const char *var, const char *value)\n> > {\n> > ...\n> > \tfree(*storage);\n> > \t*ptr = *storage = xstrdup(value);\n> > \treturn 0;\n> > }\n> >\n> > #define GIT_CONFIG_STRING(name, var, value) \\\n> > git_config_string_smart(&(name), &(name##_storage), var, value)\n> >\n> > Or something like that.\n> \n> The attitude the approach takes is that \"run once and let exit(3)\n> clean after us\" programs *should* care.\n\nEven with \"let exit clean up\", we are still leaking heap every time the\nvariable is assigned after the first. Again, I don't think it matters\nthat much in practice, but I think:\n\n [core]\n editor = foo\n editor = foo\n ...etc...\n\nwould leak arbitrary memory during the config parse, that would be\nallocated for the remainder of the program. I guess you could say exit()\nis handling it, but I think the point is that we are letting exit()\nhandle memory that was potentially useful until we exit, not leaks. :)\n\n> And at that point, maybe\n> \n> \tchar *some_var = xstrdup(\"default\");\n> \tgit_config_string(&some_var, ...);\n> \n> that takes \"char **\" and frees the current storage before assigning\n> to it may be simpler than the two-variable approach.\n\nThat _is_ much nicer, but you cannot use xstrdup() as the initializer\nfor a global \"static char *some_var\", which is what the majority of the\nconfig variables are. It's this \"static initializer sometimes, run-time\nheap sometimes\" duality to the variables that makes handling it such a\npain.\n\nWith that strategy, we'd have to have a big initialize_defaults()\nfunction. Which actually might not be _too_ bad since we now have\ncommon-main.c, but:\n\n - it sucks to keep the default values far away from the declarations\n\n - it does carry a runtime cost. Not a huge one, but it sucks to pay it\n on every program startup, even if we're not using those variables.\n\n-Peff\n"},{"id":"349216","messageId":"xmqqa7sb5ew1.fsf@gitster-ct.c.googlers.com","threadId":"48626","inReplyTo":"20180604045122.GE14451@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-04T04:55:58Z","receivedAt":"2018-06-04T04:56:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> With that strategy, we'd have to have a big initialize_defaults()\n> function. Which actually might not be _too_ bad since we now have\n> common-main.c, but:\n>\n> - it sucks to keep the default values far away from the declarations\n>\n> - it does carry a runtime cost. Not a huge one, but it sucks to pay it\n> on every program startup, even if we're not using those variables.\n\nTrue, and s/even if/even though most of the time/ the situation is\neven worse X-<.\n"},{"id":"349217","messageId":"20180604050033.GA17818@sigill.intra.peff.net","threadId":"48626","inReplyTo":"20180604035637.GA15408@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-04T05:00:34Z","receivedAt":"2018-06-04T05:00:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 03, 2018 at 11:56:37PM -0400, Jeff King wrote:\n\n> So sometimes some_var needs to be freed and sometimes not (and every one\n> of those uses is a potential leak, but it's OK because they're all\n> program-lifetime globals anyway, and people don't _tend_ to set the same\n> option over and over, leaking heap memory). And C being C, we can't\n> convert a pointer-to-pointer-to-const into a pointer-to-pointer without\n> an explicit cast.\n> \n> Doing it \"right\" in C would probably involve two variables:\n> \n> const char *some_var = \"default\";\n> const char *some_var_storage = NULL;\n> \n> int git_config_string_smart(const char **ptr, char **storage,\n> const char *var, const char *value)\n> {\n> ...\n> \tfree(*storage);\n> \t*ptr = *storage = xstrdup(value);\n> \treturn 0;\n> }\n> \n> #define GIT_CONFIG_STRING(name, var, value) \\\n> git_config_string_smart(&(name), &(name##_storage), var, value)\n> \n> Or something like that.\n\nOh, one other option I forgot to mention: we already have an \"intern\"\nhashmap helper. So we could just replace the xstrdup() with strintern()\nand magically the memory isn't \"leaked\".\n\nI think this is a little bit of a hack, though. It catches my:\n\n [core]\n editor = foo\n editor = foo\n\ncase, because we only ever make one copy of the string \"foo\". But if I\ndo:\n\n [core]\n editor = 1\n editor = 2\n ...\n\nthen we get unique strings. And while they're not _technically_ leaked,\nsince the hashmap still knows about them, they might as well be. They're\nstill wasting space through the duration of the program run.\n\n-Peff\n"},{"id":"350614","messageId":"nycvar.QRO.7.76.6.1806210857520.11870@tvgsbejvaqbjf.bet","threadId":"48626","inReplyTo":"20180604045122.GE14451@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-21T07:03:05Z","receivedAt":"2018-06-21T07:03:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 4 Jun 2018, Jeff King wrote:\n\n> On Mon, Jun 04, 2018 at 01:26:57PM +0900, Junio C Hamano wrote:\n> \n> > And at that point, maybe\n> > \n> > \tchar *some_var = xstrdup(\"default\");\n> > \tgit_config_string(&some_var, ...);\n> > \n> > that takes \"char **\" and frees the current storage before assigning to\n> > it may be simpler than the two-variable approach.\n> \n> That _is_ much nicer, but you cannot use xstrdup() as the initializer\n> for a global \"static char *some_var\", which is what the majority of the\n> config variables are. It's this \"static initializer sometimes, run-time\n> heap sometimes\" duality to the variables that makes handling it such a\n> pain.\n\nThis makes me think of Michael's proposal to teach strbuf some sort of\nSTRBUF_INIT_CONST(\"default\") which would set the appropriate len and set\nalloc to 0.\n\nThat way, we could turn those settings into strbufs that only allocate\nmemory when/if needed.\n\nWe would need to be careful to revisit all strbuf_*() functions to make\nsure that they test not only for enough space when growing the string, but\nalso when reducing it (unless reducing to len 0, in which case we can\nreassign strbuf_slopbuf to the buf field).\n\nWe also would need to revisit strbuf_*() to understand alloc < len to mean\nthat we cannot realloc(), but have to malloc() && memcpy().\n\nAs a bonus, the pattern used for\n\n[section]\n\tvar = 1\n\tvar = 2\n\tvar = 3\n\nwould change from three times strdup() and two times free() to one time\nmalloc().\n\nCiao,\nDscho\n"},{"id":"350625","messageId":"20180621114632.GA15293@sigill.intra.peff.net","threadId":"48626","inReplyTo":"nycvar.QRO.7.76.6.1806210857520.11870@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] sequencer.c: plug mem leak in git_sequencer_config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-21T11:46:33Z","receivedAt":"2018-06-21T11:46:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 21, 2018 at 09:03:05AM +0200, Johannes Schindelin wrote:\n\n> > > And at that point, maybe\n> > > \n> > > \tchar *some_var = xstrdup(\"default\");\n> > > \tgit_config_string(&some_var, ...);\n> > > \n> > > that takes \"char **\" and frees the current storage before assigning to\n> > > it may be simpler than the two-variable approach.\n> > \n> > That _is_ much nicer, but you cannot use xstrdup() as the initializer\n> > for a global \"static char *some_var\", which is what the majority of the\n> > config variables are. It's this \"static initializer sometimes, run-time\n> > heap sometimes\" duality to the variables that makes handling it such a\n> > pain.\n> \n> This makes me think of Michael's proposal to teach strbuf some sort of\n> STRBUF_INIT_CONST(\"default\") which would set the appropriate len and set\n> alloc to 0.\n> \n> That way, we could turn those settings into strbufs that only allocate\n> memory when/if needed.\n\nYes! I should have thought about that as soon as I started saying \"you\nneed two variables...\". That is a good indication that you need a\nstruct. ;)\n\nI think the result would be quite readable and pleasant to work with.\n\nI tried to dig up previous conversations about this to see if there were\nany patches shown, but I couldn't find any (mostly I found the\nconversation about using stack buffers in strbufs, which is not quite\nthe same thing, since we _do_ want to write in those).\n\n-Peff\n"}]}