{"thread":{"id":"40621","subject":"[PATCH 1/2 jk/war-on-sprintf] read_branches_file: plug a FILE* leak","startedAt":"2015-10-23T06:02:51Z","lastAt":"2015-10-23T17:18:25Z","messageCount":5,"participants":["Johannes Sixt","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"272129","messageId":"2c89c60e470def8f85941933c9fafe4db314628a.1445579874.git.j6t@kdbg.org","threadId":"40621","inReplyTo":null,"subject":"[PATCH 1/2 jk/war-on-sprintf] read_branches_file: plug a FILE* leak","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-10-23T06:02:51Z","receivedAt":"2015-10-23T06:02:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The earlier rewrite f28e3ab2 (read_branches_file: simplify string handling)\nof read_branches_file() lost an fclose() call. Put it back.\n\nAs on Windows files that are open cannot be removed, the leak manifests in\na failure of 'git remote rename origin origin' when the remote's URL is\nspecified in .git/branches/origin, because by the time that the command\nattempts to remove this file, it is still open.\n\nSigned-off-by: Johannes Sixt \n---\n remote.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/remote.c b/remote.c\nindex 1101f82..fb16153 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -282,6 +282,7 @@ static void read_branches_file(struct remote *remote)\n \t\treturn;\n \n \tstrbuf_getline(&buf, f, '\\n');\n+\tfclose(f);\n \tstrbuf_trim(&buf);\n \tif (!buf.len) {\n \t\tstrbuf_release(&buf);\n-- \n2.3.2.245.gb5bf9d3\n"},{"id":"272130","messageId":"28a610a30ab9d4f742be2f3225db341ea40c8a51.1445579874.git.j6t@kdbg.org","threadId":"40621","inReplyTo":"2c89c60e470def8f85941933c9fafe4db314628a.1445579874.git.j6t@kdbg.org","subject":"[PATCH 2/2 jk/war-on-sprintf] compat/mingw.c: remove printf format warning","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-10-23T06:02:52Z","receivedAt":"2015-10-23T06:02:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"5096d490 (convert trivial sprintf / strcpy calls to xsnprintf) converted\ntwo sprintf calls. Now GCC warns that \"format '%u' expects argument of\ntype 'unsigned int', but argument 4 has type 'long unsigned int'\".\nInstead of changing the format string, use a variable of type unsigned\nin place of the typedef-ed type DWORD, which hides that it is actually an\nunsigned long.\n\nThere is no correctness issue with the old code because unsigned long and\nunsigned are always of the same size on Windows, even in 64-bit builds.\n\nSigned-off-by: Johannes Sixt \n---\nI do not know why there are no warnings with the old code. Apparently, the\nsystem provided sprintf declaration does not have format-printf\nannotation.\n\n compat/mingw.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex a168800..90bdb1e 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -2131,7 +2131,7 @@ void mingw_startup()\n \n int uname(struct utsname *buf)\n {\n-\tDWORD v = GetVersion();\n+\tunsigned v = (unsigned)GetVersion();\n \tmemset(buf, 0, sizeof(*buf));\n \txsnprintf(buf->sysname, sizeof(buf->sysname), \"Windows\");\n \txsnprintf(buf->release, sizeof(buf->release),\n-- \n2.3.2.245.gb5bf9d3\n"},{"id":"272137","messageId":"20151023111903.GA14996@sigill.intra.peff.net","threadId":"40621","inReplyTo":"2c89c60e470def8f85941933c9fafe4db314628a.1445579874.git.j6t@kdbg.org","subject":"Re: [PATCH 1/2 jk/war-on-sprintf] read_branches_file: plug a FILE* leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-23T11:19:03Z","receivedAt":"2015-10-23T11:19:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 23, 2015 at 08:02:51AM +0200, Johannes Sixt wrote:\n\n> The earlier rewrite f28e3ab2 (read_branches_file: simplify string handling)\n> of read_branches_file() lost an fclose() call. Put it back.\n> \n> As on Windows files that are open cannot be removed, the leak manifests in\n> a failure of 'git remote rename origin origin' when the remote's URL is\n> specified in .git/branches/origin, because by the time that the command\n> attempts to remove this file, it is still open.\n\nThanks for catching. This was due to my last-minute rewrite of\nstrbuf_read_file into fopen/strbuf_getline. Your patch looks good to me.\n\n-Peff\n"},{"id":"272138","messageId":"20151023112019.GB14996@sigill.intra.peff.net","threadId":"40621","inReplyTo":"28a610a30ab9d4f742be2f3225db341ea40c8a51.1445579874.git.j6t@kdbg.org","subject":"Re: [PATCH 2/2 jk/war-on-sprintf] compat/mingw.c: remove printf format warning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-23T11:20:19Z","receivedAt":"2015-10-23T11:20:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 23, 2015 at 08:02:52AM +0200, Johannes Sixt wrote:\n\n> 5096d490 (convert trivial sprintf / strcpy calls to xsnprintf) converted\n> two sprintf calls. Now GCC warns that \"format '%u' expects argument of\n> type 'unsigned int', but argument 4 has type 'long unsigned int'\".\n> Instead of changing the format string, use a variable of type unsigned\n> in place of the typedef-ed type DWORD, which hides that it is actually an\n> unsigned long.\n> \n> There is no correctness issue with the old code because unsigned long and\n> unsigned are always of the same size on Windows, even in 64-bit builds.\n> \n> Signed-off-by: Johannes Sixt \n> ---\n> I do not know why there are no warnings with the old code. Apparently, the\n> system provided sprintf declaration does not have format-printf\n> annotation.\n\nMakes sense, and the patch looks obviously correct.\n\n-Peff\n"},{"id":"272147","messageId":"xmqqfv11pkpq.fsf@gitster.mtv.corp.google.com","threadId":"40621","inReplyTo":"2c89c60e470def8f85941933c9fafe4db314628a.1445579874.git.j6t@kdbg.org","subject":"Re: [PATCH 1/2 jk/war-on-sprintf] read_branches_file: plug a FILE* leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-23T17:18:25Z","receivedAt":"2015-10-23T17:18:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> The earlier rewrite f28e3ab2 (read_branches_file: simplify string handling)\n> of read_branches_file() lost an fclose() call. Put it back.\n>\n> As on Windows files that are open cannot be removed, the leak manifests in\n> a failure of 'git remote rename origin origin' when the remote's URL is\n> specified in .git/branches/origin, because by the time that the command\n> attempts to remove this file, it is still open.\n>\n> Signed-off-by: Johannes Sixt \n> ---\n\nThanks (and also for 2/2).\n\n> remote.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/remote.c b/remote.c\n> index 1101f82..fb16153 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -282,6 +282,7 @@ static void read_branches_file(struct remote *remote)\n> \t\treturn;\n> \n> \tstrbuf_getline(&buf, f, '\\n');\n> +\tfclose(f);\n> \tstrbuf_trim(&buf);\n> \tif (!buf.len) {\n> \t\tstrbuf_release(&buf);\n"}]}