{"thread":{"id":"46874","subject":"[PATCH] builtin/: add UNLEAKs","startedAt":"2017-10-01T17:42:27Z","lastAt":"2017-10-03T06:20:55Z","messageCount":5,"participants":["Martin Ågren","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"329313","messageId":"20171001174208.1693753-1-martin.agren@gmail.com","threadId":"46874","inReplyTo":null,"subject":"[PATCH] builtin/: add UNLEAKs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T17:42:08Z","receivedAt":"2017-10-01T17:42:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Add some UNLEAKs where we are about to return from `cmd_*`. UNLEAK the\nvariables in the same order as we've declared them. While addressing\n`msg` in builtin/tag.c, convert the existing `strbuf_release()` calls as\nwell.\n\nSigned-off-by: Martin Ågren \n---\n builtin/checkout.c | 1 +\n builtin/diff-index.c | 1 +\n builtin/diff.c | 3 +++\n builtin/name-rev.c | 1 +\n builtin/tag.c | 9 +++++----\n 5 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3345a0d16..2daa4412a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1298,6 +1298,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \t}\n \n+\tUNLEAK(opts);\n \tif (opts.patch_mode || opts.pathspec.nr)\n \t\treturn checkout_paths(&opts, new.name);\n \telse\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 9d772f8f2..522f4fdff 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -56,5 +56,6 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\treturn -1;\n \t}\n \tresult = run_diff_index(&rev, cached);\n+\tUNLEAK(rev);\n \treturn diff_result_code(&rev.diffopt, result);\n }\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 7e3ebcea3..f5bbd4d75 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -464,5 +464,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tresult = diff_result_code(&rev.diffopt, result);\n \tif (1 < rev.diffopt.skip_stat_unmatch)\n \t\trefresh_index_quietly();\n+\tUNLEAK(rev);\n+\tUNLEAK(ent);\n+\tUNLEAK(blob);\n \treturn result;\n }\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 598da6c8b..9e088ebd1 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -494,5 +494,6 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)\n \t\t\t\t always, allow_undefined, data.name_only);\n \t}\n \n+\tUNLEAK(revs);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex c62779418..34efba579 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -552,9 +552,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (force && !is_null_oid(&prev) && oidcmp(&prev, &object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev.hash, DEFAULT_ABBREV));\n \n-\tstrbuf_release(&err);\n-\tstrbuf_release(&buf);\n-\tstrbuf_release(&ref);\n-\tstrbuf_release(&reflog_msg);\n+\tUNLEAK(buf);\n+\tUNLEAK(ref);\n+\tUNLEAK(reflog_msg);\n+\tUNLEAK(msg);\n+\tUNLEAK(err);\n \treturn 0;\n }\n-- \n2.14.2.666.gea220ee40\n\n"},{"id":"329374","messageId":"xmqq1smmqjk1.fsf@gitster.mtv.corp.google.com","threadId":"46874","inReplyTo":"20171001174208.1693753-1-martin.agren@gmail.com","subject":"Re: [PATCH] builtin/: add UNLEAKs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-02T04:07:26Z","receivedAt":"2017-10-02T04:07:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> ... While addressing `msg` in builtin/tag.c, convert the existing\n> `strbuf_release()` calls as well.\n\nThat part of this patch made me raise eyebrows a bit but only\nslightly. We are about to leave the function to exit anyway, and\nall the other that got new UNLEAK(var) at the end are doing the same\nthing.\n\nSo overall I think this patch makes sense. Thanks.\n"},{"id":"329414","messageId":"20171002062528.naa5hrqoao5sifs3@sigill.intra.peff.net","threadId":"46874","inReplyTo":"20171001174208.1693753-1-martin.agren@gmail.com","subject":"Re: [PATCH] builtin/: add UNLEAKs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T06:25:28Z","receivedAt":"2017-10-02T06:25:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 01, 2017 at 07:42:08PM +0200, Martin Ågren wrote:\n\n> Add some UNLEAKs where we are about to return from `cmd_*`. UNLEAK the\n> variables in the same order as we've declared them. While addressing\n> `msg` in builtin/tag.c, convert the existing `strbuf_release()` calls as\n> well.\n\nIt might have raised Junio's eyebrows less to say something like:\n\n ...convert the existing strbuf_release() calls as well (they're not\n wrong, but they also accomplish nothing and create an inconsistency\n with the UNLEAKed variables).\n\nThat aside, the patch looks good.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 3345a0d16..2daa4412a 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1298,6 +1298,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n> \t\tstrbuf_release(&buf);\n> \t}\n> \n> +\tUNLEAK(opts);\n> \tif (opts.patch_mode || opts.pathspec.nr)\n> \t\treturn checkout_paths(&opts, new.name);\n> \telse\n\nSeeing hunks like this makes me happy with the UNLEAK() solution. It\nwould have been a real pain to do this via actual freeing.\n\n-Peff\n"},{"id":"329432","messageId":"CAN0heSoson-KkbgVnRPdTmRL2MMDU1+L6XdOHhi6xcZS5rwQcQ@mail.gmail.com","threadId":"46874","inReplyTo":"20171002062528.naa5hrqoao5sifs3@sigill.intra.peff.net","subject":"Re: [PATCH] builtin/: add UNLEAKs","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-02T10:20:56Z","receivedAt":"2017-10-02T10:21:02Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 October 2017 at 08:25, Jeff King wrote:\n> On Sun, Oct 01, 2017 at 07:42:08PM +0200, Martin Ågren wrote:\n>\n>> Add some UNLEAKs where we are about to return from `cmd_*`. UNLEAK the\n>> variables in the same order as we've declared them. While addressing\n>> `msg` in builtin/tag.c, convert the existing `strbuf_release()` calls as\n>> well.\n>\n> It might have raised Junio's eyebrows less to say something like:\n>\n> ...convert the existing strbuf_release() calls as well (they're not\n> wrong, but they also accomplish nothing and create an inconsistency\n> with the UNLEAKed variables).\n\nMost likely yes. Sorry about that. I have yet to be critiqued for\nwriting *too much* in a commit message. That should tell me something.\n\n> Seeing hunks like this makes me happy with the UNLEAK() solution. It\n> would have been a real pain to do this via actual freeing.\n\nYes, I was very happy to have it handy. :-)\n\nMartin\n"},{"id":"329529","messageId":"xmqq3770kb0k.fsf@gitster.mtv.corp.google.com","threadId":"46874","inReplyTo":"CAN0heSoson-KkbgVnRPdTmRL2MMDU1+L6XdOHhi6xcZS5rwQcQ@mail.gmail.com","subject":"Re: [PATCH] builtin/: add UNLEAKs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-03T06:20:43Z","receivedAt":"2017-10-03T06:20:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n>> Seeing hunks like this makes me happy with the UNLEAK() solution. It\n>> would have been a real pain to do this via actual freeing.\n>\n> Yes, I was very happy to have it handy. :-)\n\nOK, let's merge this to 'next', then.\n"}]}