{"thread":{"id":"45713","subject":"[PATCH] server-info: avoid calling fclose(3) twice in update_info_file()","startedAt":"2017-04-16T16:56:17Z","lastAt":"2017-04-17T01:37:50Z","messageCount":2,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316953","messageId":"137d732f-7a23-8a8a-9f2b-7f16bb361c5a@web.de","threadId":"45713","inReplyTo":null,"subject":"[PATCH] server-info: avoid calling fclose(3) twice in update_info_file()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-16T16:55:58Z","receivedAt":"2017-04-16T16:56:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If an error occurs when or after closing the stream we call fclose(3)\nagain in the error handler.  The second call can exhibit undefined\nbehavior, so make sure to call fclose(3) at most once.  Also avoid\ncalling close(2) after fd has been successfully associated with the\nstream, as fclose(3) has become responsible for doing that beyond\nthis point.\n\nFound with Cppcheck.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n server-info.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 7bc4e75d22..f6c1a3dfb0 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -14,19 +14,21 @@ static int update_info_file(char *path, int (*generate)(FILE *))\n \tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n \tint ret = -1;\n \tint fd = -1;\n-\tFILE *fp = NULL;\n+\tFILE *fp = NULL, *to_close;\n \n \tsafe_create_leading_directories(path);\n \tfd = git_mkstemp_mode(tmp, 0666);\n \tif (fd < 0)\n \t\tgoto out;\n-\tfp = fdopen(fd, \"w\");\n+\tto_close = fp = fdopen(fd, \"w\");\n \tif (!fp)\n \t\tgoto out;\n+\tfd = -1;\n \tret = generate(fp);\n \tif (ret)\n \t\tgoto out;\n-\tif (fclose(fp))\n+\tfp = NULL;\n+\tif (fclose(to_close))\n \t\tgoto out;\n \tif (adjust_shared_perm(tmp) < 0)\n \t\tgoto out;\n-- \n2.12.2\n\n"},{"id":"316976","messageId":"20170417013733.ga362sz67n7vbt32@sigill.intra.peff.net","threadId":"45713","inReplyTo":"137d732f-7a23-8a8a-9f2b-7f16bb361c5a@web.de","subject":"Re: [PATCH] server-info: avoid calling fclose(3) twice in update_info_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-17T01:37:33Z","receivedAt":"2017-04-17T01:37:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 16, 2017 at 06:55:58PM +0200, René Scharfe wrote:\n\n> If an error occurs when or after closing the stream we call fclose(3)\n> again in the error handler.  The second call can exhibit undefined\n> behavior, so make sure to call fclose(3) at most once.\n\nYikes. Good catch.\n\n> Also avoid\n> calling close(2) after fd has been successfully associated with the\n> stream, as fclose(3) has become responsible for doing that beyond\n> this point.\n\nYeah, the \"else if\" in the error-code was supposed to handle that, but\nit comes back when you fix the double-fclose by setting fp to NULL.\n\nSo the whole thing looks good to me. Thanks for cleaning up my mess.\n\n-Peff\n"}]}