{"thread":{"id":"27817","subject":"[PATCH] Fix config_file file leak.","startedAt":"2011-07-14T18:19:48Z","lastAt":"2011-07-20T13:12:14Z","messageCount":4,"participants":["Chris Wilson","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"171351","messageId":"20110714181948.GA23288@localhost","threadId":"27817","inReplyTo":null,"subject":"[PATCH] Fix config_file file leak.","fromName":"Chris Wilson","fromEmail":"cwilson@vigilantsw.com","sentAt":"2011-07-14T18:19:48Z","receivedAt":"2011-07-14T18:19:48Z","isPatch":true,"sender":{"key":"cwilson@vigilantsw.com","avatar":null},"body":"Hi,\n\nWe are using Sentry (a C/C++ static analysis tool) to analyze\ngit on a nightly basis. Sentry found that a file leak\nwas recently introduced in the commit 924aaf3.\n\nI'm hoping the attached patch correctly fixes up this leak.\n\nThanks,\nChris\n\n-- \nChris Wilson\nhttp://vigilantsw.com/\nVigilant Software, A C/C++ Static Analysis Company\n\n---\n config.c |    5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 1fc063b..bf61f09 100644\n--- a/config.c\n+++ b/config.c\n@@ -1434,7 +1434,7 @@ int git_config_rename_section(const char *old_name, const char *new\n        struct lock_file *lock = xcalloc(sizeof(struct lock_file), 1);\n        int out_fd;\n        char buf[1024];\n-       FILE *config_file;\n+       FILE *config_file = 0;\n\n        if (config_exclusive_filename)\n                config_filename = xstrdup(config_exclusive_filename);\n@@ -1498,12 +1498,13 @@ int git_config_rename_section(const char *old_name, const char *n\n                        goto out;\n                }\n        }\n-       fclose(config_file);\n  unlock_and_out:\n        if (commit_lock_file(lock) < 0)\n                ret = error(\"could not commit config file %s\", config_filename);\n  out: \n        free(config_filename);\n+        if (config_file)\n+               fclose(config_file);\n        return ret;\n }\n\n-- \n1.7.0.4\n"},{"id":"171353","messageId":"20110714182919.GB23288@localhost","threadId":"27817","inReplyTo":"20110714181948.GA23288@localhost","subject":"Re: [PATCH] Fix config_file file leak.","fromName":"Chris Wilson","fromEmail":"cwilson@vigilantsw.com","sentAt":"2011-07-14T18:29:19Z","receivedAt":"2011-07-14T18:29:19Z","isPatch":true,"sender":{"key":"cwilson@vigilantsw.com","avatar":null},"body":"On Thu, Jul 14, 2011 at 02:19:48PM -0400, Chris Wilson wrote:\n> Hi,\n> \n> We are using Sentry (a C/C++ static analysis tool) to analyze\n> git on a nightly basis. Sentry found that a file leak\n> was recently introduced in the commit 924aaf3.\n> \n> I'm hoping the attached patch correctly fixes up this leak.\n\nOops, sorry if I was unclear. This happens in config.c here,\nhttp://git.kernel.org/?p=git/git.git;a=blob;f=config.c;h=1fc063b2562101687b9215e5b697a91fcffdd5bb;hb=924aaf3ef764a5e8e976f68e024ecacf54ff6306\n  \nThis happens once the file is opened successfully here,\n  1449         if (!(config_file = fopen(config_filename, \"rb\"))) {\nand then you enter the while loop here,\n  1454         while (fgets(buf, sizeof(buf), config_file)) {\nand then you take any 'goto out;' in the while loop, which doesn't\nclose the file handle.\n\n1505  out:\n1506         free(config_filename);\n1507         return ret;\n1508 }\n\n\nChris\n"},{"id":"171685","messageId":"4E25BEDB.6040002@ramsay1.demon.co.uk","threadId":"27817","inReplyTo":"20110714181948.GA23288@localhost","subject":"Re: [PATCH] Fix config_file file leak.","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-07-19T17:28:59Z","receivedAt":"2011-07-19T17:28:59Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Chris Wilson wrote:\n> Hi,\n> \n> We are using Sentry (a C/C++ static analysis tool) to analyze\n> git on a nightly basis. Sentry found that a file leak\n> was recently introduced in the commit 924aaf3.\n\nHmmm ..., commit 924aaf3 did *not* introduce a file handle leak.\nIt would seem that the change in scope of the file handle made it\neasier for Sentry to see the *existing* (potential) file handle leak.\n\nno?\n\nOther than that, ...\n\nATB,\nRamsay Jones\n"},{"id":"171734","messageId":"20110720131214.GC25822@localhost","threadId":"27817","inReplyTo":"4E25BEDB.6040002@ramsay1.demon.co.uk","subject":"Re: [PATCH] Fix config_file file leak.","fromName":"Chris Wilson","fromEmail":"cwilson@vigilantsw.com","sentAt":"2011-07-20T13:12:14Z","receivedAt":"2011-07-20T13:12:14Z","isPatch":true,"sender":{"key":"cwilson@vigilantsw.com","avatar":null},"body":"On Tue, Jul 19, 2011 at 06:28:59PM +0100, Ramsay Jones wrote:\n> Chris Wilson wrote:\n> > Hi,\n> > \n> > We are using Sentry (a C/C++ static analysis tool) to analyze\n> > git on a nightly basis. Sentry found that a file leak\n> > was recently introduced in the commit 924aaf3.\n> \n> Hmmm ..., commit 924aaf3 did *not* introduce a file handle leak.\n> It would seem that the change in scope of the file handle made it\n> easier for Sentry to see the *existing* (potential) file handle leak.\n> \n> no?\n\nAh yes, that does seem like that case. Thanks for pointing that out.\n\n> Other than that, ...\n\nLooks like this file handle will leak whenever a write fails.\n\nChris\n"}]}