{"thread":{"id":"28619","subject":"Fix another file leak","startedAt":"2011-10-07T01:41:37Z","lastAt":"2011-10-07T07:40:22Z","messageCount":5,"participants":["Chris Wilson","René Scharfe","Tay Ray Chuan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"177128","messageId":"20111007014136.GB10839@localhost","threadId":"28619","inReplyTo":null,"subject":"Fix another file leak","fromName":"Chris Wilson","fromEmail":"cwilson@vigilantsw.com","sentAt":"2011-10-07T01:41:37Z","receivedAt":"2011-10-07T01:41:37Z","isPatch":false,"sender":{"key":"cwilson@vigilantsw.com","avatar":null},"body":"Hi,\n\nVigilant Sentry (our C/C++ static analysis tool) found that\ncommit 6d4bb383, added a file leak to builtin/fetch.c.\n\nstatic int store_updated_refs(...\n{ \n FILE *fp;\n ...\n fp = fopen(filename, \"a\");\n if (!fp)\n return error(_(\"cannot open %s: %s\\n\"), filename, strerror(errno));\n ....\n\n if (check_everything_connected(iterate_ref_map, 0, &rm))\n return error(_(\"%s did not send all necessary objects\\n\"), url);\n\nPlease close the file handle before returning from the function.\n\nThanks,\nChris\n\n-- \nChris Wilson\nhttp://vigilantsw.com/\nVigilant Software, LLC\n"},{"id":"177134","messageId":"4E8E98A7.8010008@lsrfire.ath.cx","threadId":"28619","inReplyTo":"20111007014136.GB10839@localhost","subject":"[PATCH] fetch: plug two leaks on error exit in store_updated_refs","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-10-07T06:13:59Z","receivedAt":"2011-10-07T06:13:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Close FETCH_HEAD and release the string url even if we have to leave the\nfunction store_updated_refs() early.\n\nReported-by: Chris Wilson \nSigned-off-by: Rene Scharfe \n---\n builtin/fetch.c | 8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7a4e41c..79db796 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -379,8 +379,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\turl = xstrdup(\"foreign\");\n \n \trm = ref_map;\n-\tif (check_everything_connected(iterate_ref_map, 0, &rm))\n-\t\treturn error(_(\"%s did not send all necessary objects\\n\"), url);\n+\tif (check_everything_connected(iterate_ref_map, 0, &rm)) {\n+\t\terror(_(\"%s did not send all necessary objects\\n\"), url);\n+\t\tfree(url);\n+\t\tfclose(fp);\n+\t\treturn -1;\n+\t}\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tstruct ref *ref = NULL;\n-- \n1.7.7\n"},{"id":"177136","messageId":"CALUzUxp4Eo7j=kM7YPJbj70-rwuyFK5V1mZZMY7vBwwPYWS6gQ@mail.gmail.com","threadId":"28619","inReplyTo":"4E8E98A7.8010008@lsrfire.ath.cx","subject":"Re: [PATCH] fetch: plug two leaks on error exit in store_updated_refs","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-10-07T06:49:12Z","receivedAt":"2011-10-07T06:49:12Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Fri, Oct 7, 2011 at 2:13 PM, René Scharfe\n wrote:\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 7a4e41c..79db796 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -379,8 +379,12 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n>                url = xstrdup(\"foreign\");\n>\n>        rm = ref_map;\n> -       if (check_everything_connected(iterate_ref_map, 0, &rm))\n> -               return error(_(\"%s did not send all necessary objects\\n\"), url);\n> +       if (check_everything_connected(iterate_ref_map, 0, &rm)) {\n> +               error(_(\"%s did not send all necessary objects\\n\"), url);\n> +               free(url);\n> +               fclose(fp);\n> +               return -1;\n> +       }\n>\n>        for (rm = ref_map; rm; rm = rm->next) {\n>                struct ref *ref = NULL;\n> --\n> 1.7.7\n\nHow about reusing the function's cleanup calls, like this?\n\n-- >8 --\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex fc254b6..56267c4 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -423,8 +423,10 @@ static int store_updated_refs(const char\n*raw_url, const char *remote_name,\n \telse\n \t\turl = xstrdup(\"foreign\");\n\n-\tif (check_everything_connected(ref_map, 0))\n-\t\treturn error(_(\"%s did not send all necessary objects\\n\"), url);\n+\tif (check_everything_connected(ref_map, 0)) {\n+\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n+\t\tgoto abort;\n+\t}\n\n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tstruct ref *ref = NULL;\n@@ -506,12 +508,15 @@ static int store_updated_refs(const char\n*raw_url, const char *remote_name,\n \t\t\t\tfprintf(stderr, \" %s\\n\", note);\n \t\t}\n \t}\n-\tfree(url);\n-\tfclose(fp);\n+\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n \t\t \"branches\"), remote_name);\n+\n+abort:\n+\tfree(url);\n+\tfclose(fp);\n \treturn rc;\n }\n\n--\n\n-- \nCheers,\nRay Chuan\n"},{"id":"177138","messageId":"4E8EA33E.5020009@lsrfire.ath.cx","threadId":"28619","inReplyTo":"CALUzUxp4Eo7j=kM7YPJbj70-rwuyFK5V1mZZMY7vBwwPYWS6gQ@mail.gmail.com","subject":"Re: [PATCH] fetch: plug two leaks on error exit in store_updated_refs","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-10-07T06:59:10Z","receivedAt":"2011-10-07T06:59:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.10.2011 08:49, schrieb Tay Ray Chuan:\n> How about reusing the function's cleanup calls, like this?\n\nYes, that's better.\n\n> -- >8 --\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index fc254b6..56267c4 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -423,8 +423,10 @@ static int store_updated_refs(const char\n> *raw_url, const char *remote_name,\n> \telse\n> \t\turl = xstrdup(\"foreign\");\n> \n> -\tif (check_everything_connected(ref_map, 0))\n> -\t\treturn error(_(\"%s did not send all necessary objects\\n\"), url);\n> +\tif (check_everything_connected(ref_map, 0)) {\n> +\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n> +\t\tgoto abort;\n> +\t}\n> \n> \tfor (rm = ref_map; rm; rm = rm->next) {\n> \t\tstruct ref *ref = NULL;\n> @@ -506,12 +508,15 @@ static int store_updated_refs(const char\n> *raw_url, const char *remote_name,\n> \t\t\t\tfprintf(stderr, \" %s\\n\", note);\n> \t\t}\n> \t}\n> -\tfree(url);\n> -\tfclose(fp);\n> +\n> \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n> \t\terror(_(\"some local refs could not be updated; try running\\n\"\n> \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n> \t\t \"branches\"), remote_name);\n> +\n> +abort:\n> +\tfree(url);\n> +\tfclose(fp);\n> \treturn rc;\n> }\n> \n\nMicro-nit: If you start the label with a space (\"+ abort:\") then the\ncode continues to play nice with git grep -W.\n\nRené\n"},{"id":"177140","messageId":"1317973222-7340-1-git-send-email-rctay89@gmail.com","threadId":"28619","inReplyTo":"4E8EA33E.5020009@lsrfire.ath.cx","subject":"[PATCH v2] fetch: plug two leaks on error exit in store_updated_refs","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2011-10-07T07:40:22Z","receivedAt":"2011-10-07T07:40:22Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Close FETCH_HEAD and release the string url even if we have to leave the\nfunction store_updated_refs() early.\n\nReported-by: Chris Wilson \nHelped-by: Rene Scharfe \nSigned-off-by: Tay Ray Chuan \n---\n builtin/fetch.c | 13 +++++++++----\n 1 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex fc254b6..9b7ce10 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -423,8 +423,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \telse\n \t\turl = xstrdup(\"foreign\");\n \n-\tif (check_everything_connected(ref_map, 0))\n-\t\treturn error(_(\"%s did not send all necessary objects\\n\"), url);\n+\tif (check_everything_connected(ref_map, 0)) {\n+\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n+\t\tgoto abort;\n+\t}\n \n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tstruct ref *ref = NULL;\n@@ -506,12 +508,15 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tfprintf(stderr, \" %s\\n\", note);\n \t\t}\n \t}\n-\tfree(url);\n-\tfclose(fp);\n+\n \tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n \t\t \" 'git remote prune %s' to remove any old, conflicting \"\n \t\t \"branches\"), remote_name);\n+\n+ abort:\n+\tfree(url);\n+\tfclose(fp);\n \treturn rc;\n }\n \n-- \n1.7.7.584.g16d0ea\n"}]}