{"thread":{"id":"56732","subject":"[PATCH 0/2] some leak fixes on fs/ssh-signing-fix","startedAt":"2021-10-18T17:14:56Z","lastAt":"2021-10-19T21:01:54Z","messageCount":5,"participants":["Jeff King","Fabian Stelzer"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"438990","messageId":"YW2qpuosxr9PNwVa@coredump.intra.peff.net","threadId":"56732","inReplyTo":null,"subject":"[PATCH 0/2] some leak fixes on fs/ssh-signing-fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-18T17:11:37Z","receivedAt":"2021-10-18T17:14:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes two small leaks on top of fs/ssh-signing-fix noticed by\nCoverity. I guess it's too late to squash them in, so I prepared patches\non top.\n\n  [1/2]: gpg-interface: fix leak of \"line\" in parse_ssh_output()\n  [2/2]: gpg-interface: fix leak of strbufs in get_ssh_key_fingerprint()\n\n gpg-interface.c | 14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\n-Peff\n"},{"id":"438991","messageId":"YW2rlCAtRumXzVfX@coredump.intra.peff.net","threadId":"56732","inReplyTo":"YW2qpuosxr9PNwVa@coredump.intra.peff.net","subject":"[PATCH 1/2] gpg-interface: fix leak of \"line\" in parse_ssh_output()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-18T17:15:00Z","receivedAt":"2021-10-18T17:15:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We xmemdupz() this buffer, but never free it. Let's do so. We'll use a\ncleanup label, since there are multiple exits from the function.\n\nNote that it was also declared a \"const char *\". We could switch that to\n\"char *\" to indicate that it's allocated, but that make it awkward to\nuse with skip_prefix(). So instead, we'll introduce an extra non-const\npointer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 433482307c..c60b9cd19d 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -365,6 +365,7 @@ static int verify_gpg_signed_buffer(struct signature_check *sigc,\n static void parse_ssh_output(struct signature_check *sigc)\n {\n \tconst char *line, *principal, *search;\n+\tchar *to_free;\n \tchar *key = NULL;\n \n \t/*\n@@ -383,7 +384,7 @@ static void parse_ssh_output(struct signature_check *sigc)\n \tsigc->result = 'B';\n \tsigc->trust_level = TRUST_NEVER;\n \n-\tline = xmemdupz(sigc->output, strcspn(sigc->output, \"\\n\"));\n+\tline = to_free = xmemdupz(sigc->output, strcspn(sigc->output, \"\\n\"));\n \n \tif (skip_prefix(line, \"Good \\\"git\\\" signature for \", &line)) {\n \t\t/* Valid signature and known principal */\n@@ -403,7 +404,7 @@ static void parse_ssh_output(struct signature_check *sigc)\n \t\tsigc->result = 'G';\n \t\tsigc->trust_level = TRUST_UNDEFINED;\n \t} else {\n-\t\treturn;\n+\t\tgoto cleanup;\n \t}\n \n \tkey = strstr(line, \"key\");\n@@ -417,6 +418,9 @@ static void parse_ssh_output(struct signature_check *sigc)\n \t\t */\n \t\tsigc->result = 'B';\n \t}\n+\n+cleanup:\n+\tfree(to_free);\n }\n \n static int verify_ssh_signed_buffer(struct signature_check *sigc,\n-- \n2.33.1.1223.g80c1dbe6e5\n\n"},{"id":"438992","messageId":"YW2ruTL2XReEeIAh@coredump.intra.peff.net","threadId":"56732","inReplyTo":"YW2qpuosxr9PNwVa@coredump.intra.peff.net","subject":"[PATCH 2/2] gpg-interface: fix leak of strbufs in get_ssh_key_fingerprint()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-18T17:15:37Z","receivedAt":"2021-10-18T17:15:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We read stdout from gpg into a strbuf, then split it into a list of\nstrbufs, pull out one element, and return it. But we don't free either\nthe original stdout buffer, nor the list returned from strbuf_split().\n\nThis patch fixes both. Note that we have to detach the returned string\nfrom its strbuf before calling strbuf_list_free(), as that would\notherwise throw it away.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex c60b9cd19d..800d8caa67 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -711,6 +711,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n \tint ret = -1;\n \tstruct strbuf fingerprint_stdout = STRBUF_INIT;\n \tstruct strbuf **fingerprint;\n+\tchar *fingerprint_ret;\n \n \t/*\n \t * With SSH Signing this can contain a filename or a public key\n@@ -737,7 +738,10 @@ static char *get_ssh_key_fingerprint(const char *signing_key)\n \t\tdie_errno(_(\"failed to get the ssh fingerprint for key '%s'\"),\n \t\t\t  signing_key);\n \n-\treturn strbuf_detach(fingerprint[1], NULL);\n+\tfingerprint_ret = strbuf_detach(fingerprint[1], NULL);\n+\tstrbuf_list_free(fingerprint);\n+\tstrbuf_release(&fingerprint_stdout);\n+\treturn fingerprint_ret;\n }\n \n /* Returns the first public key from an ssh-agent to use for signing */\n-- \n2.33.1.1223.g80c1dbe6e5\n"},{"id":"439018","messageId":"af0cf842-a16c-6cc3-fb61-b61422b24b17@gigacodes.de","threadId":"56732","inReplyTo":"YW2qpuosxr9PNwVa@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] some leak fixes on fs/ssh-signing-fix","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-19T08:16:41Z","receivedAt":"2021-10-19T08:16:49Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 18.10.21 19:11, Jeff King wrote:\n> This fixes two small leaks on top of fs/ssh-signing-fix noticed by\n> Coverity. I guess it's too late to squash them in, so I prepared patches\n> on top.\n> \n>   [1/2]: gpg-interface: fix leak of \"line\" in parse_ssh_output()\n>   [2/2]: gpg-interface: fix leak of strbufs in get_ssh_key_fingerprint()\n> \n>  gpg-interface.c | 14 +++++++++++---\n>  1 file changed, 11 insertions(+), 3 deletions(-)\n> \n> -Peff\n> \n\nThanks.\nBoth of these look good.\n\nIs coverity included in the ci/gh actions? Where would these notices\nshow up?\n\nKind regards,\nFabian\n"},{"id":"439041","messageId":"YW8yP0mb7j15y6Yy@coredump.intra.peff.net","threadId":"56732","inReplyTo":"af0cf842-a16c-6cc3-fb61-b61422b24b17@gigacodes.de","subject":"Re: [PATCH 0/2] some leak fixes on fs/ssh-signing-fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-19T21:01:51Z","receivedAt":"2021-10-19T21:01:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 19, 2021 at 10:16:41AM +0200, Fabian Stelzer wrote:\n\n> Is coverity included in the ci/gh actions? Where would these notices\n> show up?\n\nNot currently. I've been playing with running it in an action, and may\nsubmit something to make it more official. There's some discussion here:\n\n  https://lore.kernel.org/git/YV5dmkkuCqAY2qqG@coredump.intra.peff.net/\n\nThere's also some on-going work to make the test suite run without\nleak-checkers (I didn't dig them up, but you can find recent topics and\ncommits from Ævar). But we've got a ways to go, so you'd likely find a\nbunch of existing leaks if you tried it.\n\nSo no, for now there was nothing obvious you could have seen that would\nhave alerted you.\n\n-Peff\n"}]}