{"thread":{"id":"38028","subject":"GIT: [PATCH] exec_cmd: system_path memory leak fix","startedAt":"2014-11-23T13:56:20Z","lastAt":"2014-11-28T11:13:24Z","messageCount":33,"participants":["0xAX","Junio C Hamano","Alex Kuleshov","Jeff King","Eric Sunshine","Alexander Kuleshov","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252388","messageId":"1416750981-24446-1-git-send-email-kuleshovmail@gmail.com","threadId":"38028","inReplyTo":null,"subject":"GIT: [PATCH] exec_cmd: system_path memory leak fix","fromName":"0xAX","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-23T13:56:20Z","receivedAt":"2014-11-23T13:56:20Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello All,\nI found memory leak at exec_cmd.c in system_path function with valgrind.\nAfter this patch valgrind doesn't show any memory leaks, but I'm not sure\nthat it is the best way to solve this problem. There are a couple of places\nthat uses system_path, if this way will be good, i'll make another patches\nto fix this leak.\n\nWaiting for feedback.\n\nSigned-off-by: Alex Kuleshov \n"},{"id":"252389","messageId":"1416750981-24446-2-git-send-email-kuleshovmail@gmail.com","threadId":"38028","inReplyTo":"1416750981-24446-1-git-send-email-kuleshovmail@gmail.com","subject":"[PATCH] exec_cmd: system_path memory leak fix","fromName":"0xAX","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-23T13:56:21Z","receivedAt":"2014-11-23T13:56:21Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Signed-off-by: 0xAX \n---\n exec_cmd.c | 25 ++++++++++++++++---------\n exec_cmd.h | 4 ++--\n git.c | 12 +++++++++---\n 3 files changed, 27 insertions(+), 14 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 698e752..08f8f80 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -6,7 +6,7 @@\n static const char *argv_exec_path;\n static const char *argv0_path;\n \n-const char *system_path(const char *path)\n+char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n \tstatic const char *prefix;\n@@ -14,9 +14,10 @@ const char *system_path(const char *path)\n \tstatic const char *prefix = PREFIX;\n #endif\n \tstruct strbuf d = STRBUF_INIT;\n+\tchar *new_path = NULL;\n \n \tif (is_absolute_path(path))\n-\t\treturn path;\n+\t\treturn strdup(path);\n \n #ifdef RUNTIME_PREFIX\n \tassert(argv0_path);\n@@ -32,10 +33,13 @@ const char *system_path(const char *path)\n \t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n \t}\n #endif\n-\n \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\tnew_path = malloc((strlen(prefix) + strlen(path)) + 2);\n+\tsprintf(new_path, \"%s/%s\", prefix, path);\n+\n+\tstrbuf_release(&d);\n+\n+\treturn new_path;\n }\n \n const char *git_extract_argv0_path(const char *argv0)\n@@ -68,16 +72,16 @@ void git_set_argv_exec_path(const char *exec_path)\n \n \n /* Returns the highest-priority, location to look for git programs. */\n-const char *git_exec_path(void)\n+char *git_exec_path(void)\n {\n \tconst char *env;\n \n \tif (argv_exec_path)\n-\t\treturn argv_exec_path;\n+\t\treturn strdup(argv_exec_path);\n \n \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n \tif (env && *env) {\n-\t\treturn env;\n+\t\treturn strdup(env);\n \t}\n \n \treturn system_path(GIT_EXEC_PATH);\n@@ -96,7 +100,9 @@ void setup_path(void)\n \tconst char *old_path = getenv(\"PATH\");\n \tstruct strbuf new_path = STRBUF_INIT;\n \n-\tadd_path(&new_path, git_exec_path());\n+\tchar *exec_path = git_exec_path();\n+\n+\tadd_path(&new_path, exec_path);\n \tadd_path(&new_path, argv0_path);\n \n \tif (old_path)\n@@ -107,6 +113,7 @@ void setup_path(void)\n \tsetenv(\"PATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n+\tfree(exec_path);\n }\n \n const char **prepare_git_cmd(const char **argv)\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex e4c9702..03c8599 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -3,12 +3,12 @@\n \n extern void git_set_argv_exec_path(const char *exec_path);\n extern const char *git_extract_argv0_path(const char *path);\n-extern const char *git_exec_path(void);\n+extern char *git_exec_path(void);\n extern void setup_path(void);\n extern const char **prepare_git_cmd(const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n LAST_ARG_MUST_BE_NULL\n extern int execl_git_cmd(const char *cmd, ...);\n-extern const char *system_path(const char *path);\n+extern char *system_path(const char *path);\n \n #endif /* GIT_EXEC_CMD_H */\ndiff --git a/git.c b/git.c\nindex 82d7a1c..499dc2a 100644\n--- a/git.c\n+++ b/git.c\n@@ -99,13 +99,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\texit(0);\n \t\t\t}\n \t\t} else if (!strcmp(cmd, \"--html-path\")) {\n-\t\t\tputs(system_path(GIT_HTML_PATH));\n+\t\t\tchar *git_html_path = system_path(GIT_HTML_PATH);\n+\t\t\tputs(git_html_path);\n+\t\t\tfree(git_html_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--man-path\")) {\n-\t\t\tputs(system_path(GIT_MAN_PATH));\n+\t\t\tchar *git_man_path = system_path(GIT_MAN_PATH);\n+\t\t\tputs(git_man_path);\n+\t\t\tfree(git_man_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--info-path\")) {\n-\t\t\tputs(system_path(GIT_INFO_PATH));\n+\t\t\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\t\t\tputs(git_info_path);\n+\t\t\tfree(git_info_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n \t\t\tuse_pager = 1;\n-- \n2.2.0.rc3.191.gfdea3f0\n"},{"id":"252390","messageId":"87h9xq108c.fsf@gmail.com","threadId":"38028","inReplyTo":"1416750981-24446-2-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"0xAX","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-23T14:01:39Z","receivedAt":"2014-11-23T14:01:39Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nFor example some testing with valgrind:\n\n1. pu (952f0aaadff4afca1497450911587a973c8cca02)\n\nvalgrind git help\n==27034== Memcheck, a memory error detector\n==27034== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.\n==27034== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info\n==27034== Command: git help\n==27034==\n==27034== HERE GIT HELP OUTPUT\n==27034==\n==27034== HEAP SUMMARY:\n==27034== in use at exit: 65 bytes in 1 blocks\n==27034== total heap usage: 71 allocs, 70 frees, 11,928 bytes allocated\n==27034==\n==27034== LEAK SUMMARY:\n==27034== definitely lost: 65 bytes in 1 blocks\n==27034== indirectly lost: 0 bytes in 0 blocks\n==27034== possibly lost: 0 bytes in 0 blocks\n==27034== still reachable: 0 bytes in 0 blocks\n==27034== suppressed: 0 bytes in 0 blocks\n==27034== Rerun with --leak-check=full to see details of leaked memory\n==27034==\n==27034== For counts of detected and suppressed errors, rerun with: -v\n==27034== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)\n\n2. With patch:\n\nvalgrind git help\n==28945== Memcheck, a memory error detector\n==28945== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.\n==28945== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info\n==28945== Command: git help\n==28945==\n==28945== HERE git help OUTPUT\n==28945==\n==28945== HEAP SUMMARY:\n==28945== in use at exit: 0 bytes in 0 blocks\n==28945== total heap usage: 72 allocs, 72 frees, 11,956 bytes allocated\n==28945==\n==28945== All heap blocks were freed -- no leaks are possible\n==28945==\n==28945== For counts of detected and suppressed errors, rerun with: -v\n==28945== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)\n\nOf course it is very small leak, but i just started to deep in git\nsource code. Hope i will be useful for community.\n\nThank you.\n\n0xAX @ 2014-11-23 19:56 ALMT:\n\n> Signed-off-by: 0xAX \n> ---\n> exec_cmd.c | 25 ++++++++++++++++---------\n> exec_cmd.h | 4 ++--\n> git.c | 12 +++++++++---\n> 3 files changed, 27 insertions(+), 14 deletions(-)\n>\n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index 698e752..08f8f80 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -6,7 +6,7 @@\n> static const char *argv_exec_path;\n> static const char *argv0_path;\n>\n> -const char *system_path(const char *path)\n> +char *system_path(const char *path)\n> {\n> #ifdef RUNTIME_PREFIX\n> \tstatic const char *prefix;\n> @@ -14,9 +14,10 @@ const char *system_path(const char *path)\n> \tstatic const char *prefix = PREFIX;\n> #endif\n> \tstruct strbuf d = STRBUF_INIT;\n> +\tchar *new_path = NULL;\n>\n> \tif (is_absolute_path(path))\n> -\t\treturn path;\n> +\t\treturn strdup(path);\n>\n> #ifdef RUNTIME_PREFIX\n> \tassert(argv0_path);\n> @@ -32,10 +33,13 @@ const char *system_path(const char *path)\n> \t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n> \t}\n> #endif\n> -\n> \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n> -\tpath = strbuf_detach(&d, NULL);\n> -\treturn path;\n> +\tnew_path = malloc((strlen(prefix) + strlen(path)) + 2);\n> +\tsprintf(new_path, \"%s/%s\", prefix, path);\n> +\n> +\tstrbuf_release(&d);\n> +\n> +\treturn new_path;\n> }\n>\n> const char *git_extract_argv0_path(const char *argv0)\n> @@ -68,16 +72,16 @@ void git_set_argv_exec_path(const char *exec_path)\n>\n>\n> /* Returns the highest-priority, location to look for git programs. */\n> -const char *git_exec_path(void)\n> +char *git_exec_path(void)\n> {\n> \tconst char *env;\n>\n> \tif (argv_exec_path)\n> -\t\treturn argv_exec_path;\n> +\t\treturn strdup(argv_exec_path);\n>\n> \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n> \tif (env && *env) {\n> -\t\treturn env;\n> +\t\treturn strdup(env);\n> \t}\n>\n> \treturn system_path(GIT_EXEC_PATH);\n> @@ -96,7 +100,9 @@ void setup_path(void)\n> \tconst char *old_path = getenv(\"PATH\");\n> \tstruct strbuf new_path = STRBUF_INIT;\n>\n> -\tadd_path(&new_path, git_exec_path());\n> +\tchar *exec_path = git_exec_path();\n> +\n> +\tadd_path(&new_path, exec_path);\n> \tadd_path(&new_path, argv0_path);\n>\n> \tif (old_path)\n> @@ -107,6 +113,7 @@ void setup_path(void)\n> \tsetenv(\"PATH\", new_path.buf, 1);\n>\n> \tstrbuf_release(&new_path);\n> +\tfree(exec_path);\n> }\n>\n> const char **prepare_git_cmd(const char **argv)\n> diff --git a/exec_cmd.h b/exec_cmd.h\n> index e4c9702..03c8599 100644\n> --- a/exec_cmd.h\n> +++ b/exec_cmd.h\n> @@ -3,12 +3,12 @@\n>\n> extern void git_set_argv_exec_path(const char *exec_path);\n> extern const char *git_extract_argv0_path(const char *path);\n> -extern const char *git_exec_path(void);\n> +extern char *git_exec_path(void);\n> extern void setup_path(void);\n> extern const char **prepare_git_cmd(const char **argv);\n> extern int execv_git_cmd(const char **argv); /* NULL terminated */\n> LAST_ARG_MUST_BE_NULL\n> extern int execl_git_cmd(const char *cmd, ...);\n> -extern const char *system_path(const char *path);\n> +extern char *system_path(const char *path);\n>\n> #endif /* GIT_EXEC_CMD_H */\n> diff --git a/git.c b/git.c\n> index 82d7a1c..499dc2a 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -99,13 +99,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\t\texit(0);\n> \t\t\t}\n> \t\t} else if (!strcmp(cmd, \"--html-path\")) {\n> -\t\t\tputs(system_path(GIT_HTML_PATH));\n> +\t\t\tchar *git_html_path = system_path(GIT_HTML_PATH);\n> +\t\t\tputs(git_html_path);\n> +\t\t\tfree(git_html_path);\n> \t\t\texit(0);\n> \t\t} else if (!strcmp(cmd, \"--man-path\")) {\n> -\t\t\tputs(system_path(GIT_MAN_PATH));\n> +\t\t\tchar *git_man_path = system_path(GIT_MAN_PATH);\n> +\t\t\tputs(git_man_path);\n> +\t\t\tfree(git_man_path);\n> \t\t\texit(0);\n> \t\t} else if (!strcmp(cmd, \"--info-path\")) {\n> -\t\t\tputs(system_path(GIT_INFO_PATH));\n> +\t\t\tchar *git_info_path = system_path(GIT_INFO_PATH);\n> +\t\t\tputs(git_info_path);\n> +\t\t\tfree(git_info_path);\n> \t\t\texit(0);\n> \t\t} else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n> \t\t\tuse_pager = 1;\n\n--\nBest regards.\n0xAX\n"},{"id":"252398","messageId":"xmqqmw7hydoe.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"1416750981-24446-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: GIT: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-23T18:28:17Z","receivedAt":"2014-11-23T18:28:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"0xAX writes:\n\n> Hello All,\n> I found memory leak at exec_cmd.c in system_path function with valgrind.\n> After this patch valgrind doesn't show any memory leaks, but I'm not sure\n> that it is the best way to solve this problem. There are a couple of places\n> that uses system_path, if this way will be good, i'll make another patches\n> to fix this leak.\n>\n> Waiting for feedback.\n\nIt is a bit sad that the callers of the function are prepared for\nthe returned value to be volatile (that is why the ones that want to\ndo something else between the time they call it and the time they\nuse the value returned do make copies), while other callers that\nimmediately use the returned value do not have to be worried about\nfreeing it, but with the change you have to force everybody to\nremember freeing the returned value.\n\nIf you instead limited to your change only to these two points:\n\n - make \"struct strbuf d\" static;\n - return d.buf instead of the result of strbuf_detach(&d)\n\nthe updated system_path() will keep the promise all the caller\nexpects from it, i.e. the value out of the function is volatile but\nthe callers do not have to free it, in all cases, no?\n\n> Signed-off-by: Alex Kuleshov \n\nPlease have that S-o-b: line (and the same name in GIT_AUTHOR_NAME)\non the patches that we'd use \"git am\" on, not on the cover letter\n;-). Nom de guerre nobody else recognizes outside your close\nfriends may be fun, but I feel that it is not quite appropriate to\nappear in \"git shortlog -s\" output on a public project like ours.\n\nThanks.\n"},{"id":"252399","messageId":"xmqqioi5ycme.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"1416750981-24446-2-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-23T18:51:05Z","receivedAt":"2014-11-23T18:51:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"0xAX writes:\n\n> Signed-off-by: 0xAX \n> ---\n\nThe comment on names I've already mentioned elsewhere.\n\nYou need a better explanation than a \"no log message\", as you are\nnot doing \"system-path memory leak fix\".\n\nYou are doing a lot more. Perhaps the story would start like this:\n\n system_path(): make the callers own the returned string\n\n The function sometimes returns a newly allocated string and\n sometimes returns a borrowed string, the latter of which the\n callers must not free().\n\n The existing callers all assume that the return value belongs to\n the callee and most of them copy it with strdup() when they want\n to keep it around. They end up leaking the returned copy when\n the callee returned a new string.\n \n Change the contract between the callers and system_path() to\n make the returned string owned by the callers; they are\n responsible for freeing it when done, but they do not have to\n make their own copy to store it away.\n\n This accidentally fixes some unsafe callers as well. For\n example, ...\n\n\n> exec_cmd.c | 25 ++++++++++++++++---------\n> exec_cmd.h | 4 ++--\n> git.c | 12 +++++++++---\n\nEven though I said that this changes the contract between the caller\nand the callee and make things wasteful for some, I personally think\nit is going in the right direction.\n\nThe change accidentally fixes some unsafe callers. For example, the\nfirst hit from \"git grep system_path\" is this:\n\n attr.c- static const char *system_wide;\n attr.c- if (!system_wide)\n attr.c: system_wide = system_path(ETC_GITATTRIBUTES);\n attr.c- return system_wide;\n\nThis is obviously unsafe for a volatile return value from the callee\nand needs to have strdup() on it, but with the patch there no longer\nis need for such a caller-side strdup().\n\nBut this change also introduces new bugs, I think. For example, the\nsecond hit from \"git grep system_path\" is this:\n\n builtin/help.c: strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n\nNow the caller owns and is responsible for freeing the returned\nvalue, but without opening the file in question in an editor or a\npager we can tell immediately that there is no way this line is not\nadding a new memory leak.\n\n> index 698e752..08f8f80 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -6,7 +6,7 @@\n> static const char *argv_exec_path;\n> static const char *argv0_path;\n> \n> -const char *system_path(const char *path)\n> +char *system_path(const char *path)\n> {\n> #ifdef RUNTIME_PREFIX\n> \tstatic const char *prefix;\n> @@ -14,9 +14,10 @@ const char *system_path(const char *path)\n> \tstatic const char *prefix = PREFIX;\n> #endif\n> \tstruct strbuf d = STRBUF_INIT;\n> +\tchar *new_path = NULL;\n> \n> \tif (is_absolute_path(path))\n> -\t\treturn path;\n> +\t\treturn strdup(path);\n> \n> #ifdef RUNTIME_PREFIX\n> \tassert(argv0_path);\n> @@ -32,10 +33,13 @@ const char *system_path(const char *path)\n> \t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n> \t}\n> #endif\n> -\n> \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n> -\tpath = strbuf_detach(&d, NULL);\n> -\treturn path;\n> +\tnew_path = malloc((strlen(prefix) + strlen(path)) + 2);\n> +\tsprintf(new_path, \"%s/%s\", prefix, path);\n> +\n> +\tstrbuf_release(&d);\n> +\n> +\treturn new_path;\n\nAre you duplicating what strbuf_addf() is doing on the strbuf d,\nmanually creating the same in new_path, while discarding what the\nexisting code you did not remove with this patch already computed?\n\nIsn't it sufficient to add strdup(path) for the absolute case and do\nnothing else to this function? I have no idea what you are doing\nhere.\n"},{"id":"252402","messageId":"87tx1pensq.fsf@gmail.com","threadId":"38028","inReplyTo":"xmqqioi5ycme.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Alex Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-23T19:06:32Z","receivedAt":"2014-11-23T19:06:32Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nJunio C Hamano @ 2014-11-24 00:51 ALMT:\n\n> 0xAX writes:\n>\n>> Signed-off-by: 0xAX \n>> ---\n>\n> The comment on names I've already mentioned elsewhere.\n\nYes, i understand about names.\n\n>\n> You need a better explanation than a \"no log message\", as you are\n> not doing \"system-path memory leak fix\".\n>\n> You are doing a lot more. Perhaps the story would start like this:\n>\n> system_path(): make the callers own the returned string\n\nDid it.\n\n>\n> The function sometimes returns a newly allocated string and\n> sometimes returns a borrowed string, the latter of which the\n> callers must not free().\n>\n> The existing callers all assume that the return value belongs to\n> the callee and most of them copy it with strdup() when they want\n> to keep it around. They end up leaking the returned copy when\n> the callee returned a new string.\n>\n> Change the contract between the callers and system_path() to\n> make the returned string owned by the callers; they are\n> responsible for freeing it when done, but they do not have to\n> make their own copy to store it away.\n\nYes you're right, i just started to read git source code some days ago,\nand it's hard to understand in some places for the start. Now i see it,\nthanks for explanation.\n\n>\n> This accidentally fixes some unsafe callers as well. For\n> example, ...\n>\n>\n>> exec_cmd.c | 25 ++++++++++++++++---------\n>> exec_cmd.h | 4 ++--\n>> git.c | 12 +++++++++---\n>\n> Even though I said that this changes the contract between the caller\n> and the callee and make things wasteful for some, I personally think\n> it is going in the right direction.\n>\n> The change accidentally fixes some unsafe callers. For example, the\n> first hit from \"git grep system_path\" is this:\n>\n> attr.c- static const char *system_wide;\n> attr.c- if (!system_wide)\n> attr.c: system_wide = system_path(ETC_GITATTRIBUTES);\n> attr.c- return system_wide;\n>\n> This is obviously unsafe for a volatile return value from the callee\n> and needs to have strdup() on it, but with the patch there no longer\n> is need for such a caller-side strdup().\n>\n> But this change also introduces new bugs, I think. For example, the\n> second hit from \"git grep system_path\" is this:\n>\n> builtin/help.c: strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n>\n> Now the caller owns and is responsible for freeing the returned\n> value, but without opening the file in question in an editor or a\n> pager we can tell immediately that there is no way this line is not\n> adding a new memory leak.\n>\n>> index 698e752..08f8f80 100644\n>> --- a/exec_cmd.c\n>> +++ b/exec_cmd.c\n>> @@ -6,7 +6,7 @@\n>> static const char *argv_exec_path;\n>> static const char *argv0_path;\n>>\n>> -const char *system_path(const char *path)\n>> +char *system_path(const char *path)\n>> {\n>> #ifdef RUNTIME_PREFIX\n>> \tstatic const char *prefix;\n>> @@ -14,9 +14,10 @@ const char *system_path(const char *path)\n>> \tstatic const char *prefix = PREFIX;\n>> #endif\n>> \tstruct strbuf d = STRBUF_INIT;\n>> +\tchar *new_path = NULL;\n>>\n>> \tif (is_absolute_path(path))\n>> -\t\treturn path;\n>> +\t\treturn strdup(path);\n>>\n>> #ifdef RUNTIME_PREFIX\n>> \tassert(argv0_path);\n>> @@ -32,10 +33,13 @@ const char *system_path(const char *path)\n>> \t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n>> \t}\n>> #endif\n>> -\n>> \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n>> -\tpath = strbuf_detach(&d, NULL);\n>> -\treturn path;\n>> +\tnew_path = malloc((strlen(prefix) + strlen(path)) + 2);\n>> +\tsprintf(new_path, \"%s/%s\", prefix, path);\n>> +\n>> +\tstrbuf_release(&d);\n>> +\n>> +\treturn new_path;\n>\n> Are you duplicating what strbuf_addf() is doing on the strbuf d,\n> manually creating the same in new_path, while discarding what the\n> existing code you did not remove with this patch already computed?\n>\n> Isn't it sufficient to add strdup(path) for the absolute case and do\n> nothing else to this function? I have no idea what you are doing\n> here.\n\nI have added changes from your previous feedback, how can I attach\nsecond (changed) patch to this mail thread with git send-email?\n\n--\nBest regards.\n0xAX\n"},{"id":"252403","messageId":"87sih9en65.fsf@gmail.com","threadId":"38028","inReplyTo":"xmqqioi5ycme.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Alex Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-23T19:19:29Z","receivedAt":"2014-11-23T19:19:29Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nSigned-off-by: Alex Kuleshov \n---\n exec_cmd.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 698e752..7ed9bcc 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -13,7 +13,7 @@ const char *system_path(const char *path)\n #else\n \tstatic const char *prefix = PREFIX;\n #endif\n-\tstruct strbuf d = STRBUF_INIT;\n+\tstatic struct strbuf d = STRBUF_INIT;\n\n \tif (is_absolute_path(path))\n \t\treturn path;\n@@ -34,8 +34,7 @@ const char *system_path(const char *path)\n #endif\n\n \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\treturn d.buf;\n }\n\n const char *git_extract_argv0_path(const char *argv0)\n@@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path)\n void setup_path(void)\n {\n \tconst char *old_path = getenv(\"PATH\");\n-\tstruct strbuf new_path = STRBUF_INIT;\n+\tstatic struct strbuf new_path = STRBUF_INIT;\n\n \tadd_path(&new_path, git_exec_path());\n \tadd_path(&new_path, argv0_path);\n--\n2.2.0.rc3.190.g952f0aa.dirty\n"},{"id":"252405","messageId":"20141123194247.GA16605@peff.net","threadId":"38028","inReplyTo":"87sih9en65.fsf@gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-23T19:42:47Z","receivedAt":"2014-11-23T19:42:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 24, 2014 at 01:19:29AM +0600, Alex Kuleshov wrote:\n\n> \n> Signed-off-by: Alex Kuleshov \n> ---\n> exec_cmd.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n> \n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index 698e752..7ed9bcc 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -13,7 +13,7 @@ const char *system_path(const char *path)\n> #else\n> \tstatic const char *prefix = PREFIX;\n> #endif\n> -\tstruct strbuf d = STRBUF_INIT;\n> +\tstatic struct strbuf d = STRBUF_INIT;\n> \n> \tif (is_absolute_path(path))\n> \t\treturn path;\n> @@ -34,8 +34,7 @@ const char *system_path(const char *path)\n> #endif\n> \n> \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n> -\tpath = strbuf_detach(&d, NULL);\n> -\treturn path;\n> +\treturn d.buf;\n> }\n\nIf I am reading this right, calls to system_path() will always reuse the\nsame buffer, even if they are called with another \"path\" argument. So\nall callers must make sure to make a copy if they are going to hold on\nto it for a long time. Grepping for callers shows us saving the result\nto a static variable in at least git_etc_gitattributes, copy_templates,\nand get_html_page_path. Don't these all need to learn to xstrdup the\nreturn value?\n\n-Peff\n"},{"id":"252408","messageId":"CAPig+cS11fQMUkn2D=3oqac=NA2wRp9rap_XOk=Jfj4pPGWP+Q@mail.gmail.com","threadId":"38028","inReplyTo":"87sih9en65.fsf@gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-11-23T20:07:15Z","receivedAt":"2014-11-23T20:07:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 23, 2014 at 2:19 PM, Alex Kuleshov wrote:\n>\n> Signed-off-by: Alex Kuleshov \n> ---\n> exec_cmd.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index 698e752..7ed9bcc 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -13,7 +13,7 @@ const char *system_path(const char *path)\n> #else\n> static const char *prefix = PREFIX;\n> #endif\n> - struct strbuf d = STRBUF_INIT;\n> + static struct strbuf d = STRBUF_INIT;\n\nCurious. Did the unit tests pass with this change?\n\nIn the original code, the strbuf starts out empty each time\nsystem_path() is called, and strbuf_addf() populates it. After your\nchange, the strbuf starts empty only on the very first call, and\nsubsequent calls append more content rather than replacing it. At the\nvery least, to fix, you now need to invoke strbuf_reset() before\ninvoking strbuf_addf().\n\n> if (is_absolute_path(path))\n> return path;\n> @@ -34,8 +34,7 @@ const char *system_path(const char *path)\n> #endif\n>\n> strbuf_addf(&d, \"%s/%s\", prefix, path);\n> - path = strbuf_detach(&d, NULL);\n> - return path;\n> + return d.buf;\n> }\n>\n> const char *git_extract_argv0_path(const char *argv0)\n> @@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path)\n> void setup_path(void)\n> {\n> const char *old_path = getenv(\"PATH\");\n> - struct strbuf new_path = STRBUF_INIT;\n> + static struct strbuf new_path = STRBUF_INIT;\n>\n> add_path(&new_path, git_exec_path());\n> add_path(&new_path, argv0_path);\n\nNot sure what this change is about. The last couple lines of this function are:\n\n setenv(\"PATH\", new_path.buf, 1);\n strbuf_release(&new_path);\n\nwhich means that the buffer held by the strbuf is being released\nanyhow, whether static or not.\n"},{"id":"252409","messageId":"xmqq7fyly3xj.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"87sih9en65.fsf@gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-23T21:58:48Z","receivedAt":"2014-11-23T21:58:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Kuleshov writes:\n\n> Signed-off-by: Alex Kuleshov \n\nSo this one does not change the contract between the caller and the\ncallee. It does not change the fact that you need to explain your\nchange, though. The story should read something like this:\n\n system_path(): always return a volatile result\n\n The function sometimes returns a newly allocated string and\n sometimes returns a borrowed string, the latter of which the\n callers must not free().\n\n The existing callers all assume that the return value belongs to\n the callee and most of them copy it with strdup() when they want\n to keep it around. They end up leaking the returned copy when\n the callee returned a new string.\n\n Make sure all callers receive a volatile result, so that they do\n not have to be worried about freeing the returned string.\n\n There however are existing callers that are already broken, for\n example, ...\n\n Fixing these callers are done as separate patches, that can be\n applied either before or after this patch.\n\nPersonally, as I already said, I think the approach the previous\nversion took (but not the implementation) to change the contract\nbetween the callers and this function is probably a good idea in\nthe longer term. Leaking a bit by forgetting to convert a few\ncallers to free the returned values will not affect the correctness,\nbut making the returned value consistently more volatile will\nexpose existing breakage more often and will break codepaths that\nhappened to be working by accident.\n\nThanks.\n"},{"id":"252416","messageId":"87mw7haxdp.fsf@gmail.com","threadId":"38028","inReplyTo":"xmqq7fyly3xj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Alex Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-24T07:02:22Z","receivedAt":"2014-11-24T07:02:22Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nJeff King:\n\n\n>If I am reading this right, calls to system_path() will always reuse the\n>same buffer, even if they are called with another \"path\" argument. So\n>all callers must make sure to make a copy if they are going to hold on\n>to it for a long time. Grepping for callers shows us saving the result\n>to a static variable in at least git_etc_gitattributes, copy_templates,\n>and get_html_page_path. Don't these all need to learn to xstrdup the\n>return value?\n\nHello Jeff, yes as i wrote in previous message i saw that there some\nplaces uses system_path function and I just wanted to find correct way\nto solve problem with system_path, in near time i'll check and adapt if\nneed all of this places for updated system_path.\n\nEric Sunshine:\n\n>Curious. Did the unit tests pass with this change?\n\nYes i launched unit tests and there are the same result as in origin/pu.\n\n>Not sure what this change is about. The last couple lines of this function are:\n>\n> setenv(\"PATH\", new_path.buf, 1);\n> strbuf_release(&new_path);\n>\n>which means that the buffer held by the strbuf is being released\n>anyhow, whether static or not.\n\nAh, yes, just a type, will update it.\n\nJunio C Hamano:\n\n>Fixing these callers are done as separate patches, that can be\n>applied either before or after this patch.\n\nHow to do it better? Update this patch, fix all callers which broken and\nconcat this patches to one or make separate patches?\n\nThanks all for feedback.\n\n--\nBest regards.\n0xAX\n"},{"id":"252419","messageId":"CAPc5daWnJCGPTMA0Hyg8QEAmsnF17ZQ9njGq8xVaUAfuwCRQDA@mail.gmail.com","threadId":"38028","inReplyTo":"87mw7haxdp.fsf@gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-24T07:37:50Z","receivedAt":"2014-11-24T07:37:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[jc: added those who were mentioned but were missing back to Cc]\n\nOn Sun, Nov 23, 2014 at 11:02 PM, Alex Kuleshov wrote:\n>\n> Junio C Hamano:\n>\n>>Fixing these callers are done as separate patches, that can be\n>>applied either before or after this patch.\n>\n> How to do it better? Update this patch, fix all callers which broken and\n> concat this patches to one or make separate patches?\n\nAs I said, I do not think the approach your v2 takes is better than the original\napproach to pass the ownership of the returned value to the caller. I'd say that\na cleaned up v1 that makes sure it adds a necessary strdup() in the codepath\nwhere it returns an absolute pathname given as-is, with necessary changes to\ncallers that do not currently free the received result to free it when they are\ndone, and to callers that currently do strdup() of the received result not to do\nstrdup(), in a single patch, would be the right thing to do.\n\nI think I already wrote the bulk of proposed commit message for you for such\na change earlier ;-) The one that talks about changing the contract between the\nsystem_path() and its callers.\n\nThanks.\n"},{"id":"252420","messageId":"87lhn1au67.fsf@gmail.com","threadId":"38028","inReplyTo":"CAPc5daWnJCGPTMA0Hyg8QEAmsnF17ZQ9njGq8xVaUAfuwCRQDA@mail.gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Alex Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-24T08:12:47Z","receivedAt":"2014-11-24T08:12:47Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nJunio C Hamano @ 2014-11-24 13:37 ALMT:\n\n> [jc: added those who were mentioned but were missing back to Cc]\n>\n> On Sun, Nov 23, 2014 at 11:02 PM, Alex Kuleshov wrote:\n>>\n>> Junio C Hamano:\n>>\n>>>Fixing these callers are done as separate patches, that can be\n>>>applied either before or after this patch.\n>>\n>> How to do it better? Update this patch, fix all callers which broken and\n>> concat this patches to one or make separate patches?\n>\n> As I said, I do not think the approach your v2 takes is better than the original\n> approach to pass the ownership of the returned value to the caller. I'd say that\n> a cleaned up v1 that makes sure it adds a necessary strdup() in the codepath\n> where it returns an absolute pathname given as-is, with necessary changes to\n> callers that do not currently free the received result to free it when they are\n> done, and to callers that currently do strdup() of the received result not to do\n> strdup(), in a single patch, would be the right thing to do.\n>\n> I think I already wrote the bulk of proposed commit message for you for such\n> a change earlier ;-) The one that talks about changing the contract between the\n> system_path() and its callers.\n>\n> Thanks.\n\nOK, but i'm little confused now, long thread :)\n\nSo what's our next strategy?\n\nI'll make system_path will return volatile value, so callers must free\nit by itself, but we have two types of callers:\n\nCallers which doesn't need to store system_path return value (like git\n--man-path and etc.. in git.c), and which need to store it (as in attr.c).\n\nWe must to free system_path return value in first case, and does not\nneed to create copy of returned string with xstrdup.\n\nAm i correct or i'm wrong with this?\n\nThank you.\n\n--\nBest regards.\n0xAX\n"},{"id":"252427","messageId":"87fvd891fy.fsf@gmail.com","threadId":"38028","inReplyTo":"CAPc5daWnJCGPTMA0Hyg8QEAmsnF17ZQ9njGq8xVaUAfuwCRQDA@mail.gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-24T13:11:40Z","receivedAt":"2014-11-24T13:11:40Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nNow system_path returns path which is allocated string to callers;\nIt prevents memory leaks in some places. All callers of system_path\nare owners of path string and they must release it.\n\nSigned-off-by: Alexander Kuleshov \n---\n attr.c | 8 +++---\n builtin/config.c | 73 +++++++++++++++++++++++++++++++++++++++++--------------\n builtin/help.c | 30 ++++++++++++++++-------\n builtin/init-db.c | 12 +++++++--\n cache.h | 4 +--\n config.c | 10 +++++---\n exec_cmd.c | 22 ++++++++---------\n exec_cmd.h | 4 +--\n git.c | 16 +++++++++---\n wrapper.c | 4 ++-\n 10 files changed, 127 insertions(+), 56 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex cd54697..f96ddb4 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -462,9 +462,9 @@ static void drop_attr_stack(void)\n \t}\n }\n\n-static const char *git_etc_gitattributes(void)\n+static char *git_etc_gitattributes(void)\n {\n-\tstatic const char *system_wide;\n+\tstatic char *system_wide;\n \tif (!system_wide)\n \t\tsystem_wide = system_path(ETC_GITATTRIBUTES);\n \treturn system_wide;\n@@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)\n \tattr_stack = elem;\n\n \tif (git_attr_system()) {\n-\t\telem = read_attr_from_file(git_etc_gitattributes(), 1);\n+\t\tchar *etc_attributes = git_etc_gitattributes();\n+\t\telem = read_attr_from_file(etc_attributes, 1);\n+\t\tfree(etc_attributes);\n \t\tif (elem) {\n \t\t\telem->origin = NULL;\n \t\t\telem->prev = attr_stack;\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 15a7bea..8ab3179 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (given_config_source.blob)\n \t\t\tdie(\"editing blobs is not supported\");\n \t\tgit_config(git_default_config, NULL);\n-\t\tconfig_file = xstrdup(given_config_source.file ?\n-\t\t\t\t given_config_source.file : git_path(\"config\"));\n+\n+\t\tif (use_system_config)\n+\t\t\tconfig_file = given_config_source.file ?\n+\t\t\t\t\t\t given_config_source.file : git_path(\"config\");\n+\t\telse\n+\t\t\tconfig_file = xstrdup(given_config_source.file ?\n+\t\t\t\t\t\t\t\t given_config_source.file : git_path(\"config\"));\n+\n \t\tif (use_global_config) {\n \t\t\tint fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \t\t\tif (fd) {\n@@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (ret == CONFIG_NOTHING_SET)\n \t\t\terror(\"cannot overwrite multiple values with a single value\\n\"\n \t\t\t\" Use a regexp, --add or --replace-all to change %s.\", argv[0]);\n+\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\treturn ret;\n \t}\n \telse if (actions == ACTION_SET_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value, argv[2], 0);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t argv[0], value, argv[2], 0);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_ADD) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value,\n-\t\t\t\t\t\t CONFIG_REGEX_NONE, 0);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file, argv[0],\n+\t\t\t\t\t\t\t\t\t\t\t value, CONFIG_REGEX_NONE, 0);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value, argv[2], 1);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file, argv[0],\n+\t\t\t\t\t\t\t\t\t\t\t value, argv[2], 1);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_GET) {\n \t\tcheck_argc(argc, 1, 2);\n@@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\treturn get_urlmatch(argv[0], argv[1]);\n \t}\n \telse if (actions == ACTION_UNSET) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n-\t\tif (argc == 2)\n-\t\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t\t argv[0], NULL, argv[1], 0);\n-\t\telse\n-\t\t\treturn git_config_set_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], NULL);\n+\t\tif (argc == 2){\n+\t\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t\t argv[0], NULL, argv[1], 0);\n+\t\t\tif (use_system_config)\n+\t\t\t\tfree(given_config_source.file);\n+\t\t\treturn ret;\n+\t\t}\n+\t\telse{\n+\t\t\tret = git_config_set_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t argv[0], NULL);\n+\t\t\tif (use_system_config)\n+\t\t\t\tfree(given_config_source.file);\n+\t\t\treturn ret;\n+\t\t}\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], NULL, argv[1], 1);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t argv[0], NULL, argv[1], 1);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tret = git_config_rename_section_in_file(given_config_source.file,\n-\t\t\t\t\t\t\targv[0], argv[1]);\n+\t\t\t\t\t\t\t\t\t\t\t\targv[0], argv[1]);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t\tif (ret == 0)\n@@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 1, 1);\n \t\tret = git_config_rename_section_in_file(given_config_source.file,\n \t\t\t\t\t\t\targv[0], NULL);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t\tif (ret == 0)\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b3c818e..20ffbb1 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -326,7 +326,8 @@ static void setup_man_path(void)\n \t * old_path, the ':' at the end will let 'man' to try\n \t * system-wide paths after ours to find the manual page. If\n \t * there is old_path, we need ':' as delimiter. */\n-\tstrbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n+\tchar *git_man_path = system_path(GIT_MAN_PATH);\n+\tstrbuf_addstr(&new_path, git_man_path);\n \tstrbuf_addch(&new_path, ':');\n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n@@ -334,6 +335,7 @@ static void setup_man_path(void)\n \tsetenv(\"MANPATH\", new_path.buf, 1);\n\n \tstrbuf_release(&new_path);\n+\tfree(git_man_path);\n }\n\n static void exec_viewer(const char *name, const char *page)\n@@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd)\n static void show_info_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n-\tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n+\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\tsetenv(\"INFOPATH\", git_info_path, 1);\n+\tfree(git_info_path);\n \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n \tdie(_(\"no info viewer handled the request\"));\n }\n\n-static void get_html_page_path(struct strbuf *page_path, const char *page)\n+static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)\n {\n \tstruct stat st;\n-\tif (!html_path)\n-\t\thtml_path = system_path(GIT_HTML_PATH);\n\n \t/* Check that we have a git documentation directory. */\n \tif (!strstr(html_path, \"://\")) {\n \t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n-\t\t || !S_ISREG(st.st_mode))\n-\t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n+\t\t\t\t|| !S_ISREG(st.st_mode)){\n+\t\t\tprintf(\"'%s': not a documentation directory.\\n\", html_path);\n+\t\t\tfree(html_path);\n+\t\t\texit(1);\n+\t\t}\n \t}\n\n \tstrbuf_init(page_path, 0);\n@@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)\n * HTML.\n */\n #ifndef open_html\n-static void open_html(const char *path)\n+static void open_html(char *path)\n {\n \texecl_git_cmd(\"web--browse\", \"-c\", \"help.browser\", path, (char *)NULL);\n+\tfree(path);\n }\n #endif\n\n@@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n \tstruct strbuf page_path; /* it leaks but we exec bellow */\n+\tchar* html_path = NULL;\n+\n+\tif (!html_path)\n+\t\thtml_path = system_path(GIT_HTML_PATH);\n\n-\tget_html_page_path(&page_path, page);\n+\tget_html_page_path(&page_path, page, html_path);\n\n \topen_html(page_path.buf);\n }\n@@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n\n \treturn 0;\n }\n+\n+// git help --web commit\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 2619ca5..6f36f3f 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)\n \t\ttemplate_dir = init_db_template_dir;\n \tif (!template_dir)\n \t\ttemplate_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);\n-\tif (!template_dir[0])\n+\tif (!template_dir[0]){\n+\t\tif (!template_dir)\n+\t\t\tfree((char*)template_dir);\n \t\treturn;\n+\t}\n \ttemplate_len = strlen(template_dir);\n-\tif (PATH_MAX <= (template_len+strlen(\"/config\")))\n+\tif (PATH_MAX <= (template_len+strlen(\"/config\"))){\n+\t\tif (!template_dir)\n+\t\t\tfree((char*)template_dir);\n \t\tdie(_(\"insanely long template path %s\"), template_dir);\n+\t}\n \tstrcpy(template_path, template_dir);\n \tif (template_path[template_len-1] != '/') {\n \t\ttemplate_path[template_len++] = '/';\n@@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir)\n \tdir = opendir(template_path);\n \tif (!dir) {\n \t\twarning(_(\"templates not found %s\"), template_dir);\n+\t\tfree((char*)template_dir);\n \t\treturn;\n \t}\n\n@@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir)\n \t\t\t\"a wrong format version %d from '%s'\"),\n \t\t\trepository_format_version,\n \t\t\ttemplate_dir);\n+\t\tfree((char*)template_dir);\n \t\tclosedir(dir);\n \t\treturn;\n \t}\ndiff --git a/cache.h b/cache.h\nindex 1b05115..7048e7a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1354,7 +1354,7 @@ extern int update_server_info(int);\n\n struct git_config_source {\n \tunsigned int use_stdin:1;\n-\tconst char *file;\n+\tchar *file;\n \tconst char *blob;\n };\n\n@@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int\n extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\n extern int git_config_rename_section_in_file(const char *, const char *, const char *);\n-extern const char *git_etc_gitconfig(void);\n+extern char *git_etc_gitconfig(void);\n extern int check_repository_format_version(const char *var, const char *value, void *cb);\n extern int git_env_bool(const char *, int);\n extern unsigned long git_env_ulong(const char *, unsigned long);\ndiff --git a/config.c b/config.c\nindex ae1398f..0ed8890 100644\n--- a/config.c\n+++ b/config.c\n@@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn,\n \treturn git_config_from_blob_sha1(fn, name, sha1, data);\n }\n\n-const char *git_etc_gitconfig(void)\n+char *git_etc_gitconfig(void)\n {\n-\tstatic const char *system_wide;\n+\tstatic char *system_wide;\n \tif (!system_wide)\n \t\tsystem_wide = system_path(ETC_GITCONFIG);\n \treturn system_wide;\n@@ -1172,13 +1172,15 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \tint ret = 0, found = 0;\n \tchar *xdg_config = NULL;\n \tchar *user_config = NULL;\n+\tchar *git_etc_config = git_etc_gitconfig();\n\n \thome_config_paths(&user_config, &xdg_config, \"config\");\n\n-\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n-\t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n+\tif (git_etc_config && !access_or_die(git_etc_config, R_OK, 0)) {\n+\t\tret += git_config_from_file(fn, git_etc_config,\n \t\t\t\t\t data);\n \t\tfound += 1;\n+\t\tfree(git_etc_config);\n \t}\n\n \tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 698e752..d35ecac 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -6,7 +6,7 @@\n static const char *argv_exec_path;\n static const char *argv0_path;\n\n-const char *system_path(const char *path)\n+char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n \tstatic const char *prefix;\n@@ -16,7 +16,7 @@ const char *system_path(const char *path)\n \tstruct strbuf d = STRBUF_INIT;\n\n \tif (is_absolute_path(path))\n-\t\treturn path;\n+\t\treturn strdup(path);\n\n #ifdef RUNTIME_PREFIX\n \tassert(argv0_path);\n@@ -34,8 +34,7 @@ const char *system_path(const char *path)\n #endif\n\n \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\treturn d.buf;\n }\n\n const char *git_extract_argv0_path(const char *argv0)\n@@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)\n\n\n /* Returns the highest-priority, location to look for git programs. */\n-const char *git_exec_path(void)\n+char *git_exec_path(void)\n {\n-\tconst char *env;\n+\tchar *env;\n\n \tif (argv_exec_path)\n-\t\treturn argv_exec_path;\n+\t\treturn strdup(argv_exec_path);\n\n \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n-\tif (env && *env) {\n-\t\treturn env;\n-\t}\n+\tif (env && *env)\n+\t\treturn strdup(env);\n\n \treturn system_path(GIT_EXEC_PATH);\n }\n@@ -96,7 +94,8 @@ void setup_path(void)\n \tconst char *old_path = getenv(\"PATH\");\n \tstruct strbuf new_path = STRBUF_INIT;\n\n-\tadd_path(&new_path, git_exec_path());\n+\tchar *exec_path = git_exec_path();\n+\tadd_path(&new_path, exec_path);\n \tadd_path(&new_path, argv0_path);\n\n \tif (old_path)\n@@ -107,6 +106,7 @@ void setup_path(void)\n \tsetenv(\"PATH\", new_path.buf, 1);\n\n \tstrbuf_release(&new_path);\n+\tfree(exec_path);\n }\n\n const char **prepare_git_cmd(const char **argv)\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex e4c9702..03c8599 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -3,12 +3,12 @@\n\n extern void git_set_argv_exec_path(const char *exec_path);\n extern const char *git_extract_argv0_path(const char *path);\n-extern const char *git_exec_path(void);\n+extern char *git_exec_path(void);\n extern void setup_path(void);\n extern const char **prepare_git_cmd(const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n LAST_ARG_MUST_BE_NULL\n extern int execl_git_cmd(const char *cmd, ...);\n-extern const char *system_path(const char *path);\n+extern char *system_path(const char *path);\n\n #endif /* GIT_EXEC_CMD_H */\ndiff --git a/git.c b/git.c\nindex 82d7a1c..d01c4f1 100644\n--- a/git.c\n+++ b/git.c\n@@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tif (*cmd == '=')\n \t\t\t\tgit_set_argv_exec_path(cmd + 1);\n \t\t\telse {\n-\t\t\t\tputs(git_exec_path());\n+\t\t\t\tchar *exec_path = git_exec_path();\n+\t\t\t\tputs(exec_path);\n+\t\t\t\tfree(exec_path);\n \t\t\t\texit(0);\n \t\t\t}\n \t\t} else if (!strcmp(cmd, \"--html-path\")) {\n-\t\t\tputs(system_path(GIT_HTML_PATH));\n+\t\t\tchar *git_html_path = system_path(GIT_HTML_PATH);\n+\t\t\tputs(git_html_path);\n+\t\t\tfree(git_html_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--man-path\")) {\n-\t\t\tputs(system_path(GIT_MAN_PATH));\n+\t\t\tchar *git_man_path = system_path(GIT_MAN_PATH);\n+\t\t\tputs(git_man_path);\n+\t\t\tfree(git_man_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--info-path\")) {\n-\t\t\tputs(system_path(GIT_INFO_PATH));\n+\t\t\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\t\t\tputs(git_info_path);\n+\t\t\tfree(git_info_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n \t\t\tuse_pager = 1;\ndiff --git a/wrapper.c b/wrapper.c\nindex 007ec0d..ede9268 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -526,8 +526,10 @@ int access_or_warn(const char *path, int mode, unsigned flag)\n int access_or_die(const char *path, int mode, unsigned flag)\n {\n \tint ret = access(path, mode);\n-\tif (ret && !access_error_is_ok(errno, flag))\n+\tif (ret && !access_error_is_ok(errno, flag)){\n+\t\tfree((char*)path);\n \t\tdie_errno(_(\"unable to access '%s'\"), path);\n+\t}\n \treturn ret;\n }\n\n--\n2.2.0-rc3\n"},{"id":"252428","messageId":"87mw7gae8k.fsf@gmail.com","threadId":"38028","inReplyTo":"CAPc5daWnJCGPTMA0Hyg8QEAmsnF17ZQ9njGq8xVaUAfuwCRQDA@mail.gmail.com","subject":"Re: [PATCH] exec_cmd: system_path memory leak fix","fromName":"Alex Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-24T14:00:40Z","receivedAt":"2014-11-24T14:00:40Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nNow system_path returns path which is allocated string to callers;\nIt prevents memory leaks in some places. All callers of system_path\nare owners of path string and they must release it.\n\nAdded new parameter to wrapper.c/int access_or_die - etc_config, because\nonly etc_config in this case use system_path and we don't know do we\nneed to free path or not.\n\nSigned-off-by: Alexander Kuleshov \n---\n attr.c | 8 +++---\n builtin/config.c | 73 +++++++++++++++++++++++++++++++++++++++++--------------\n builtin/help.c | 30 ++++++++++++++++-------\n builtin/init-db.c | 12 +++++++--\n cache.h | 4 +--\n config.c | 19 ++++++++-------\n exec_cmd.c | 22 ++++++++---------\n exec_cmd.h | 4 +--\n git-compat-util.h | 2 +-\n git.c | 16 +++++++++---\n wrapper.c | 7 ++++--\n 11 files changed, 134 insertions(+), 63 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex cd54697..f96ddb4 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -462,9 +462,9 @@ static void drop_attr_stack(void)\n \t}\n }\n\n-static const char *git_etc_gitattributes(void)\n+static char *git_etc_gitattributes(void)\n {\n-\tstatic const char *system_wide;\n+\tstatic char *system_wide;\n \tif (!system_wide)\n \t\tsystem_wide = system_path(ETC_GITATTRIBUTES);\n \treturn system_wide;\n@@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)\n \tattr_stack = elem;\n\n \tif (git_attr_system()) {\n-\t\telem = read_attr_from_file(git_etc_gitattributes(), 1);\n+\t\tchar *etc_attributes = git_etc_gitattributes();\n+\t\telem = read_attr_from_file(etc_attributes, 1);\n+\t\tfree(etc_attributes);\n \t\tif (elem) {\n \t\t\telem->origin = NULL;\n \t\t\telem->prev = attr_stack;\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 15a7bea..266d42b 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (given_config_source.blob)\n \t\t\tdie(\"editing blobs is not supported\");\n \t\tgit_config(git_default_config, NULL);\n-\t\tconfig_file = xstrdup(given_config_source.file ?\n-\t\t\t\t given_config_source.file : git_path(\"config\"));\n+\n+\t\tif (use_system_config)\n+\t\t\tconfig_file = given_config_source.file ?\n+\t\t\t\t\t\t\tgiven_config_source.file : git_path(\"config\");\n+\t\telse\n+\t\t\tconfig_file = xstrdup(given_config_source.file ?\n+\t\t\t\t\t\t\t\t given_config_source.file : git_path(\"config\"));\n+\n \t\tif (use_global_config) {\n \t\t\tint fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \t\t\tif (fd) {\n@@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (ret == CONFIG_NOTHING_SET)\n \t\t\terror(\"cannot overwrite multiple values with a single value\\n\"\n \t\t\t\" Use a regexp, --add or --replace-all to change %s.\", argv[0]);\n+\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\treturn ret;\n \t}\n \telse if (actions == ACTION_SET_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value, argv[2], 0);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t argv[0], value, argv[2], 0);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_ADD) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value,\n-\t\t\t\t\t\t CONFIG_REGEX_NONE, 0);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file, argv[0],\n+\t\t\t\t\t\t\t\t\t\t\t value, CONFIG_REGEX_NONE, 0);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value, argv[2], 1);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file, argv[0],\n+\t\t\t\t\t\t\t\t\t\t\t value, argv[2], 1);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_GET) {\n \t\tcheck_argc(argc, 1, 2);\n@@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\treturn get_urlmatch(argv[0], argv[1]);\n \t}\n \telse if (actions == ACTION_UNSET) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n-\t\tif (argc == 2)\n-\t\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t\t argv[0], NULL, argv[1], 0);\n-\t\telse\n-\t\t\treturn git_config_set_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], NULL);\n+\t\tif (argc == 2){\n+\t\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t\t argv[0], NULL, argv[1], 0);\n+\t\t\tif (use_system_config)\n+\t\t\t\tfree(given_config_source.file);\n+\t\t\treturn ret;\n+\t\t}\n+\t\telse{\n+\t\t\tret = git_config_set_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t argv[0], NULL);\n+\t\t\tif (use_system_config)\n+\t\t\t\tfree(given_config_source.file);\n+\t\t\treturn ret;\n+\t\t}\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], NULL, argv[1], 1);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t argv[0], NULL, argv[1], 1);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tret = git_config_rename_section_in_file(given_config_source.file,\n-\t\t\t\t\t\t\targv[0], argv[1]);\n+\t\t\t\t\t\t\t\t\t\t\t\targv[0], argv[1]);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t\tif (ret == 0)\n@@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 1, 1);\n \t\tret = git_config_rename_section_in_file(given_config_source.file,\n \t\t\t\t\t\t\targv[0], NULL);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t\tif (ret == 0)\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b3c818e..20ffbb1 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -326,7 +326,8 @@ static void setup_man_path(void)\n \t * old_path, the ':' at the end will let 'man' to try\n \t * system-wide paths after ours to find the manual page. If\n \t * there is old_path, we need ':' as delimiter. */\n-\tstrbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n+\tchar *git_man_path = system_path(GIT_MAN_PATH);\n+\tstrbuf_addstr(&new_path, git_man_path);\n \tstrbuf_addch(&new_path, ':');\n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n@@ -334,6 +335,7 @@ static void setup_man_path(void)\n \tsetenv(\"MANPATH\", new_path.buf, 1);\n\n \tstrbuf_release(&new_path);\n+\tfree(git_man_path);\n }\n\n static void exec_viewer(const char *name, const char *page)\n@@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd)\n static void show_info_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n-\tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n+\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\tsetenv(\"INFOPATH\", git_info_path, 1);\n+\tfree(git_info_path);\n \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n \tdie(_(\"no info viewer handled the request\"));\n }\n\n-static void get_html_page_path(struct strbuf *page_path, const char *page)\n+static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)\n {\n \tstruct stat st;\n-\tif (!html_path)\n-\t\thtml_path = system_path(GIT_HTML_PATH);\n\n \t/* Check that we have a git documentation directory. */\n \tif (!strstr(html_path, \"://\")) {\n \t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n-\t\t || !S_ISREG(st.st_mode))\n-\t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n+\t\t\t\t|| !S_ISREG(st.st_mode)){\n+\t\t\tprintf(\"'%s': not a documentation directory.\\n\", html_path);\n+\t\t\tfree(html_path);\n+\t\t\texit(1);\n+\t\t}\n \t}\n\n \tstrbuf_init(page_path, 0);\n@@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)\n * HTML.\n */\n #ifndef open_html\n-static void open_html(const char *path)\n+static void open_html(char *path)\n {\n \texecl_git_cmd(\"web--browse\", \"-c\", \"help.browser\", path, (char *)NULL);\n+\tfree(path);\n }\n #endif\n\n@@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n \tstruct strbuf page_path; /* it leaks but we exec bellow */\n+\tchar* html_path = NULL;\n+\n+\tif (!html_path)\n+\t\thtml_path = system_path(GIT_HTML_PATH);\n\n-\tget_html_page_path(&page_path, page);\n+\tget_html_page_path(&page_path, page, html_path);\n\n \topen_html(page_path.buf);\n }\n@@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n\n \treturn 0;\n }\n+\n+// git help --web commit\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 2619ca5..6f36f3f 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)\n \t\ttemplate_dir = init_db_template_dir;\n \tif (!template_dir)\n \t\ttemplate_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);\n-\tif (!template_dir[0])\n+\tif (!template_dir[0]){\n+\t\tif (!template_dir)\n+\t\t\tfree((char*)template_dir);\n \t\treturn;\n+\t}\n \ttemplate_len = strlen(template_dir);\n-\tif (PATH_MAX <= (template_len+strlen(\"/config\")))\n+\tif (PATH_MAX <= (template_len+strlen(\"/config\"))){\n+\t\tif (!template_dir)\n+\t\t\tfree((char*)template_dir);\n \t\tdie(_(\"insanely long template path %s\"), template_dir);\n+\t}\n \tstrcpy(template_path, template_dir);\n \tif (template_path[template_len-1] != '/') {\n \t\ttemplate_path[template_len++] = '/';\n@@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir)\n \tdir = opendir(template_path);\n \tif (!dir) {\n \t\twarning(_(\"templates not found %s\"), template_dir);\n+\t\tfree((char*)template_dir);\n \t\treturn;\n \t}\n\n@@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir)\n \t\t\t\"a wrong format version %d from '%s'\"),\n \t\t\trepository_format_version,\n \t\t\ttemplate_dir);\n+\t\tfree((char*)template_dir);\n \t\tclosedir(dir);\n \t\treturn;\n \t}\ndiff --git a/cache.h b/cache.h\nindex 1b05115..7048e7a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1354,7 +1354,7 @@ extern int update_server_info(int);\n\n struct git_config_source {\n \tunsigned int use_stdin:1;\n-\tconst char *file;\n+\tchar *file;\n \tconst char *blob;\n };\n\n@@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int\n extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\n extern int git_config_rename_section_in_file(const char *, const char *, const char *);\n-extern const char *git_etc_gitconfig(void);\n+extern char *git_etc_gitconfig(void);\n extern int check_repository_format_version(const char *var, const char *value, void *cb);\n extern int git_env_bool(const char *, int);\n extern unsigned long git_env_ulong(const char *, unsigned long);\ndiff --git a/config.c b/config.c\nindex ae1398f..b71c0fb 100644\n--- a/config.c\n+++ b/config.c\n@@ -122,7 +122,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t\tpath = buf.buf;\n \t}\n\n-\tif (!access_or_die(path, R_OK, 0)) {\n+\tif (!access_or_die(path, R_OK, 0, 0)) {\n \t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n \t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n \t\t\t cf && cf->name ? cf->name : \"the command line\");\n@@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn,\n \treturn git_config_from_blob_sha1(fn, name, sha1, data);\n }\n\n-const char *git_etc_gitconfig(void)\n+char *git_etc_gitconfig(void)\n {\n-\tstatic const char *system_wide;\n+\tstatic char *system_wide;\n \tif (!system_wide)\n \t\tsystem_wide = system_path(ETC_GITCONFIG);\n \treturn system_wide;\n@@ -1172,26 +1172,27 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \tint ret = 0, found = 0;\n \tchar *xdg_config = NULL;\n \tchar *user_config = NULL;\n+\tchar *git_etc_config = git_etc_gitconfig();\n\n \thome_config_paths(&user_config, &xdg_config, \"config\");\n\n-\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n-\t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n-\t\t\t\t\t data);\n+\tif (git_etc_config && !access_or_die(git_etc_config, R_OK, 0, 1)) {\n+\t\tret += git_config_from_file(fn, git_etc_config, data);\n \t\tfound += 1;\n+\t\tfree(git_etc_config);\n \t}\n\n-\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {\n+\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK, 0)) {\n \t\tret += git_config_from_file(fn, xdg_config, data);\n \t\tfound += 1;\n \t}\n\n-\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {\n+\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK, 0)) {\n \t\tret += git_config_from_file(fn, user_config, data);\n \t\tfound += 1;\n \t}\n\n-\tif (repo_config && !access_or_die(repo_config, R_OK, 0)) {\n+\tif (repo_config && !access_or_die(repo_config, R_OK, 0, 0)) {\n \t\tret += git_config_from_file(fn, repo_config, data);\n \t\tfound += 1;\n \t}\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 698e752..d35ecac 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -6,7 +6,7 @@\n static const char *argv_exec_path;\n static const char *argv0_path;\n\n-const char *system_path(const char *path)\n+char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n \tstatic const char *prefix;\n@@ -16,7 +16,7 @@ const char *system_path(const char *path)\n \tstruct strbuf d = STRBUF_INIT;\n\n \tif (is_absolute_path(path))\n-\t\treturn path;\n+\t\treturn strdup(path);\n\n #ifdef RUNTIME_PREFIX\n \tassert(argv0_path);\n@@ -34,8 +34,7 @@ const char *system_path(const char *path)\n #endif\n\n \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\treturn d.buf;\n }\n\n const char *git_extract_argv0_path(const char *argv0)\n@@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)\n\n\n /* Returns the highest-priority, location to look for git programs. */\n-const char *git_exec_path(void)\n+char *git_exec_path(void)\n {\n-\tconst char *env;\n+\tchar *env;\n\n \tif (argv_exec_path)\n-\t\treturn argv_exec_path;\n+\t\treturn strdup(argv_exec_path);\n\n \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n-\tif (env && *env) {\n-\t\treturn env;\n-\t}\n+\tif (env && *env)\n+\t\treturn strdup(env);\n\n \treturn system_path(GIT_EXEC_PATH);\n }\n@@ -96,7 +94,8 @@ void setup_path(void)\n \tconst char *old_path = getenv(\"PATH\");\n \tstruct strbuf new_path = STRBUF_INIT;\n\n-\tadd_path(&new_path, git_exec_path());\n+\tchar *exec_path = git_exec_path();\n+\tadd_path(&new_path, exec_path);\n \tadd_path(&new_path, argv0_path);\n\n \tif (old_path)\n@@ -107,6 +106,7 @@ void setup_path(void)\n \tsetenv(\"PATH\", new_path.buf, 1);\n\n \tstrbuf_release(&new_path);\n+\tfree(exec_path);\n }\n\n const char **prepare_git_cmd(const char **argv)\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex e4c9702..03c8599 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -3,12 +3,12 @@\n\n extern void git_set_argv_exec_path(const char *exec_path);\n extern const char *git_extract_argv0_path(const char *path);\n-extern const char *git_exec_path(void);\n+extern char *git_exec_path(void);\n extern void setup_path(void);\n extern const char **prepare_git_cmd(const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n LAST_ARG_MUST_BE_NULL\n extern int execl_git_cmd(const char *cmd, ...);\n-extern const char *system_path(const char *path);\n+extern char *system_path(const char *path);\n\n #endif /* GIT_EXEC_CMD_H */\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 400e921..fcc88db 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -813,7 +813,7 @@ int remove_or_warn(unsigned int mode, const char *path);\n */\n #define ACCESS_EACCES_OK (1U << 0)\n int access_or_warn(const char *path, int mode, unsigned flag);\n-int access_or_die(const char *path, int mode, unsigned flag);\n+int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config);\n\n /* Warn on an inaccessible file that ought to be accessible */\n void warn_on_inaccessible(const char *path);\ndiff --git a/git.c b/git.c\nindex 82d7a1c..d01c4f1 100644\n--- a/git.c\n+++ b/git.c\n@@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tif (*cmd == '=')\n \t\t\t\tgit_set_argv_exec_path(cmd + 1);\n \t\t\telse {\n-\t\t\t\tputs(git_exec_path());\n+\t\t\t\tchar *exec_path = git_exec_path();\n+\t\t\t\tputs(exec_path);\n+\t\t\t\tfree(exec_path);\n \t\t\t\texit(0);\n \t\t\t}\n \t\t} else if (!strcmp(cmd, \"--html-path\")) {\n-\t\t\tputs(system_path(GIT_HTML_PATH));\n+\t\t\tchar *git_html_path = system_path(GIT_HTML_PATH);\n+\t\t\tputs(git_html_path);\n+\t\t\tfree(git_html_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--man-path\")) {\n-\t\t\tputs(system_path(GIT_MAN_PATH));\n+\t\t\tchar *git_man_path = system_path(GIT_MAN_PATH);\n+\t\t\tputs(git_man_path);\n+\t\t\tfree(git_man_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--info-path\")) {\n-\t\t\tputs(system_path(GIT_INFO_PATH));\n+\t\t\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\t\t\tputs(git_info_path);\n+\t\t\tfree(git_info_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n \t\t\tuse_pager = 1;\ndiff --git a/wrapper.c b/wrapper.c\nindex 007ec0d..f97adbc 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -523,11 +523,14 @@ int access_or_warn(const char *path, int mode, unsigned flag)\n \treturn ret;\n }\n\n-int access_or_die(const char *path, int mode, unsigned flag)\n+int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config)\n {\n \tint ret = access(path, mode);\n-\tif (ret && !access_error_is_ok(errno, flag))\n+\tif (ret && !access_error_is_ok(errno, flag)){\n+\t\tif (etc_config)\n+\t\t\tfree((char*)path);\n \t\tdie_errno(_(\"unable to access '%s'\"), path);\n+\t}\n \treturn ret;\n }\n\n--\n2.2.0.rc3.191.g70a3888.dirty\n"},{"id":"252429","messageId":"1416838063-16797-1-git-send-email-kuleshovmail@gmail.com","threadId":"38028","inReplyTo":"87mw7gae8k.fsf@gmail.com","subject":"[PATCH] change contract between system_path and it's callers","fromName":"0xAX","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-24T14:07:43Z","receivedAt":"2014-11-24T14:07:43Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Now system_path returns path which is allocated string to callers;\nIt prevents memory leaks in some places. All callers of system_path\nare owners of path string and they must release it.\n\nSigned-off-by: Alexander Kuleshov \n---\n attr.c | 8 +++---\n builtin/config.c | 73 +++++++++++++++++++++++++++++++++++++++++--------------\n builtin/help.c | 30 ++++++++++++++++-------\n builtin/init-db.c | 12 +++++++--\n cache.h | 4 +--\n config.c | 19 ++++++++-------\n exec_cmd.c | 22 ++++++++---------\n exec_cmd.h | 4 +--\n git-compat-util.h | 2 +-\n git.c | 16 +++++++++---\n wrapper.c | 7 ++++--\n 11 files changed, 134 insertions(+), 63 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex cd54697..f96ddb4 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -462,9 +462,9 @@ static void drop_attr_stack(void)\n \t}\n }\n \n-static const char *git_etc_gitattributes(void)\n+static char *git_etc_gitattributes(void)\n {\n-\tstatic const char *system_wide;\n+\tstatic char *system_wide;\n \tif (!system_wide)\n \t\tsystem_wide = system_path(ETC_GITATTRIBUTES);\n \treturn system_wide;\n@@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)\n \tattr_stack = elem;\n \n \tif (git_attr_system()) {\n-\t\telem = read_attr_from_file(git_etc_gitattributes(), 1);\n+\t\tchar *etc_attributes = git_etc_gitattributes();\n+\t\telem = read_attr_from_file(etc_attributes, 1);\n+\t\tfree(etc_attributes);\n \t\tif (elem) {\n \t\t\telem->origin = NULL;\n \t\t\telem->prev = attr_stack;\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 15a7bea..266d42b 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (given_config_source.blob)\n \t\t\tdie(\"editing blobs is not supported\");\n \t\tgit_config(git_default_config, NULL);\n-\t\tconfig_file = xstrdup(given_config_source.file ?\n-\t\t\t\t given_config_source.file : git_path(\"config\"));\n+\n+\t\tif (use_system_config)\n+\t\t\tconfig_file = given_config_source.file ?\n+\t\t\t\t\t\t\tgiven_config_source.file : git_path(\"config\");\n+\t\telse\n+\t\t\tconfig_file = xstrdup(given_config_source.file ?\n+\t\t\t\t\t\t\t\t given_config_source.file : git_path(\"config\"));\n+\n \t\tif (use_global_config) {\n \t\t\tint fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \t\t\tif (fd) {\n@@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (ret == CONFIG_NOTHING_SET)\n \t\t\terror(\"cannot overwrite multiple values with a single value\\n\"\n \t\t\t\" Use a regexp, --add or --replace-all to change %s.\", argv[0]);\n+\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\treturn ret;\n \t}\n \telse if (actions == ACTION_SET_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value, argv[2], 0);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t argv[0], value, argv[2], 0);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_ADD) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value,\n-\t\t\t\t\t\t CONFIG_REGEX_NONE, 0);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file, argv[0],\n+\t\t\t\t\t\t\t\t\t\t\t value, CONFIG_REGEX_NONE, 0);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], value, argv[2], 1);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file, argv[0],\n+\t\t\t\t\t\t\t\t\t\t\t value, argv[2], 1);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_GET) {\n \t\tcheck_argc(argc, 1, 2);\n@@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\treturn get_urlmatch(argv[0], argv[1]);\n \t}\n \telse if (actions == ACTION_UNSET) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n-\t\tif (argc == 2)\n-\t\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t\t argv[0], NULL, argv[1], 0);\n-\t\telse\n-\t\t\treturn git_config_set_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], NULL);\n+\t\tif (argc == 2){\n+\t\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t\t argv[0], NULL, argv[1], 0);\n+\t\t\tif (use_system_config)\n+\t\t\t\tfree(given_config_source.file);\n+\t\t\treturn ret;\n+\t\t}\n+\t\telse{\n+\t\t\tret = git_config_set_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t argv[0], NULL);\n+\t\t\tif (use_system_config)\n+\t\t\t\tfree(given_config_source.file);\n+\t\t\treturn ret;\n+\t\t}\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n+\t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n-\t\treturn git_config_set_multivar_in_file(given_config_source.file,\n-\t\t\t\t\t\t argv[0], NULL, argv[1], 1);\n+\t\tret = git_config_set_multivar_in_file(given_config_source.file,\n+\t\t\t\t\t\t\t\t\t\t\t argv[0], NULL, argv[1], 1);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n+\t\treturn ret;\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tint ret;\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tret = git_config_rename_section_in_file(given_config_source.file,\n-\t\t\t\t\t\t\targv[0], argv[1]);\n+\t\t\t\t\t\t\t\t\t\t\t\targv[0], argv[1]);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t\tif (ret == 0)\n@@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_argc(argc, 1, 1);\n \t\tret = git_config_rename_section_in_file(given_config_source.file,\n \t\t\t\t\t\t\targv[0], NULL);\n+\t\tif (use_system_config)\n+\t\t\tfree(given_config_source.file);\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t\tif (ret == 0)\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b3c818e..20ffbb1 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -326,7 +326,8 @@ static void setup_man_path(void)\n \t * old_path, the ':' at the end will let 'man' to try\n \t * system-wide paths after ours to find the manual page. If\n \t * there is old_path, we need ':' as delimiter. */\n-\tstrbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n+\tchar *git_man_path = system_path(GIT_MAN_PATH);\n+\tstrbuf_addstr(&new_path, git_man_path);\n \tstrbuf_addch(&new_path, ':');\n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n@@ -334,6 +335,7 @@ static void setup_man_path(void)\n \tsetenv(\"MANPATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n+\tfree(git_man_path);\n }\n \n static void exec_viewer(const char *name, const char *page)\n@@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd)\n static void show_info_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n-\tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n+\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\tsetenv(\"INFOPATH\", git_info_path, 1);\n+\tfree(git_info_path);\n \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n \tdie(_(\"no info viewer handled the request\"));\n }\n \n-static void get_html_page_path(struct strbuf *page_path, const char *page)\n+static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)\n {\n \tstruct stat st;\n-\tif (!html_path)\n-\t\thtml_path = system_path(GIT_HTML_PATH);\n \n \t/* Check that we have a git documentation directory. */\n \tif (!strstr(html_path, \"://\")) {\n \t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n-\t\t || !S_ISREG(st.st_mode))\n-\t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n+\t\t\t\t|| !S_ISREG(st.st_mode)){\n+\t\t\tprintf(\"'%s': not a documentation directory.\\n\", html_path);\n+\t\t\tfree(html_path);\n+\t\t\texit(1);\n+\t\t}\n \t}\n \n \tstrbuf_init(page_path, 0);\n@@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)\n * HTML.\n */\n #ifndef open_html\n-static void open_html(const char *path)\n+static void open_html(char *path)\n {\n \texecl_git_cmd(\"web--browse\", \"-c\", \"help.browser\", path, (char *)NULL);\n+\tfree(path);\n }\n #endif\n \n@@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n \tstruct strbuf page_path; /* it leaks but we exec bellow */\n+\tchar* html_path = NULL;\n+\n+\tif (!html_path)\n+\t\thtml_path = system_path(GIT_HTML_PATH);\n \n-\tget_html_page_path(&page_path, page);\n+\tget_html_page_path(&page_path, page, html_path);\n \n \topen_html(page_path.buf);\n }\n@@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \n \treturn 0;\n }\n+\n+// git help --web commit\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 2619ca5..6f36f3f 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)\n \t\ttemplate_dir = init_db_template_dir;\n \tif (!template_dir)\n \t\ttemplate_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);\n-\tif (!template_dir[0])\n+\tif (!template_dir[0]){\n+\t\tif (!template_dir)\n+\t\t\tfree((char*)template_dir);\n \t\treturn;\n+\t}\n \ttemplate_len = strlen(template_dir);\n-\tif (PATH_MAX <= (template_len+strlen(\"/config\")))\n+\tif (PATH_MAX <= (template_len+strlen(\"/config\"))){\n+\t\tif (!template_dir)\n+\t\t\tfree((char*)template_dir);\n \t\tdie(_(\"insanely long template path %s\"), template_dir);\n+\t}\n \tstrcpy(template_path, template_dir);\n \tif (template_path[template_len-1] != '/') {\n \t\ttemplate_path[template_len++] = '/';\n@@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir)\n \tdir = opendir(template_path);\n \tif (!dir) {\n \t\twarning(_(\"templates not found %s\"), template_dir);\n+\t\tfree((char*)template_dir);\n \t\treturn;\n \t}\n \n@@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir)\n \t\t\t\"a wrong format version %d from '%s'\"),\n \t\t\trepository_format_version,\n \t\t\ttemplate_dir);\n+\t\tfree((char*)template_dir);\n \t\tclosedir(dir);\n \t\treturn;\n \t}\ndiff --git a/cache.h b/cache.h\nindex 1b05115..7048e7a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1354,7 +1354,7 @@ extern int update_server_info(int);\n \n struct git_config_source {\n \tunsigned int use_stdin:1;\n-\tconst char *file;\n+\tchar *file;\n \tconst char *blob;\n };\n \n@@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int\n extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\n extern int git_config_rename_section_in_file(const char *, const char *, const char *);\n-extern const char *git_etc_gitconfig(void);\n+extern char *git_etc_gitconfig(void);\n extern int check_repository_format_version(const char *var, const char *value, void *cb);\n extern int git_env_bool(const char *, int);\n extern unsigned long git_env_ulong(const char *, unsigned long);\ndiff --git a/config.c b/config.c\nindex ae1398f..b71c0fb 100644\n--- a/config.c\n+++ b/config.c\n@@ -122,7 +122,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t\tpath = buf.buf;\n \t}\n \n-\tif (!access_or_die(path, R_OK, 0)) {\n+\tif (!access_or_die(path, R_OK, 0, 0)) {\n \t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n \t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n \t\t\t cf && cf->name ? cf->name : \"the command line\");\n@@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn,\n \treturn git_config_from_blob_sha1(fn, name, sha1, data);\n }\n \n-const char *git_etc_gitconfig(void)\n+char *git_etc_gitconfig(void)\n {\n-\tstatic const char *system_wide;\n+\tstatic char *system_wide;\n \tif (!system_wide)\n \t\tsystem_wide = system_path(ETC_GITCONFIG);\n \treturn system_wide;\n@@ -1172,26 +1172,27 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \tint ret = 0, found = 0;\n \tchar *xdg_config = NULL;\n \tchar *user_config = NULL;\n+\tchar *git_etc_config = git_etc_gitconfig();\n \n \thome_config_paths(&user_config, &xdg_config, \"config\");\n \n-\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n-\t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n-\t\t\t\t\t data);\n+\tif (git_etc_config && !access_or_die(git_etc_config, R_OK, 0, 1)) {\n+\t\tret += git_config_from_file(fn, git_etc_config, data);\n \t\tfound += 1;\n+\t\tfree(git_etc_config);\n \t}\n \n-\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {\n+\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK, 0)) {\n \t\tret += git_config_from_file(fn, xdg_config, data);\n \t\tfound += 1;\n \t}\n \n-\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {\n+\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK, 0)) {\n \t\tret += git_config_from_file(fn, user_config, data);\n \t\tfound += 1;\n \t}\n \n-\tif (repo_config && !access_or_die(repo_config, R_OK, 0)) {\n+\tif (repo_config && !access_or_die(repo_config, R_OK, 0, 0)) {\n \t\tret += git_config_from_file(fn, repo_config, data);\n \t\tfound += 1;\n \t}\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 698e752..d35ecac 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -6,7 +6,7 @@\n static const char *argv_exec_path;\n static const char *argv0_path;\n \n-const char *system_path(const char *path)\n+char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n \tstatic const char *prefix;\n@@ -16,7 +16,7 @@ const char *system_path(const char *path)\n \tstruct strbuf d = STRBUF_INIT;\n \n \tif (is_absolute_path(path))\n-\t\treturn path;\n+\t\treturn strdup(path);\n \n #ifdef RUNTIME_PREFIX\n \tassert(argv0_path);\n@@ -34,8 +34,7 @@ const char *system_path(const char *path)\n #endif\n \n \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\treturn d.buf;\n }\n \n const char *git_extract_argv0_path(const char *argv0)\n@@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)\n \n \n /* Returns the highest-priority, location to look for git programs. */\n-const char *git_exec_path(void)\n+char *git_exec_path(void)\n {\n-\tconst char *env;\n+\tchar *env;\n \n \tif (argv_exec_path)\n-\t\treturn argv_exec_path;\n+\t\treturn strdup(argv_exec_path);\n \n \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n-\tif (env && *env) {\n-\t\treturn env;\n-\t}\n+\tif (env && *env)\n+\t\treturn strdup(env);\n \n \treturn system_path(GIT_EXEC_PATH);\n }\n@@ -96,7 +94,8 @@ void setup_path(void)\n \tconst char *old_path = getenv(\"PATH\");\n \tstruct strbuf new_path = STRBUF_INIT;\n \n-\tadd_path(&new_path, git_exec_path());\n+\tchar *exec_path = git_exec_path();\n+\tadd_path(&new_path, exec_path);\n \tadd_path(&new_path, argv0_path);\n \n \tif (old_path)\n@@ -107,6 +106,7 @@ void setup_path(void)\n \tsetenv(\"PATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n+\tfree(exec_path);\n }\n \n const char **prepare_git_cmd(const char **argv)\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex e4c9702..03c8599 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -3,12 +3,12 @@\n \n extern void git_set_argv_exec_path(const char *exec_path);\n extern const char *git_extract_argv0_path(const char *path);\n-extern const char *git_exec_path(void);\n+extern char *git_exec_path(void);\n extern void setup_path(void);\n extern const char **prepare_git_cmd(const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n LAST_ARG_MUST_BE_NULL\n extern int execl_git_cmd(const char *cmd, ...);\n-extern const char *system_path(const char *path);\n+extern char *system_path(const char *path);\n \n #endif /* GIT_EXEC_CMD_H */\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 400e921..fcc88db 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -813,7 +813,7 @@ int remove_or_warn(unsigned int mode, const char *path);\n */\n #define ACCESS_EACCES_OK (1U << 0)\n int access_or_warn(const char *path, int mode, unsigned flag);\n-int access_or_die(const char *path, int mode, unsigned flag);\n+int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config);\n \n /* Warn on an inaccessible file that ought to be accessible */\n void warn_on_inaccessible(const char *path);\ndiff --git a/git.c b/git.c\nindex 82d7a1c..d01c4f1 100644\n--- a/git.c\n+++ b/git.c\n@@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tif (*cmd == '=')\n \t\t\t\tgit_set_argv_exec_path(cmd + 1);\n \t\t\telse {\n-\t\t\t\tputs(git_exec_path());\n+\t\t\t\tchar *exec_path = git_exec_path();\n+\t\t\t\tputs(exec_path);\n+\t\t\t\tfree(exec_path);\n \t\t\t\texit(0);\n \t\t\t}\n \t\t} else if (!strcmp(cmd, \"--html-path\")) {\n-\t\t\tputs(system_path(GIT_HTML_PATH));\n+\t\t\tchar *git_html_path = system_path(GIT_HTML_PATH);\n+\t\t\tputs(git_html_path);\n+\t\t\tfree(git_html_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--man-path\")) {\n-\t\t\tputs(system_path(GIT_MAN_PATH));\n+\t\t\tchar *git_man_path = system_path(GIT_MAN_PATH);\n+\t\t\tputs(git_man_path);\n+\t\t\tfree(git_man_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--info-path\")) {\n-\t\t\tputs(system_path(GIT_INFO_PATH));\n+\t\t\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\t\t\tputs(git_info_path);\n+\t\t\tfree(git_info_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n \t\t\tuse_pager = 1;\ndiff --git a/wrapper.c b/wrapper.c\nindex 007ec0d..f97adbc 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -523,11 +523,14 @@ int access_or_warn(const char *path, int mode, unsigned flag)\n \treturn ret;\n }\n \n-int access_or_die(const char *path, int mode, unsigned flag)\n+int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config)\n {\n \tint ret = access(path, mode);\n-\tif (ret && !access_error_is_ok(errno, flag))\n+\tif (ret && !access_error_is_ok(errno, flag)){\n+\t\tif (etc_config)\n+\t\t\tfree((char*)path);\n \t\tdie_errno(_(\"unable to access '%s'\"), path);\n+\t}\n \treturn ret;\n }\n \n-- \n2.2.0.rc3.191.g70a3888.dirty\n"},{"id":"252441","messageId":"xmqqoarwwfz1.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"1416838063-16797-1-git-send-email-kuleshovmail@gmail.com","subject":"Re*: [PATCH] change contract between system_path and it's callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-24T19:33:54Z","receivedAt":"2014-11-24T19:33:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"0xAX writes:\n\n> Now system_path returns path which is allocated string to callers;\n> It prevents memory leaks in some places. All callers of system_path\n> are owners of path string and they must release it.\n>\n> Signed-off-by: Alexander Kuleshov \n> ---\n\n> -static const char *git_etc_gitattributes(void)\n> +static char *git_etc_gitattributes(void)\n\nHmph, I think this should keep returning \"const char *\", as the\ncaller is not expected to free the pointer or write into the memory\nheld by the returned string. The same applies to the change of the\ntype in \"struct git_config_source\".\n\nThe change in the semantics of system_path() made the \"get once and\nkeep returning it\" this function does safer and correct, but this\nfunction itself did not change any behaviour from its caller's point\nof view.\n\n> {\n> -\tstatic const char *system_wide;\n> +\tstatic char *system_wide;\n> \tif (!system_wide)\n> \t\tsystem_wide = system_path(ETC_GITATTRIBUTES);\n> \treturn system_wide;\n\n\n> @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)\n> \tattr_stack = elem;\n> \n> \tif (git_attr_system()) {\n> -\t\telem = read_attr_from_file(git_etc_gitattributes(), 1);\n> +\t\tchar *etc_attributes = git_etc_gitattributes();\n> +\t\telem = read_attr_from_file(etc_attributes, 1);\n> +\t\tfree(etc_attributes);\n\nAnd freeing here is actively wrong, I think. You are freeing the\npiece of memory still pointed by \"static char *system_wide\" in the\nfunction git_etc_gitattributes(); when it is called again, the\ncaller will get a pointer into the memory you have freed here.\n\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 15a7bea..266d42b 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> \t\tif (given_config_source.blob)\n> \t\t\tdie(\"editing blobs is not supported\");\n> \t\tgit_config(git_default_config, NULL);\n> -\t\tconfig_file = xstrdup(given_config_source.file ?\n> -\t\t\t\t given_config_source.file : git_path(\"config\"));\n> +\n> +\t\tif (use_system_config)\n> +\t\t\tconfig_file = given_config_source.file ?\n> +\t\t\t\t\t\t\tgiven_config_source.file : git_path(\"config\");\n> +\t\telse\n> +\t\t\tconfig_file = xstrdup(given_config_source.file ?\n> +\t\t\t\t\t\t\t\t given_config_source.file : git_path(\"config\"));\n> +\n\nSorry, but I do not understand this change.\n\nWhy do you need \"if use-system-config, do not allocate\" here and\nthen later everywhere \"if use-system-config, free it\"? I would\nunderstand if the change were \"earlier we had mixture of borrowed\nand owned strings, but we make sure we always own the string by\nrunning xstrdup() in the caller when we used to let this function\nborrow, so that we can always free() before returning from here\",\nor something.\n\nFor example, in the original code, use_local_config codepath uses\ngit_pathdup(), which is an allocated piece of memory, to initialize\ngiven_config_source.file. Doesn't it need to be freed?\n\nEven if it is not in the use_system_config mode, if\ngiven_config_source.file is not an absolute path, we store a copy of\nprefix-filename result to given_config_source.file. Doesn't it need\nto be freed?\n\nI think the implementation strategy you took makes the result\nunnecessarily messy and error prone. Let's step back a bit.\n\nWhen you have code that sometimes borrows memory and sometimes owns\nmemory, writing the clean-up part like this is error prone:\n\n\tchar *var;\n\n\tif (A)\n\t\tvar = borrowed memory;\n\telse if (B)\n\t\tvar = borrowed memory;\n\telse if (C)\n\t\tvar = xstrdup(borrowed memory);\n\telse\t\n\t\tvar = borrowed memory;\n\n\tuse var; /* a loong piece of code in reality */\n\n\tif (C)\n \tfree(var);\n\nbecause the set-up part can and will change over time as the code\nevolves. If written this way:\n\n\tchar *var, *to_free = NULL;\n\n\tif (A)\n\t\tvar = borrowed memory;\n\telse if (B)\n\t\tvar = borrowed memory;\n\telse if (C)\n\t\tto_free = var = xstrdup(borrowed memory);\n\telse\t\n\t\tvar = borrowed memory;\n\n\tuse var; /* a loong piece of code in reality */\n\n\tfree(to_free);\n\nthe clean-up part would not have to worry how the set-up part\ndecided when to borrow memory and when to own memory. Another way\nto do so would obviously be:\n\n\tchar *var;\n\n\tif (A)\n\t\tvar = xstrdup(borrowed memory);\n\telse if (B)\n\t\tvar = xstrdup(borrowed memory);\n\telse if (C)\n\t\tvar = xstrdup(borrowed memory);\n\telse\t\n\t\tvar = xstrdup(borrowed memory);\n\n\tuse var; /* a loong piece of code in reality */\n\n \tfree(var);\n\nto always own the memory, but depending on the original code, it may\nmake the result more inefficient and the patch more noisy.\n\nThis is especially true when the clean-up part is spread all over\nthe place like your patch does, i.e. the \"use var\" part of the code\nlooks like\n\n\tif (X) {\n \t... a looong piece of code to use var ...\n\t if (C)\n \tfree(var);\n\t} else if (Y) {\n \t... a looong piece of code to use var ...\n\t if (C)\n \tfree(var);\n } else {\n \t... a looong piece of code to use var ...\n\t if (C)\n \tfree(var);\n\t}\n\nwhich is simply unmaintainable. Better to make \"use var\" and the\nclean-up part structured more like this, using the to_free trick:\n\n\tif (X) {\n \t... a looong piece of code to use var ...\n\t\tgoto cleanup;\n\t} else if (Y) {\n \t... a looong piece of code to use var ...\n\t\tgoto cleanup;\n } else {\n \t... a looong piece of code to use var ...\n\t\tgoto cleanup;\n\t}\n\n\tcleanup:\n\t\tfree(to_free);\n\n> diff --git a/builtin/help.c b/builtin/help.c\n> index b3c818e..20ffbb1 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -326,7 +326,8 @@ static void setup_man_path(void)\n> \t * old_path, the ':' at the end will let 'man' to try\n> \t * system-wide paths after ours to find the manual page. If\n> \t * there is old_path, we need ':' as delimiter. */\n> -\tstrbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n> +\tchar *git_man_path = system_path(GIT_MAN_PATH);\n\nDefile variable in the definition block above to avoid future\ndecl-after-stmt, i.e.\n\n\t const char *old_path = getenv(\"MANPATH\");\n\t+char *git_man_path = system_path(GIT_MAN_PATH);\n\n\t /* We should always put ...\n\n> +\tstrbuf_addstr(&new_path, git_man_path);\n\nAnd better free it immediately after you are done, instead of at the\nend of the function.\n\n> \tstrbuf_addch(&new_path, ':');\n> \tif (old_path)\n> \t\tstrbuf_addstr(&new_path, old_path);\n> @@ -334,6 +335,7 @@ static void setup_man_path(void)\n> \tsetenv(\"MANPATH\", new_path.buf, 1);\n> \n> \tstrbuf_release(&new_path);\n> +\tfree(git_man_path);\n> }\n\n> -static void get_html_page_path(struct strbuf *page_path, const char *page)\n> +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)\n> {\n> \tstruct stat st;\n> -\tif (!html_path)\n> -\t\thtml_path = system_path(GIT_HTML_PATH);\n> \n> \t/* Check that we have a git documentation directory. */\n> \tif (!strstr(html_path, \"://\")) {\n> \t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n> -\t\t || !S_ISREG(st.st_mode))\n> -\t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n> +\t\t\t\t|| !S_ISREG(st.st_mode)){\n> +\t\t\tprintf(\"'%s': not a documentation directory.\\n\", html_path);\n> +\t\t\tfree(html_path);\n> +\t\t\texit(1);\n\nThis is an unnecessary churn, and an unwarranted change to the\nexternally visible behaviour. die() gives its message to the\nstandard error stream (not to the standard output stream), and exits\nwith 128 (not with 1).\n\nWe are exiting immediately after; it is not worth changing the API\nto the caller or external behaviour, only to free(html_path)\nconditionally.\n\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 2619ca5..6f36f3f 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)\n> \t\ttemplate_dir = init_db_template_dir;\n> \tif (!template_dir)\n> \t\ttemplate_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);\n> -\tif (!template_dir[0])\n> +\tif (!template_dir[0]){\n\nA SP between ) and {\n\n> +\t\tif (!template_dir)\n> +\t\t\tfree((char*)template_dir);\n\nNo need for \"call free only if it is not NULL\"; free(NULL) works\njust fine.\n\n> \t\treturn;\n> +\t}\n> \ttemplate_len = strlen(template_dir);\n> -\tif (PATH_MAX <= (template_len+strlen(\"/config\")))\n> +\tif (PATH_MAX <= (template_len+strlen(\"/config\"))){\n> +\t\tif (!template_dir)\n> +\t\t\tfree((char*)template_dir);\n> \t\tdie(_(\"insanely long template path %s\"), template_dir);\n\nYou are freeing and then using it in the message, and it cannot be\nNULL anyway so \"if (!template_dir)\" is doubly unnecessary.\n\nI wouldn't bother freeing immediately before dying, exiting, or\nexecing.\n\nHere is my quick attempt, starting solely from the output of \"git\ngrep system_path\". How does it look?\n\nOne thing to note is that this illustration does not consider memory\npointed at by the \"system_wide\" variable here (from attr.c)\n\n static const char *git_etc_gitattributes(void)\n {\n static const char *system_wide;\n if (!system_wide)\n system_wide = system_path(ETC_GITATTRIBUTES);\n return system_wide;\n }\n\nat the point of process exit as a \"leak\".\n\n-- >8 --\nSubject: system_path(): always return free'able memory to the caller\n\nThe function sometimes returns a newly allocated string and\nsometimes returns a borrowed string, the latter of which the callers\nmust not free(). The existing callers all assume that the return\nvalue belongs to the callee and most of them copy it with strdup()\nwhen they want to keep it around. They end up leaking the returned\ncopy when the callee returned a new string because they cannot tell\nif they should free it.\n\nChange the contract between the callers and system_path() to make\nthe returned string owned by the callers; they are responsible for\nfreeing it when done, but they do not have to make their own copy to\nstore it away.\n\nAdjust the callers to make sure they do not leak the returned string\nonce they are done, but do not bother freeing it just before dying,\nexiting or exec'ing other program to avoid unnecessary churn.\n\nReported-by: Alexander Kuleshov \nSigned-off-by: Junio C Hamano \n---\n builtin/help.c | 9 +++++++--\n builtin/init-db.c | 15 ++++++++++-----\n exec_cmd.c | 7 +++----\n exec_cmd.h | 2 +-\n 4 files changed, 21 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b3c818e..e78c135 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -321,16 +321,18 @@ static void setup_man_path(void)\n {\n \tstruct strbuf new_path = STRBUF_INIT;\n \tconst char *old_path = getenv(\"MANPATH\");\n+\tchar *git_man_path = system_path(GIT_MAN_PATH);\n \n \t/* We should always put ':' after our path. If there is no\n \t * old_path, the ':' at the end will let 'man' to try\n \t * system-wide paths after ours to find the manual page. If\n \t * there is old_path, we need ':' as delimiter. */\n-\tstrbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n+\tstrbuf_addstr(&new_path, git_man_path);\n \tstrbuf_addch(&new_path, ':');\n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n \n+\tfree(git_man_path);\n \tsetenv(\"MANPATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n@@ -380,8 +382,10 @@ static void show_info_page(const char *git_cmd)\n static void get_html_page_path(struct strbuf *page_path, const char *page)\n {\n \tstruct stat st;\n+\tchar *to_free = NULL;\n+\n \tif (!html_path)\n-\t\thtml_path = system_path(GIT_HTML_PATH);\n+\t\thtml_path = to_free = system_path(GIT_HTML_PATH);\n \n \t/* Check that we have a git documentation directory. */\n \tif (!strstr(html_path, \"://\")) {\n@@ -392,6 +396,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)\n \n \tstrbuf_init(page_path, 0);\n \tstrbuf_addf(page_path, \"%s/%s.html\", html_path, page);\n+\tfree(to_free);\n }\n \n /*\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 2619ca5..9966522 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -119,15 +119,18 @@ static void copy_templates(const char *template_dir)\n \tDIR *dir;\n \tconst char *git_dir = get_git_dir();\n \tint len = strlen(git_dir);\n+\tchar *to_free = NULL;\n \n \tif (!template_dir)\n \t\ttemplate_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);\n \tif (!template_dir)\n \t\ttemplate_dir = init_db_template_dir;\n \tif (!template_dir)\n-\t\ttemplate_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);\n-\tif (!template_dir[0])\n+\t\ttemplate_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);\n+\tif (!template_dir[0]) {\n+\t\tfree(to_free);\n \t\treturn;\n+\t}\n \ttemplate_len = strlen(template_dir);\n \tif (PATH_MAX <= (template_len+strlen(\"/config\")))\n \t\tdie(_(\"insanely long template path %s\"), template_dir);\n@@ -139,7 +142,7 @@ static void copy_templates(const char *template_dir)\n \tdir = opendir(template_path);\n \tif (!dir) {\n \t\twarning(_(\"templates not found %s\"), template_dir);\n-\t\treturn;\n+\t\tgoto free_return;\n \t}\n \n \t/* Make sure that template is from the correct vintage */\n@@ -155,8 +158,7 @@ static void copy_templates(const char *template_dir)\n \t\t\t\"a wrong format version %d from '%s'\"),\n \t\t\trepository_format_version,\n \t\t\ttemplate_dir);\n-\t\tclosedir(dir);\n-\t\treturn;\n+\t\tgoto close_free_return;\n \t}\n \n \tmemcpy(path, git_dir, len);\n@@ -166,7 +168,10 @@ static void copy_templates(const char *template_dir)\n \tcopy_templates_1(path, len,\n \t\t\t template_path, template_len,\n \t\t\t dir);\n+close_free_return:\n \tclosedir(dir);\n+free_return:\n+\tfree(to_free);\n }\n \n static int git_init_db_config(const char *k, const char *v, void *cb)\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 698e752..8ab37b5 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -6,7 +6,7 @@\n static const char *argv_exec_path;\n static const char *argv0_path;\n \n-const char *system_path(const char *path)\n+char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n \tstatic const char *prefix;\n@@ -16,7 +16,7 @@ const char *system_path(const char *path)\n \tstruct strbuf d = STRBUF_INIT;\n \n \tif (is_absolute_path(path))\n-\t\treturn path;\n+\t\treturn xstrdup(path);\n \n #ifdef RUNTIME_PREFIX\n \tassert(argv0_path);\n@@ -34,8 +34,7 @@ const char *system_path(const char *path)\n #endif\n \n \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\treturn strbuf_detach(&d, NULL);\n }\n \n const char *git_extract_argv0_path(const char *argv0)\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex e4c9702..93b0c02 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -9,6 +9,6 @@ extern const char **prepare_git_cmd(const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n LAST_ARG_MUST_BE_NULL\n extern int execl_git_cmd(const char *cmd, ...);\n-extern const char *system_path(const char *path);\n+extern char *system_path(const char *path);\n \n #endif /* GIT_EXEC_CMD_H */\n"},{"id":"252444","messageId":"87ppcc4b2g.fsf@gmail.com","threadId":"38028","inReplyTo":"xmqqoarwwfz1.fsf@gitster.dls.corp.google.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Alex Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-24T19:53:35Z","receivedAt":"2014-11-24T19:53:35Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"\nJunio C Hamano @ 2014-11-25 01:33 ALMT:\n\n>> -static const char *git_etc_gitattributes(void)\n>> +static char *git_etc_gitattributes(void)\n>\n> Hmph, I think this should keep returning \"const char *\", as the\n> caller is not expected to free the pointer or write into the memory\n> held by the returned string. The same applies to the change of the\n> type in \"struct git_config_source\".\n>\n> The change in the semantics of system_path() made the \"get once and\n> keep returning it\" this function does safer and correct, but this\n> function itself did not change any behaviour from its caller's point\n> of view.\n\nUnderstand, will fix it.\n\n>\n>> {\n>> -\tstatic const char *system_wide;\n>> +\tstatic char *system_wide;\n>> \tif (!system_wide)\n>> \t\tsystem_wide = system_path(ETC_GITATTRIBUTES);\n>> \treturn system_wide;\n>\n>\n>> @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void)\n>> \tattr_stack = elem;\n>>\n>> \tif (git_attr_system()) {\n>> -\t\telem = read_attr_from_file(git_etc_gitattributes(), 1);\n>> +\t\tchar *etc_attributes = git_etc_gitattributes();\n>> +\t\telem = read_attr_from_file(etc_attributes, 1);\n>> +\t\tfree(etc_attributes);\n>\n> And freeing here is actively wrong, I think. You are freeing the\n> piece of memory still pointed by \"static char *system_wide\" in the\n> function git_etc_gitattributes(); when it is called again, the\n> caller will get a pointer into the memory you have freed here.\n\nWhy? If i understand correctly we don't use etc_attributes anymore in\nthis function and if we'll call this function again\ngit_etc_gitattributes will create new pointer and system_path alloc\nmemory for it or i'm wrong with it?\n\n>\n>> diff --git a/builtin/config.c b/builtin/config.c\n>> index 15a7bea..266d42b 100644\n>> --- a/builtin/config.c\n>> +++ b/builtin/config.c\n>> @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>> \t\tif (given_config_source.blob)\n>> \t\t\tdie(\"editing blobs is not supported\");\n>> \t\tgit_config(git_default_config, NULL);\n>> -\t\tconfig_file = xstrdup(given_config_source.file ?\n>> -\t\t\t\t given_config_source.file : git_path(\"config\"));\n>> +\n>> +\t\tif (use_system_config)\n>> +\t\t\tconfig_file = given_config_source.file ?\n>> +\t\t\t\t\t\t\tgiven_config_source.file : git_path(\"config\");\n>> +\t\telse\n>> +\t\t\tconfig_file = xstrdup(given_config_source.file ?\n>> +\t\t\t\t\t\t\t\t given_config_source.file : git_path(\"config\"));\n>> +\n>\n> Sorry, but I do not understand this change.\n>\n> Why do you need \"if use-system-config, do not allocate\" here and\n> then later everywhere \"if use-system-config, free it\"? I would\n> understand if the change were \"earlier we had mixture of borrowed\n> and owned strings, but we make sure we always own the string by\n> running xstrdup() in the caller when we used to let this function\n> borrow, so that we can always free() before returning from here\",\n> or something.\n>\n> For example, in the original code, use_local_config codepath uses\n> git_pathdup(), which is an allocated piece of memory, to initialize\n> given_config_source.file. Doesn't it need to be freed?\n>\n> Even if it is not in the use_system_config mode, if\n> given_config_source.file is not an absolute path, we store a copy of\n> prefix-filename result to given_config_source.file. Doesn't it need\n> to be freed?\n>\n> I think the implementation strategy you took makes the result\n> unnecessarily messy and error prone. Let's step back a bit.\n>\n> When you have code that sometimes borrows memory and sometimes owns\n> memory, writing the clean-up part like this is error prone:\n>\n> \tchar *var;\n>\n> \tif (A)\n> \t\tvar = borrowed memory;\n> \telse if (B)\n> \t\tvar = borrowed memory;\n> \telse if (C)\n> \t\tvar = xstrdup(borrowed memory);\n> \telse\n> \t\tvar = borrowed memory;\n>\n> \tuse var; /* a loong piece of code in reality */\n>\n> \tif (C)\n> \tfree(var);\n>\n> because the set-up part can and will change over time as the code\n> evolves. If written this way:\n>\n> \tchar *var, *to_free = NULL;\n>\n> \tif (A)\n> \t\tvar = borrowed memory;\n> \telse if (B)\n> \t\tvar = borrowed memory;\n> \telse if (C)\n> \t\tto_free = var = xstrdup(borrowed memory);\n> \telse\n> \t\tvar = borrowed memory;\n>\n> \tuse var; /* a loong piece of code in reality */\n>\n> \tfree(to_free);\n>\n> the clean-up part would not have to worry how the set-up part\n> decided when to borrow memory and when to own memory. Another way\n> to do so would obviously be:\n>\n> \tchar *var;\n>\n> \tif (A)\n> \t\tvar = xstrdup(borrowed memory);\n> \telse if (B)\n> \t\tvar = xstrdup(borrowed memory);\n> \telse if (C)\n> \t\tvar = xstrdup(borrowed memory);\n> \telse\n> \t\tvar = xstrdup(borrowed memory);\n>\n> \tuse var; /* a loong piece of code in reality */\n>\n> \tfree(var);\n>\n> to always own the memory, but depending on the original code, it may\n> make the result more inefficient and the patch more noisy.\n>\n> This is especially true when the clean-up part is spread all over\n> the place like your patch does, i.e. the \"use var\" part of the code\n> looks like\n>\n> \tif (X) {\n> \t... a looong piece of code to use var ...\n> \t if (C)\n> \tfree(var);\n> \t} else if (Y) {\n> \t... a looong piece of code to use var ...\n> \t if (C)\n> \tfree(var);\n> } else {\n> \t... a looong piece of code to use var ...\n> \t if (C)\n> \tfree(var);\n> \t}\n>\n> which is simply unmaintainable. Better to make \"use var\" and the\n> clean-up part structured more like this, using the to_free trick:\n>\n> \tif (X) {\n> \t... a looong piece of code to use var ...\n> \t\tgoto cleanup;\n> \t} else if (Y) {\n> \t... a looong piece of code to use var ...\n> \t\tgoto cleanup;\n> } else {\n> \t... a looong piece of code to use var ...\n> \t\tgoto cleanup;\n> \t}\n>\n> \tcleanup:\n> \t\tfree(to_free);\n>\n\nWill look again on it tomorrow and hope send new patch (sorry deep night\ntoday here), anyway thanks for great explanation.\n\n>> diff --git a/builtin/help.c b/builtin/help.c\n>> index b3c818e..20ffbb1 100644\n>> --- a/builtin/help.c\n>> +++ b/builtin/help.c\n>> @@ -326,7 +326,8 @@ static void setup_man_path(void)\n>> \t * old_path, the ':' at the end will let 'man' to try\n>> \t * system-wide paths after ours to find the manual page. If\n>> \t * there is old_path, we need ':' as delimiter. */\n>> -\tstrbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n>> +\tchar *git_man_path = system_path(GIT_MAN_PATH);\n>\n> Defile variable in the definition block above to avoid future\n> decl-after-stmt, i.e.\n>\n> \t const char *old_path = getenv(\"MANPATH\");\n> \t+char *git_man_path = system_path(GIT_MAN_PATH);\n>\n> \t /* We should always put ...\n>\n>> +\tstrbuf_addstr(&new_path, git_man_path);\n>\n> And better free it immediately after you are done, instead of at the\n> end of the function.\n\nWill fix it.\n\n>\n>> \tstrbuf_addch(&new_path, ':');\n>> \tif (old_path)\n>> \t\tstrbuf_addstr(&new_path, old_path);\n>> @@ -334,6 +335,7 @@ static void setup_man_path(void)\n>> \tsetenv(\"MANPATH\", new_path.buf, 1);\n>>\n>> \tstrbuf_release(&new_path);\n>> +\tfree(git_man_path);\n>> }\n>\n>> -static void get_html_page_path(struct strbuf *page_path, const char *page)\n>> +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path)\n>> {\n>> \tstruct stat st;\n>> -\tif (!html_path)\n>> -\t\thtml_path = system_path(GIT_HTML_PATH);\n>>\n>> \t/* Check that we have a git documentation directory. */\n>> \tif (!strstr(html_path, \"://\")) {\n>> \t\tif (stat(mkpath(\"%s/git.html\", html_path), &st)\n>> -\t\t || !S_ISREG(st.st_mode))\n>> -\t\t\tdie(\"'%s': not a documentation directory.\", html_path);\n>> +\t\t\t\t|| !S_ISREG(st.st_mode)){\n>> +\t\t\tprintf(\"'%s': not a documentation directory.\\n\", html_path);\n>> +\t\t\tfree(html_path);\n>> +\t\t\texit(1);\n>\n> This is an unnecessary churn, and an unwarranted change to the\n> externally visible behaviour. die() gives its message to the\n> standard error stream (not to the standard output stream), and exits\n> with 128 (not with 1).\n>\n> We are exiting immediately after; it is not worth changing the API\n> to the caller or external behaviour, only to free(html_path)\n> conditionally.\n\nSo how can i free html_path here if when i'll return die instead exit? I\ncan't use free(html_path) before/after die call.\n\n>\n>> diff --git a/builtin/init-db.c b/builtin/init-db.c\n>> index 2619ca5..6f36f3f 100644\n>> --- a/builtin/init-db.c\n>> +++ b/builtin/init-db.c\n>> @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir)\n>> \t\ttemplate_dir = init_db_template_dir;\n>> \tif (!template_dir)\n>> \t\ttemplate_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR);\n>> -\tif (!template_dir[0])\n>> +\tif (!template_dir[0]){\n>\n> A SP between ) and {\n>\n>> +\t\tif (!template_dir)\n>> +\t\t\tfree((char*)template_dir);\n>\n> No need for \"call free only if it is not NULL\"; free(NULL) works\n> just fine.\n\nWill fix it.\n\n>\n>> \t\treturn;\n>> +\t}\n>> \ttemplate_len = strlen(template_dir);\n>> -\tif (PATH_MAX <= (template_len+strlen(\"/config\")))\n>> +\tif (PATH_MAX <= (template_len+strlen(\"/config\"))){\n>> +\t\tif (!template_dir)\n>> +\t\t\tfree((char*)template_dir);\n>> \t\tdie(_(\"insanely long template path %s\"), template_dir);\n>\n> You are freeing and then using it in the message, and it cannot be\n> NULL anyway so \"if (!template_dir)\" is doubly unnecessary.\n>\n> I wouldn't bother freeing immediately before dying, exiting, or\n> execing.\n>\n> Here is my quick attempt, starting solely from the output of \"git\n> grep system_path\". How does it look?\n\nIt looks fantastic :)\n\n>\n> One thing to note is that this illustration does not consider memory\n> pointed at by the \"system_wide\" variable here (from attr.c)\n>\n> static const char *git_etc_gitattributes(void)\n> {\n> static const char *system_wide;\n> if (!system_wide)\n> system_wide = system_path(ETC_GITATTRIBUTES);\n> return system_wide;\n> }\n>\n> at the point of process exit as a \"leak\".\n\nBut why? We allocated memory to \"system_wide\" with system_path, next git\nwill exit somewhere with die, but system_wide didn't free... Or i'm\nwrong here too?\n\nAnyway many thanks for so great feedback, tomorrow I'll reread it and\nwill try to do all things in correct way.\n\nThanky you.\n"},{"id":"252445","messageId":"xmqqk32kwdso.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"87ppcc4b2g.fsf@gmail.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-24T20:20:55Z","receivedAt":"2014-11-24T20:20:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Kuleshov writes:\n\n> Junio C Hamano @ 2014-11-25 01:33 ALMT:\n> ...\n>>> \tif (git_attr_system()) {\n>>> -\t\telem = read_attr_from_file(git_etc_gitattributes(), 1);\n>>> +\t\tchar *etc_attributes = git_etc_gitattributes();\n>>> +\t\telem = read_attr_from_file(etc_attributes, 1);\n>>> +\t\tfree(etc_attributes);\n>>\n>> And freeing here is actively wrong, I think. You are freeing the\n>> piece of memory still pointed by \"static char *system_wide\" in the\n>> function git_etc_gitattributes(); when it is called again, the\n>> caller will get a pointer into the memory you have freed here.\n>\n> Why? If i understand correctly we don't use etc_attributes anymore in\n> this function and if we'll call this function again\n> git_etc_gitattributes will create new pointer and system_path alloc\n> memory for it or i'm wrong with it?\n\nThe function keeps a singleton in \"static const char *system_wide\"\nso that it has to call system_path() only once, and keeps the value\nfor second and subsequent calls. From its callers' point of view,\nthey are only peeking the memory it returns.\n\nThis aspect does not change with your patch.\n\n -static const char *git_etc_gitattributes(void)\n +static char *git_etc_gitattributes(void)\n {\n -\tstatic const char *system_wide;\n +\tstatic char *system_wide;\n if (!system_wide)\n system_wide = system_path(ETC_GITATTRIBUTES);\n return system_wide;\n"},{"id":"252447","messageId":"xmqqbnnwwcg0.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"87ppcc4b2g.fsf@gmail.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-24T20:50:07Z","receivedAt":"2014-11-24T20:50:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Kuleshov writes:\n\n>> One thing to note is that this illustration does not consider memory\n>> pointed at by the \"system_wide\" variable here (from attr.c)\n>>\n>> static const char *git_etc_gitattributes(void)\n>> {\n>> static const char *system_wide;\n>> if (!system_wide)\n>> system_wide = system_path(ETC_GITATTRIBUTES);\n>> return system_wide;\n>> }\n>>\n>> at the point of process exit as a \"leak\".\n>\n> But why? We allocated memory to \"system_wide\" with system_path, next git\n> will exit somewhere with die, but system_wide didn't free... Or i'm\n> wrong here too?\n\nIt is in the same league as \"static const char *git_dir\" and friends\nthat appear in the file-scope-static of environment.c. Keeping small\nthings around to be cleaned up by exit() is not a crime.\n"},{"id":"252495","messageId":"CANCZXo7yDJCuhKVFG3QfSSoem+KN_9VCbGerTd+5tqQuzA7dbg@mail.gmail.com","threadId":"38028","inReplyTo":"xmqqbnnwwcg0.fsf@gitster.dls.corp.google.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-25T06:45:35Z","receivedAt":"2014-11-25T06:45:35Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello Junio,\n\nI returned static to git_etc_gitattributes return value, but now i\ncan't understand where to free 'system_wide'. As i understand\ncorrectly attr.c has following API for querying attributes:\n\ngit_attr\n\ngit_check_attr\n\nAnd as i see in code git_attr doesn't use git_attr_check, so now we\nhave only git_check_attr and git_all_stars functions which are through\nprepare_attr_stack and bootstrap_attr_stack in it uses path which\nreturned\n by system_path. So you're right if i free etc_attributes like this\nand git_etc_gitattributes will return static value we'll have access\nto freed memory if it will called again. But we have no access to\netc_attributes outside bootstrap_attr_stack so where will be best\nplace to free it?\n\nThank you\n\n2014-11-25 2:50 GMT+06:00 Junio C Hamano :\n> Alex Kuleshov writes:\n>\n>>> One thing to note is that this illustration does not consider memory\n>>> pointed at by the \"system_wide\" variable here (from attr.c)\n>>>\n>>> static const char *git_etc_gitattributes(void)\n>>> {\n>>> static const char *system_wide;\n>>> if (!system_wide)\n>>> system_wide = system_path(ETC_GITATTRIBUTES);\n>>> return system_wide;\n>>> }\n>>>\n>>> at the point of process exit as a \"leak\".\n>>\n>> But why? We allocated memory to \"system_wide\" with system_path, next git\n>> will exit somewhere with die, but system_wide didn't free... Or i'm\n>> wrong here too?\n>\n> It is in the same league as \"static const char *git_dir\" and friends\n> that appear in the file-scope-static of environment.c. Keeping small\n> things around to be cleaned up by exit() is not a crime.\n\n\n\n-- \n_________________________\n0xAX\n"},{"id":"252496","messageId":"CANCZXo4C_6Zfob9VnxjGxPbsRnUioVqC10z3Hhv09_xtrx-Pog@mail.gmail.com","threadId":"38028","inReplyTo":"CANCZXo7yDJCuhKVFG3QfSSoem+KN_9VCbGerTd+5tqQuzA7dbg@mail.gmail.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-25T07:04:46Z","receivedAt":"2014-11-25T07:04:46Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"But if we still static const char* for git_etc_gitattributes and will\nnot free it in bootstrap_attr_stack there will no memory leak, just\ntested it, so i'll remove free for it.\n\n2014-11-25 12:45 GMT+06:00 Alexander Kuleshov :\n> Hello Junio,\n>\n> I returned static to git_etc_gitattributes return value, but now i\n> can't understand where to free 'system_wide'. As i understand\n> correctly attr.c has following API for querying attributes:\n>\n> git_attr\n>\n> git_check_attr\n>\n> And as i see in code git_attr doesn't use git_attr_check, so now we\n> have only git_check_attr and git_all_stars functions which are through\n> prepare_attr_stack and bootstrap_attr_stack in it uses path which\n> returned\n> by system_path. So you're right if i free etc_attributes like this\n> and git_etc_gitattributes will return static value we'll have access\n> to freed memory if it will called again. But we have no access to\n> etc_attributes outside bootstrap_attr_stack so where will be best\n> place to free it?\n>\n> Thank you\n>\n> 2014-11-25 2:50 GMT+06:00 Junio C Hamano :\n>> Alex Kuleshov writes:\n>>\n>>>> One thing to note is that this illustration does not consider memory\n>>>> pointed at by the \"system_wide\" variable here (from attr.c)\n>>>>\n>>>> static const char *git_etc_gitattributes(void)\n>>>> {\n>>>> static const char *system_wide;\n>>>> if (!system_wide)\n>>>> system_wide = system_path(ETC_GITATTRIBUTES);\n>>>> return system_wide;\n>>>> }\n>>>>\n>>>> at the point of process exit as a \"leak\".\n>>>\n>>> But why? We allocated memory to \"system_wide\" with system_path, next git\n>>> will exit somewhere with die, but system_wide didn't free... Or i'm\n>>> wrong here too?\n>>\n>> It is in the same league as \"static const char *git_dir\" and friends\n>> that appear in the file-scope-static of environment.c. Keeping small\n>> things around to be cleaned up by exit() is not a crime.\n>\n>\n>\n> --\n> _________________________\n> 0xAX\n\n\n\n-- \n_________________________\n0xAX\n"},{"id":"252528","messageId":"xmqqbnnvtbac.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"CANCZXo4C_6Zfob9VnxjGxPbsRnUioVqC10z3Hhv09_xtrx-Pog@mail.gmail.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-25T17:55:39Z","receivedAt":"2014-11-25T17:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov writes:\n\n> But if we still static const char* for git_etc_gitattributes and will\n> not free it in bootstrap_attr_stack there will no memory leak, just\n> tested it, so i'll remove free for it.\n\nLeak or no leak, freeing there is wrong. It will free the piece of\nmemory the next caller will obtain from the git_etc_gitattributes()\nfunction. In other words, the return value from that function is\nowned by git_etc_gitattributes(), not by the caller.\n\nAs to \"leak\", in the strictest sense of the word, i.e. \"do we\nallocate on the heap and exit without freeing?\", it _is_ leaking,\nand your above \"just tested it\" probably means \"the tool I used did\nnot find/report it\". But as I said, it is not something worth\nbothering about [*1*].\n\nThink of it this way.\n\nThe git_etc_gitattributes() function goes like this with your patch\n(and there is no fundamental difference in the original version,\nwhich uses \"const char *\" where appropriate):\n\n static char *git_etc_gitattributes(void)\n {\n static char *system_wide;\n if (!system_wide)\n system_wide = system_path(ETC_GITATTRIBUTES);\n return system_wide;\n }\n\nIf you knew that the pathname for /etc/gitattributes substitute will\nnever be longer than 256 bytes, you may have coded the above like so\ninstead:\n\n static char system_wide[256];\n static char *git_etc_gitattributes(void)\n {\n if (!system_wide[0]) {\n char *ret = system_path(ETC_GITATTRIBUTES);\n if (strncpy(system_wide, ret, 256) >= 256)\n die(\"ETC_GITATTRIBUTES too long \");\n } \n return system_wide;\n }\n\nEven though we used the memory occupied by system_wide[] and did not\nclean up before exit(), nobody would complain about leaking.\n\nThe existing code is the moral equivalent of the \"up to 256 bytes\"\nillustration, but for a string whose size is not known at compile\ntime. It is using and keeping the memory until program exit.\nNobody should complain about leaking.\n\nThat is, unless (1) the held memory is expected to be very large and\n(2) you can prove that after the point you decide to insert free(),\nnobody will ever need that information again.\n\n\n[Footnote]\n\n*1* The leaks we care about are of this form:\n\n void silly_leaker(void)\n {\n printf(\"%s\\n\", system_path(ETC_GITATTRIBUTES));\n }\n\n where each invocation allocates memory, uses it and then loses\n the reference to it without doing anything to clean-up. You can\n call such a function unbounded number of times and waste\n unbounded amount of memory.\n\n The implementation of git_etc_gitattributes() is not of that\n kind. An invocation of it allocates, uses and keeps. The\n second and subsequent invocation does not allocate. When you\n call it unbounded number of times, it does not waste unbounded\n amount of memory. It just keeps what it needs to answer the\n next caller with.\n\n The pattern needs to be made thread-safe, but that is a separate\n topic.\n"},{"id":"252529","messageId":"CANCZXo53s4X4sZbQ1Tj3bVinbPVTodMvGiJR6kevYtZKkJvPYg@mail.gmail.com","threadId":"38028","inReplyTo":"CANCZXo4C_6Zfob9VnxjGxPbsRnUioVqC10z3Hhv09_xtrx-Pog@mail.gmail.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-25T17:59:05Z","receivedAt":"2014-11-25T17:59:05Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Hello Junio,\n\nI'm working on this patch and builtin/config.c, i have a little question:\n\nLook, here - https://github.com/git/git/blob/master/builtin/config.c#L509\nand in the next if clauses we get allocated path, but if i test it\nwith given config file with git config -f....\n(https://github.com/git/git/blob/master/builtin/config.c#L513) i have\nprefix NULL and next if clause doesn't execute, so if i'll put\nfree(given_config_source.file) after config action i'll get error\nbecause given_config_source.file wasn't allocated. How to be with\nthis?\n\nThank you.\n\n2014-11-25 13:04 GMT+06:00 Alexander Kuleshov :\n> But if we still static const char* for git_etc_gitattributes and will\n> not free it in bootstrap_attr_stack there will no memory leak, just\n> tested it, so i'll remove free for it.\n>\n> 2014-11-25 12:45 GMT+06:00 Alexander Kuleshov :\n>> Hello Junio,\n>>\n>> I returned static to git_etc_gitattributes return value, but now i\n>> can't understand where to free 'system_wide'. As i understand\n>> correctly attr.c has following API for querying attributes:\n>>\n>> git_attr\n>>\n>> git_check_attr\n>>\n>> And as i see in code git_attr doesn't use git_attr_check, so now we\n>> have only git_check_attr and git_all_stars functions which are through\n>> prepare_attr_stack and bootstrap_attr_stack in it uses path which\n>> returned\n>> by system_path. So you're right if i free etc_attributes like this\n>> and git_etc_gitattributes will return static value we'll have access\n>> to freed memory if it will called again. But we have no access to\n>> etc_attributes outside bootstrap_attr_stack so where will be best\n>> place to free it?\n>>\n>> Thank you\n>>\n>> 2014-11-25 2:50 GMT+06:00 Junio C Hamano :\n>>> Alex Kuleshov writes:\n>>>\n>>>>> One thing to note is that this illustration does not consider memory\n>>>>> pointed at by the \"system_wide\" variable here (from attr.c)\n>>>>>\n>>>>> static const char *git_etc_gitattributes(void)\n>>>>> {\n>>>>> static const char *system_wide;\n>>>>> if (!system_wide)\n>>>>> system_wide = system_path(ETC_GITATTRIBUTES);\n>>>>> return system_wide;\n>>>>> }\n>>>>>\n>>>>> at the point of process exit as a \"leak\".\n>>>>\n>>>> But why? We allocated memory to \"system_wide\" with system_path, next git\n>>>> will exit somewhere with die, but system_wide didn't free... Or i'm\n>>>> wrong here too?\n>>>\n>>> It is in the same league as \"static const char *git_dir\" and friends\n>>> that appear in the file-scope-static of environment.c. Keeping small\n>>> things around to be cleaned up by exit() is not a crime.\n>>\n>>\n>>\n>> --\n>> _________________________\n>> 0xAX\n>\n>\n>\n> --\n> _________________________\n> 0xAX\n\n\n\n-- \n_________________________\n0xAX\n"},{"id":"252530","messageId":"CANCZXo4=D=RPnGeAfcvD0O1hX0B43z1b11gyzjbJYGXzusfFYg@mail.gmail.com","threadId":"38028","inReplyTo":"xmqqbnnvtbac.fsf@gitster.dls.corp.google.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-25T18:03:54Z","receivedAt":"2014-11-25T18:03:54Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Ah, you little ahead me, so we do not care about config.c in this way,\nbecause it takes git_etc_gitconfig() in the same way as\ngit_etc_gitattributes\n\n2014-11-25 23:55 GMT+06:00 Junio C Hamano :\n> Alexander Kuleshov writes:\n>\n>> But if we still static const char* for git_etc_gitattributes and will\n>> not free it in bootstrap_attr_stack there will no memory leak, just\n>> tested it, so i'll remove free for it.\n>\n> Leak or no leak, freeing there is wrong. It will free the piece of\n> memory the next caller will obtain from the git_etc_gitattributes()\n> function. In other words, the return value from that function is\n> owned by git_etc_gitattributes(), not by the caller.\n>\n> As to \"leak\", in the strictest sense of the word, i.e. \"do we\n> allocate on the heap and exit without freeing?\", it _is_ leaking,\n> and your above \"just tested it\" probably means \"the tool I used did\n> not find/report it\". But as I said, it is not something worth\n> bothering about [*1*].\n>\n> Think of it this way.\n>\n> The git_etc_gitattributes() function goes like this with your patch\n> (and there is no fundamental difference in the original version,\n> which uses \"const char *\" where appropriate):\n>\n> static char *git_etc_gitattributes(void)\n> {\n> static char *system_wide;\n> if (!system_wide)\n> system_wide = system_path(ETC_GITATTRIBUTES);\n> return system_wide;\n> }\n>\n> If you knew that the pathname for /etc/gitattributes substitute will\n> never be longer than 256 bytes, you may have coded the above like so\n> instead:\n>\n> static char system_wide[256];\n> static char *git_etc_gitattributes(void)\n> {\n> if (!system_wide[0]) {\n> char *ret = system_path(ETC_GITATTRIBUTES);\n> if (strncpy(system_wide, ret, 256) >= 256)\n> die(\"ETC_GITATTRIBUTES too long \");\n> }\n> return system_wide;\n> }\n>\n> Even though we used the memory occupied by system_wide[] and did not\n> clean up before exit(), nobody would complain about leaking.\n>\n> The existing code is the moral equivalent of the \"up to 256 bytes\"\n> illustration, but for a string whose size is not known at compile\n> time. It is using and keeping the memory until program exit.\n> Nobody should complain about leaking.\n>\n> That is, unless (1) the held memory is expected to be very large and\n> (2) you can prove that after the point you decide to insert free(),\n> nobody will ever need that information again.\n>\n>\n> [Footnote]\n>\n> *1* The leaks we care about are of this form:\n>\n> void silly_leaker(void)\n> {\n> printf(\"%s\\n\", system_path(ETC_GITATTRIBUTES));\n> }\n>\n> where each invocation allocates memory, uses it and then loses\n> the reference to it without doing anything to clean-up. You can\n> call such a function unbounded number of times and waste\n> unbounded amount of memory.\n>\n> The implementation of git_etc_gitattributes() is not of that\n> kind. An invocation of it allocates, uses and keeps. The\n> second and subsequent invocation does not allocate. When you\n> call it unbounded number of times, it does not waste unbounded\n> amount of memory. It just keeps what it needs to answer the\n> next caller with.\n>\n> The pattern needs to be made thread-safe, but that is a separate\n> topic.\n\n\n\n-- \n_________________________\n0xAX\n"},{"id":"252534","messageId":"1416939854-29930-1-git-send-email-kuleshovmail@gmail.com","threadId":"38028","inReplyTo":"CANCZXo4=D=RPnGeAfcvD0O1hX0B43z1b11gyzjbJYGXzusfFYg@mail.gmail.com","subject":"[PATCH 1/1] change contract between system_path and it's callers","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-25T18:24:14Z","receivedAt":"2014-11-25T18:24:14Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Now system_path returns path which is allocated string to callers;\nIt prevents memory leaks in some places. All callers of system_path\nare owners of path string and they must release it.\n\nSigned-off-by: Alexander Kuleshov \n---\n builtin/help.c | 10 +++++++---\n exec_cmd.c | 17 +++++++++--------\n exec_cmd.h | 4 ++--\n git.c | 16 ++++++++++++----\n 4 files changed, 30 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b3c818e..544d1cc 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -321,12 +321,13 @@ static void setup_man_path(void)\n {\n \tstruct strbuf new_path = STRBUF_INIT;\n \tconst char *old_path = getenv(\"MANPATH\");\n-\n+\tchar *git_man_path = system_path(GIT_MAN_PATH);\n \t/* We should always put ':' after our path. If there is no\n \t * old_path, the ':' at the end will let 'man' to try\n \t * system-wide paths after ours to find the manual page. If\n \t * there is old_path, we need ':' as delimiter. */\n-\tstrbuf_addstr(&new_path, system_path(GIT_MAN_PATH));\n+\tstrbuf_addstr(&new_path, git_man_path);\n+\tfree(git_man_path);\n \tstrbuf_addch(&new_path, ':');\n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n@@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)\n static void show_info_page(const char *git_cmd)\n {\n \tconst char *page = cmd_to_page(git_cmd);\n-\tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n+\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\tsetenv(\"INFOPATH\", git_info_path, 1);\n+\tfree(git_info_path);\n \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n \tdie(_(\"no info viewer handled the request\"));\n }\n@@ -392,6 +395,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)\n \n \tstrbuf_init(page_path, 0);\n \tstrbuf_addf(page_path, \"%s/%s.html\", html_path, page);\n+\tfree((char*)html_path);\n }\n \n /*\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 698e752..e3ebdd6 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -6,7 +6,7 @@\n static const char *argv_exec_path;\n static const char *argv0_path;\n \n-const char *system_path(const char *path)\n+char *system_path(const char *path)\n {\n #ifdef RUNTIME_PREFIX\n \tstatic const char *prefix;\n@@ -16,7 +16,7 @@ const char *system_path(const char *path)\n \tstruct strbuf d = STRBUF_INIT;\n \n \tif (is_absolute_path(path))\n-\t\treturn path;\n+\t\treturn strdup(path);\n \n #ifdef RUNTIME_PREFIX\n \tassert(argv0_path);\n@@ -34,8 +34,7 @@ const char *system_path(const char *path)\n #endif\n \n \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n-\tpath = strbuf_detach(&d, NULL);\n-\treturn path;\n+\treturn d.buf;\n }\n \n const char *git_extract_argv0_path(const char *argv0)\n@@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)\n \n \n /* Returns the highest-priority, location to look for git programs. */\n-const char *git_exec_path(void)\n+char *git_exec_path(void)\n {\n \tconst char *env;\n \n \tif (argv_exec_path)\n-\t\treturn argv_exec_path;\n+\t\treturn strdup(argv_exec_path);\n \n \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n \tif (env && *env) {\n-\t\treturn env;\n+\t\treturn strdup(env);\n \t}\n \n \treturn system_path(GIT_EXEC_PATH);\n@@ -95,8 +94,10 @@ void setup_path(void)\n {\n \tconst char *old_path = getenv(\"PATH\");\n \tstruct strbuf new_path = STRBUF_INIT;\n+\tchar* exec_path = git_exec_path();\n \n-\tadd_path(&new_path, git_exec_path());\n+\tadd_path(&new_path, exec_path);\n+\tfree(exec_path);\n \tadd_path(&new_path, argv0_path);\n \n \tif (old_path)\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex e4c9702..03c8599 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -3,12 +3,12 @@\n \n extern void git_set_argv_exec_path(const char *exec_path);\n extern const char *git_extract_argv0_path(const char *path);\n-extern const char *git_exec_path(void);\n+extern char *git_exec_path(void);\n extern void setup_path(void);\n extern const char **prepare_git_cmd(const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n LAST_ARG_MUST_BE_NULL\n extern int execl_git_cmd(const char *cmd, ...);\n-extern const char *system_path(const char *path);\n+extern char *system_path(const char *path);\n \n #endif /* GIT_EXEC_CMD_H */\ndiff --git a/git.c b/git.c\nindex 82d7a1c..d01c4f1 100644\n--- a/git.c\n+++ b/git.c\n@@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tif (*cmd == '=')\n \t\t\t\tgit_set_argv_exec_path(cmd + 1);\n \t\t\telse {\n-\t\t\t\tputs(git_exec_path());\n+\t\t\t\tchar *exec_path = git_exec_path();\n+\t\t\t\tputs(exec_path);\n+\t\t\t\tfree(exec_path);\n \t\t\t\texit(0);\n \t\t\t}\n \t\t} else if (!strcmp(cmd, \"--html-path\")) {\n-\t\t\tputs(system_path(GIT_HTML_PATH));\n+\t\t\tchar *git_html_path = system_path(GIT_HTML_PATH);\n+\t\t\tputs(git_html_path);\n+\t\t\tfree(git_html_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--man-path\")) {\n-\t\t\tputs(system_path(GIT_MAN_PATH));\n+\t\t\tchar *git_man_path = system_path(GIT_MAN_PATH);\n+\t\t\tputs(git_man_path);\n+\t\t\tfree(git_man_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"--info-path\")) {\n-\t\t\tputs(system_path(GIT_INFO_PATH));\n+\t\t\tchar *git_info_path = system_path(GIT_INFO_PATH);\n+\t\t\tputs(git_info_path);\n+\t\t\tfree(git_info_path);\n \t\t\texit(0);\n \t\t} else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n \t\t\tuse_pager = 1;\n-- \n2.2.0.rc3.205.gea90ae3.dirty\n"},{"id":"252539","messageId":"xmqqk32jrq11.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"CANCZXo4=D=RPnGeAfcvD0O1hX0B43z1b11gyzjbJYGXzusfFYg@mail.gmail.com","subject":"Re: Re*: [PATCH] change contract between system_path and it's callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-25T20:20:10Z","receivedAt":"2014-11-25T20:20:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov writes:\n\n> Ah, you little ahead me, so we do not care about config.c in this way,\n> because it takes git_etc_gitconfig() in the same way as\n> git_etc_gitattributes\n\nYes, but looking at the file, you would notice that the\n\"use_local_config\" codepath assigns a string obtained from\ngit_pathdup() to given_config_source.file, which means that somebody\nlater in the code path would not know if given_config_source.file is\nmerely pointing at a piece of memory owned by somebody else\n(e.g. came from git_etc_gitconfig()) or if it owns the piece of\nmemory (e.g. came from git_pathdup()). In the former case the\nmemory should never be freed, but not freeing in the latter would\nleak the memory.\n\nAssignment to given_config_source.file I see in that function that\nborrows (i.e. the current code that does not fee is correct) are:\n\n - getenv(CONFIG_ENVIRONMENT)\n - NULL ;-) obviously\n - git_etc_gitconfig()\n \nAll the other assignment to given_config_source.file (including the\nones obtained from a call to home_config_paths()) makes us\nresponsible to free that piece memory, so they are technically\nleaks.\n\nBut cmd_config() is a moral equivalent of main() in a typical\nprogram, so it might not be worth worrying about allocating a single\npiece of memory that is used throughout the lifetime of that\nfunction and leaving it without freeing.\n\n \n"},{"id":"252541","messageId":"xmqqfvd7rnkb.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"1416939854-29930-1-git-send-email-kuleshovmail@gmail.com","subject":"Re: [PATCH 1/1] change contract between system_path and it's callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-25T21:13:24Z","receivedAt":"2014-11-25T21:13:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov writes:\n\n> Now system_path returns path which is allocated string to callers;\n> It prevents memory leaks in some places. All callers of system_path\n> are owners of path string and they must release it.\n>\n> Signed-off-by: Alexander Kuleshov \n> ---\n\nComparing this with what I sent out...\n\n> builtin/help.c | 10 +++++++---\n> exec_cmd.c | 17 +++++++++--------\n> exec_cmd.h | 4 ++--\n> git.c | 16 ++++++++++++----\n> 4 files changed, 30 insertions(+), 17 deletions(-)\n>\n> @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)\n> static void show_info_page(const char *git_cmd)\n> {\n> \tconst char *page = cmd_to_page(git_cmd);\n> -\tsetenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n> +\tchar *git_info_path = system_path(GIT_INFO_PATH);\n> +\tsetenv(\"INFOPATH\", git_info_path, 1);\n> +\tfree(git_info_path);\n\nWe are just about to exec; does this warrant the code churn?\n\n> \texeclp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n> \tdie(_(\"no info viewer handled the request\"));\n\n> @@ -34,8 +34,7 @@ const char *system_path(const char *path)\n> #endif\n> \n> \tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n> -\tpath = strbuf_detach(&d, NULL);\n> -\treturn path;\n> +\treturn d.buf;\n\nThese happens to be the same with the current strbuf implementation,\nbut it is a good manner to use strbuf_detach(&d, NULL) here. We\ndon't know what other de-initialization tomorrow's implementation of\nthe strbuf API may have to do in strbuf_detach().\n\n> @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)\n> \n> \n> /* Returns the highest-priority, location to look for git programs. */\n> -const char *git_exec_path(void)\n> +char *git_exec_path(void)\n> {\n> \tconst char *env;\n> \n> \tif (argv_exec_path)\n> -\t\treturn argv_exec_path;\n> +\t\treturn strdup(argv_exec_path);\n> \n> \tenv = getenv(EXEC_PATH_ENVIRONMENT);\n> \tif (env && *env) {\n> -\t\treturn env;\n> +\t\treturn strdup(env);\n> \t}\n\nNow you are making callers of git_exec_path() responsible for\nfreeing the result they receive.\n\ngit_exec_path() may be called quite a lot, which means we may end up\ncalling system_path() many times during the life of a process\nwithout freeing its return value, so this change may be worth doing,\nbut this patch is insufficient, isn't it?\n\nYou just added load_command_list() in help.c a new leak or two, for\nexample. There probably are other callers of this function but I\ndon't have time to look at all of them myself right now.\n\n> @@ -95,8 +94,10 @@ void setup_path(void)\n> {\n> \tconst char *old_path = getenv(\"PATH\");\n> \tstruct strbuf new_path = STRBUF_INIT;\n> +\tchar* exec_path = git_exec_path();\n> \n> -\tadd_path(&new_path, git_exec_path());\n> +\tadd_path(&new_path, exec_path);\n> +\tfree(exec_path);\n> \tadd_path(&new_path, argv0_path);\n\nThis part by itself is good, provided if we make it the caller's\nresponsiblity to free string returned by git_exec_path().\n\n> diff --git a/git.c b/git.c\n> index 82d7a1c..d01c4f1 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\tif (*cmd == '=')\n> \t\t\t\tgit_set_argv_exec_path(cmd + 1);\n> \t\t\telse {\n> -\t\t\t\tputs(git_exec_path());\n> +\t\t\t\tchar *exec_path = git_exec_path();\n> +\t\t\t\tputs(exec_path);\n> +\t\t\t\tfree(exec_path);\n> \t\t\t\texit(0);\n> \t\t\t}\n> \t\t} else if (!strcmp(cmd, \"--html-path\")) {\n> -\t\t\tputs(system_path(GIT_HTML_PATH));\n> +\t\t\tchar *git_html_path = system_path(GIT_HTML_PATH);\n> +\t\t\tputs(git_html_path);\n> +\t\t\tfree(git_html_path);\n> \t\t\texit(0);\n> \t\t} else if (!strcmp(cmd, \"--man-path\")) {\n> -\t\t\tputs(system_path(GIT_MAN_PATH));\n> +\t\t\tchar *git_man_path = system_path(GIT_MAN_PATH);\n> +\t\t\tputs(git_man_path);\n> +\t\t\tfree(git_man_path);\n> \t\t\texit(0);\n> \t\t} else if (!strcmp(cmd, \"--info-path\")) {\n> -\t\t\tputs(system_path(GIT_INFO_PATH));\n> +\t\t\tchar *git_info_path = system_path(GIT_INFO_PATH);\n> +\t\t\tputs(git_info_path);\n> +\t\t\tfree(git_info_path);\n> \t\t\texit(0);\n> \t\t} else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n> \t\t\tuse_pager = 1;\n\nNone of these warrant the code churn, I would say.\n"},{"id":"252567","messageId":"CANCZXo63q2YmTcYr9Vf99fAAnENNj7vE1nC18QJDYve=-OcXyQ@mail.gmail.com","threadId":"38028","inReplyTo":"xmqqfvd7rnkb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/1] change contract between system_path and it's callers","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-26T03:53:34Z","receivedAt":"2014-11-26T03:53:34Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":">\n> Comparing this with what I sent out...\n>\n> > builtin/help.c | 10 +++++++---\n> > exec_cmd.c | 17 +++++++++--------\n> > exec_cmd.h | 4 ++--\n> > git.c | 16 ++++++++++++----\n> > 4 files changed, 30 insertions(+), 17 deletions(-)\n> >\n> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)\n> > static void show_info_page(const char *git_cmd)\n> > {\n> > const char *page = cmd_to_page(git_cmd);\n> > - setenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n> > + char *git_info_path = system_path(GIT_INFO_PATH);\n> > + setenv(\"INFOPATH\", git_info_path, 1);\n> > + free(git_info_path);\n>\n> We are just about to exec; does this warrant the code churn?\n\nhmm... Can't understand what's the problem here? We get git_info_path\nfrom system_path which returns pointer which will need to free, set it in\nenvironment var and than free it...\n\n>\n> > execlp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n> > die(_(\"no info viewer handled the request\"));\n>\n> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)\n> > #endif\n> >\n> > strbuf_addf(&d, \"%s/%s\", prefix, path);\n> > - path = strbuf_detach(&d, NULL);\n> > - return path;\n> > + return d.buf;\n>\n> These happens to be the same with the current strbuf implementation,\n> but it is a good manner to use strbuf_detach(&d, NULL) here. We\n> don't know what other de-initialization tomorrow's implementation of\n> the strbuf API may have to do in strbuf_detach().\n\nHow to do it in correct way?\n\n\n strbuf_addf(&d, \"%s/%s\", prefix, path);\n path = strbuf_detach(&d, NULL);\n return (char*)path;\n\nOr something else?\n\n>\n> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)\n> >\n> >\n> > /* Returns the highest-priority, location to look for git programs. */\n> > -const char *git_exec_path(void)\n> > +char *git_exec_path(void)\n> > {\n> > const char *env;\n> >\n> > if (argv_exec_path)\n> > - return argv_exec_path;\n> > + return strdup(argv_exec_path);\n> >\n> > env = getenv(EXEC_PATH_ENVIRONMENT);\n> > if (env && *env) {\n> > - return env;\n> > + return strdup(env);\n> > }\n>\n> Now you are making callers of git_exec_path() responsible for\n> freeing the result they receive.\n>\n> git_exec_path() may be called quite a lot, which means we may end up\n> calling system_path() many times during the life of a process\n> without freeing its return value, so this change may be worth doing,\n> but this patch is insufficient, isn't it?\n>\n> You just added load_command_list() in help.c a new leak or two, for\n> example. There probably are other callers of this function but I\n> don't have time to look at all of them myself right now.\n\nYes, need to do that all git_exec_path() callers free result of git_exec_path.\n\n>\n> > @@ -95,8 +94,10 @@ void setup_path(void)\n> > {\n> > const char *old_path = getenv(\"PATH\");\n> > struct strbuf new_path = STRBUF_INIT;\n> > + char* exec_path = git_exec_path();\n> >\n> > - add_path(&new_path, git_exec_path());\n> > + add_path(&new_path, exec_path);\n> > + free(exec_path);\n> > add_path(&new_path, argv0_path);\n>\n> This part by itself is good, provided if we make it the caller's\n> responsiblity to free string returned by git_exec_path().\n>\n> > diff --git a/git.c b/git.c\n> > index 82d7a1c..d01c4f1 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> > if (*cmd == '=')\n> > git_set_argv_exec_path(cmd + 1);\n> > else {\n> > - puts(git_exec_path());\n> > + char *exec_path = git_exec_path();\n> > + puts(exec_path);\n> > + free(exec_path);\n> > exit(0);\n> > }\n> > } else if (!strcmp(cmd, \"--html-path\")) {\n> > - puts(system_path(GIT_HTML_PATH));\n> > + char *git_html_path = system_path(GIT_HTML_PATH);\n> > + puts(git_html_path);\n> > + free(git_html_path);\n> > exit(0);\n> > } else if (!strcmp(cmd, \"--man-path\")) {\n> > - puts(system_path(GIT_MAN_PATH));\n> > + char *git_man_path = system_path(GIT_MAN_PATH);\n> > + puts(git_man_path);\n> > + free(git_man_path);\n> > exit(0);\n> > } else if (!strcmp(cmd, \"--info-path\")) {\n> > - puts(system_path(GIT_INFO_PATH));\n> > + char *git_info_path = system_path(GIT_INFO_PATH);\n> > + puts(git_info_path);\n> > + free(git_info_path);\n> > exit(0);\n> > } else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n> > use_pager = 1;\n>\n> None of these warrant the code churn, I would say.\n\nSorry, english is not my first language, what did you mean when saying:\n\"code churn\"? Code duplication or something else?\n"},{"id":"252576","messageId":"CANCZXo66ZzCB=LWrup9-GBWBrT0ozOeoTm=8GUiarjte3t=2UQ@mail.gmail.com","threadId":"38028","inReplyTo":"CANCZXo63q2YmTcYr9Vf99fAAnENNj7vE1nC18QJDYve=-OcXyQ@mail.gmail.com","subject":"Re: [PATCH 1/1] change contract between system_path and it's callers","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-26T09:42:13Z","receivedAt":"2014-11-26T09:42:13Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Maybe we will discard this patch, because i looked on it and tested\nwith different places, it brings more leaks than before?\n\n2014-11-26 9:53 GMT+06:00 Alexander Kuleshov :\n>>\n>> Comparing this with what I sent out...\n>>\n>> > builtin/help.c | 10 +++++++---\n>> > exec_cmd.c | 17 +++++++++--------\n>> > exec_cmd.h | 4 ++--\n>> > git.c | 16 ++++++++++++----\n>> > 4 files changed, 30 insertions(+), 17 deletions(-)\n>> >\n>> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)\n>> > static void show_info_page(const char *git_cmd)\n>> > {\n>> > const char *page = cmd_to_page(git_cmd);\n>> > - setenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n>> > + char *git_info_path = system_path(GIT_INFO_PATH);\n>> > + setenv(\"INFOPATH\", git_info_path, 1);\n>> > + free(git_info_path);\n>>\n>> We are just about to exec; does this warrant the code churn?\n>\n> hmm... Can't understand what's the problem here? We get git_info_path\n> from system_path which returns pointer which will need to free, set it in\n> environment var and than free it...\n>\n>>\n>> > execlp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n>> > die(_(\"no info viewer handled the request\"));\n>>\n>> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)\n>> > #endif\n>> >\n>> > strbuf_addf(&d, \"%s/%s\", prefix, path);\n>> > - path = strbuf_detach(&d, NULL);\n>> > - return path;\n>> > + return d.buf;\n>>\n>> These happens to be the same with the current strbuf implementation,\n>> but it is a good manner to use strbuf_detach(&d, NULL) here. We\n>> don't know what other de-initialization tomorrow's implementation of\n>> the strbuf API may have to do in strbuf_detach().\n>\n> How to do it in correct way?\n>\n>\n> strbuf_addf(&d, \"%s/%s\", prefix, path);\n> path = strbuf_detach(&d, NULL);\n> return (char*)path;\n>\n> Or something else?\n>\n>>\n>> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)\n>> >\n>> >\n>> > /* Returns the highest-priority, location to look for git programs. */\n>> > -const char *git_exec_path(void)\n>> > +char *git_exec_path(void)\n>> > {\n>> > const char *env;\n>> >\n>> > if (argv_exec_path)\n>> > - return argv_exec_path;\n>> > + return strdup(argv_exec_path);\n>> >\n>> > env = getenv(EXEC_PATH_ENVIRONMENT);\n>> > if (env && *env) {\n>> > - return env;\n>> > + return strdup(env);\n>> > }\n>>\n>> Now you are making callers of git_exec_path() responsible for\n>> freeing the result they receive.\n>>\n>> git_exec_path() may be called quite a lot, which means we may end up\n>> calling system_path() many times during the life of a process\n>> without freeing its return value, so this change may be worth doing,\n>> but this patch is insufficient, isn't it?\n>>\n>> You just added load_command_list() in help.c a new leak or two, for\n>> example. There probably are other callers of this function but I\n>> don't have time to look at all of them myself right now.\n>\n> Yes, need to do that all git_exec_path() callers free result of git_exec_path.\n>\n>>\n>> > @@ -95,8 +94,10 @@ void setup_path(void)\n>> > {\n>> > const char *old_path = getenv(\"PATH\");\n>> > struct strbuf new_path = STRBUF_INIT;\n>> > + char* exec_path = git_exec_path();\n>> >\n>> > - add_path(&new_path, git_exec_path());\n>> > + add_path(&new_path, exec_path);\n>> > + free(exec_path);\n>> > add_path(&new_path, argv0_path);\n>>\n>> This part by itself is good, provided if we make it the caller's\n>> responsiblity to free string returned by git_exec_path().\n>>\n>> > diff --git a/git.c b/git.c\n>> > index 82d7a1c..d01c4f1 100644\n>> > --- a/git.c\n>> > +++ b/git.c\n>> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n>> > if (*cmd == '=')\n>> > git_set_argv_exec_path(cmd + 1);\n>> > else {\n>> > - puts(git_exec_path());\n>> > + char *exec_path = git_exec_path();\n>> > + puts(exec_path);\n>> > + free(exec_path);\n>> > exit(0);\n>> > }\n>> > } else if (!strcmp(cmd, \"--html-path\")) {\n>> > - puts(system_path(GIT_HTML_PATH));\n>> > + char *git_html_path = system_path(GIT_HTML_PATH);\n>> > + puts(git_html_path);\n>> > + free(git_html_path);\n>> > exit(0);\n>> > } else if (!strcmp(cmd, \"--man-path\")) {\n>> > - puts(system_path(GIT_MAN_PATH));\n>> > + char *git_man_path = system_path(GIT_MAN_PATH);\n>> > + puts(git_man_path);\n>> > + free(git_man_path);\n>> > exit(0);\n>> > } else if (!strcmp(cmd, \"--info-path\")) {\n>> > - puts(system_path(GIT_INFO_PATH));\n>> > + char *git_info_path = system_path(GIT_INFO_PATH);\n>> > + puts(git_info_path);\n>> > + free(git_info_path);\n>> > exit(0);\n>> > } else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n>> > use_pager = 1;\n>>\n>> None of these warrant the code churn, I would say.\n>\n> Sorry, english is not my first language, what did you mean when saying:\n> \"code churn\"? Code duplication or something else?\n\n\n\n-- \n_________________________\n0xAX\n"},{"id":"252580","messageId":"CANCZXo7Su_dsHeMXe+a0E=12ArQPVuwWLJNKFfbogM+JjUP4Vg@mail.gmail.com","threadId":"38028","inReplyTo":"CANCZXo66ZzCB=LWrup9-GBWBrT0ozOeoTm=8GUiarjte3t=2UQ@mail.gmail.com","subject":"Re: [PATCH 1/1] change contract between system_path and it's callers","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-26T14:00:26Z","receivedAt":"2014-11-26T14:00:26Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"What do you think if we create int variable, something like\ngiven_config_must_free = 0; and will set up it to 1 in cases where it\nwill be allocated, and than we can free it in the end of cmd_config?\nI'm reading git source code to understanding git internals and found\nlittle memory leak in cat-file, so i fixed it as i described above.\n\n2014-11-26 15:42 GMT+06:00 Alexander Kuleshov :\n> Maybe we will discard this patch, because i looked on it and tested\n> with different places, it brings more leaks than before?\n>\n> 2014-11-26 9:53 GMT+06:00 Alexander Kuleshov :\n>>>\n>>> Comparing this with what I sent out...\n>>>\n>>> > builtin/help.c | 10 +++++++---\n>>> > exec_cmd.c | 17 +++++++++--------\n>>> > exec_cmd.h | 4 ++--\n>>> > git.c | 16 ++++++++++++----\n>>> > 4 files changed, 30 insertions(+), 17 deletions(-)\n>>> >\n>>> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd)\n>>> > static void show_info_page(const char *git_cmd)\n>>> > {\n>>> > const char *page = cmd_to_page(git_cmd);\n>>> > - setenv(\"INFOPATH\", system_path(GIT_INFO_PATH), 1);\n>>> > + char *git_info_path = system_path(GIT_INFO_PATH);\n>>> > + setenv(\"INFOPATH\", git_info_path, 1);\n>>> > + free(git_info_path);\n>>>\n>>> We are just about to exec; does this warrant the code churn?\n>>\n>> hmm... Can't understand what's the problem here? We get git_info_path\n>> from system_path which returns pointer which will need to free, set it in\n>> environment var and than free it...\n>>\n>>>\n>>> > execlp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n>>> > die(_(\"no info viewer handled the request\"));\n>>>\n>>> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)\n>>> > #endif\n>>> >\n>>> > strbuf_addf(&d, \"%s/%s\", prefix, path);\n>>> > - path = strbuf_detach(&d, NULL);\n>>> > - return path;\n>>> > + return d.buf;\n>>>\n>>> These happens to be the same with the current strbuf implementation,\n>>> but it is a good manner to use strbuf_detach(&d, NULL) here. We\n>>> don't know what other de-initialization tomorrow's implementation of\n>>> the strbuf API may have to do in strbuf_detach().\n>>\n>> How to do it in correct way?\n>>\n>>\n>> strbuf_addf(&d, \"%s/%s\", prefix, path);\n>> path = strbuf_detach(&d, NULL);\n>> return (char*)path;\n>>\n>> Or something else?\n>>\n>>>\n>>> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path)\n>>> >\n>>> >\n>>> > /* Returns the highest-priority, location to look for git programs. */\n>>> > -const char *git_exec_path(void)\n>>> > +char *git_exec_path(void)\n>>> > {\n>>> > const char *env;\n>>> >\n>>> > if (argv_exec_path)\n>>> > - return argv_exec_path;\n>>> > + return strdup(argv_exec_path);\n>>> >\n>>> > env = getenv(EXEC_PATH_ENVIRONMENT);\n>>> > if (env && *env) {\n>>> > - return env;\n>>> > + return strdup(env);\n>>> > }\n>>>\n>>> Now you are making callers of git_exec_path() responsible for\n>>> freeing the result they receive.\n>>>\n>>> git_exec_path() may be called quite a lot, which means we may end up\n>>> calling system_path() many times during the life of a process\n>>> without freeing its return value, so this change may be worth doing,\n>>> but this patch is insufficient, isn't it?\n>>>\n>>> You just added load_command_list() in help.c a new leak or two, for\n>>> example. There probably are other callers of this function but I\n>>> don't have time to look at all of them myself right now.\n>>\n>> Yes, need to do that all git_exec_path() callers free result of git_exec_path.\n>>\n>>>\n>>> > @@ -95,8 +94,10 @@ void setup_path(void)\n>>> > {\n>>> > const char *old_path = getenv(\"PATH\");\n>>> > struct strbuf new_path = STRBUF_INIT;\n>>> > + char* exec_path = git_exec_path();\n>>> >\n>>> > - add_path(&new_path, git_exec_path());\n>>> > + add_path(&new_path, exec_path);\n>>> > + free(exec_path);\n>>> > add_path(&new_path, argv0_path);\n>>>\n>>> This part by itself is good, provided if we make it the caller's\n>>> responsiblity to free string returned by git_exec_path().\n>>>\n>>> > diff --git a/git.c b/git.c\n>>> > index 82d7a1c..d01c4f1 100644\n>>> > --- a/git.c\n>>> > +++ b/git.c\n>>> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n>>> > if (*cmd == '=')\n>>> > git_set_argv_exec_path(cmd + 1);\n>>> > else {\n>>> > - puts(git_exec_path());\n>>> > + char *exec_path = git_exec_path();\n>>> > + puts(exec_path);\n>>> > + free(exec_path);\n>>> > exit(0);\n>>> > }\n>>> > } else if (!strcmp(cmd, \"--html-path\")) {\n>>> > - puts(system_path(GIT_HTML_PATH));\n>>> > + char *git_html_path = system_path(GIT_HTML_PATH);\n>>> > + puts(git_html_path);\n>>> > + free(git_html_path);\n>>> > exit(0);\n>>> > } else if (!strcmp(cmd, \"--man-path\")) {\n>>> > - puts(system_path(GIT_MAN_PATH));\n>>> > + char *git_man_path = system_path(GIT_MAN_PATH);\n>>> > + puts(git_man_path);\n>>> > + free(git_man_path);\n>>> > exit(0);\n>>> > } else if (!strcmp(cmd, \"--info-path\")) {\n>>> > - puts(system_path(GIT_INFO_PATH));\n>>> > + char *git_info_path = system_path(GIT_INFO_PATH);\n>>> > + puts(git_info_path);\n>>> > + free(git_info_path);\n>>> > exit(0);\n>>> > } else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n>>> > use_pager = 1;\n>>>\n>>> None of these warrant the code churn, I would say.\n>>\n>> Sorry, english is not my first language, what did you mean when saying:\n>> \"code churn\"? Code duplication or something else?\n>\n>\n>\n> --\n> _________________________\n> 0xAX\n\n\n\n-- \n_________________________\n0xAX\n"},{"id":"252584","messageId":"xmqqy4qxrgq0.fsf@gitster.dls.corp.google.com","threadId":"38028","inReplyTo":"CANCZXo63q2YmTcYr9Vf99fAAnENNj7vE1nC18QJDYve=-OcXyQ@mail.gmail.com","subject":"Re: [PATCH 1/1] change contract between system_path and it's callers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-26T17:53:27Z","receivedAt":"2014-11-26T17:53:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Kuleshov writes:\n\n>> > + setenv(\"INFOPATH\", git_info_path, 1);\n>> > + free(git_info_path);\n>> > execlp(\"info\", \"info\", \"gitman\", page, (char *)NULL);\n>> > die(_(\"no info viewer handled the request\"));\n>>\n>> We are just about to exec; does this warrant the code churn?\n>\n> hmm... Can't understand what's the problem here?\n\nAdding a new variable to keep the allocated piece of memory so that\nyou can free after using it, and freeing it. These are not \"problem\"\nper-se.\n\nBut immediately after doing so, execlp() wipes your process and\nstarts a different program afresh. When that happens, all the heap\nmemory you allocated and were using is automatically reclaimed by\nthe system anyway.\n\nSo it may not be \"wrong\" to free, but doing anything extra is an\nunnecessary work.\n\nIf you did an \"assign to a variable to keep allocated memory, use\nit, free it and then exec\" sequence in a new code, I do not think it\nis worth fixing the code not to do so to reduce that unnecessary\nwork---it is not worth another round of review exchange. But in the\nsame way, make an existing code that is not doing an unnecessary\nwork to do so is not worth it.\n\n>> > @@ -34,8 +34,7 @@ const char *system_path(const char *path)\n>> > #endif\n>> >\n>> > strbuf_addf(&d, \"%s/%s\", prefix, path);\n>> > - path = strbuf_detach(&d, NULL);\n>> > - return path;\n>> > + return d.buf;\n>>\n>> These happens to be the same with the current strbuf implementation,\n>> but it is a good manner to use strbuf_detach(&d, NULL) here. We\n>> don't know what other de-initialization tomorrow's implementation of\n>> the strbuf API may have to do in strbuf_detach().\n>\n> How to do it in correct way?\n\nWouldn't \"return strbuf_detach(&d, NULL);\" work without any\nassignment to \"path\"?\n\n>> > ...\n>> > - puts(system_path(GIT_INFO_PATH));\n>> > + char *git_info_path = system_path(GIT_INFO_PATH);\n>> > + puts(git_info_path);\n>> > + free(git_info_path);\n>> > exit(0);\n>> > } else if (!strcmp(cmd, \"-p\") || !strcmp(cmd, \"--paginate\")) {\n>> > use_pager = 1;\n>>\n>> None of these warrant the code churn, I would say.\n>\n> Sorry, english is not my first language, what did you mean when saying:\n> \"code churn\"? Code duplication or something else?\n\nStep back a bit and _think_ why you want to avoid leaks. When a\nprogram is leaky, your process will bloat by accumulating unused\ncruft in your heap. Step back again and think the reason why you\nwant to avoid that bloat.\n\nIt is because the parts of your program that want to allocate and\nuse more memory will suffer _after_ leak happens, because leakers\nconsume and keep memory that would be otherwise useful if they did\nnot leak and freed instead. The later callers want to get more\nmemory, but cannot get it because the program leaked memory before\nthe control flow got to them.\n\nNow, who do these calls to free() you added help? Whose desire to\nallocate more memory can be harmed by the allocated but not freed\npiece of memory held by the return values from system_path()?\n\nNobody--exactly because the next thing you do is to exit.\n\nIt is not wrong per-se to free immediately before exiting; it is\njust like freeing immediately before execing. It is an unnecessary\nthing to do, and a change to do so has no or very little value, even\nif it is not a wrong thing to do.\n"},{"id":"252658","messageId":"7FDD11B6900A41269F60C9E60A11D84C@PhilipOakley","threadId":"38028","inReplyTo":"CANCZXo63q2YmTcYr9Vf99fAAnENNj7vE1nC18QJDYve=-OcXyQ@mail.gmail.com","subject":"Re: [PATCH 1/1] change contract between system_path and it's callers","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-11-28T11:13:24Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Alexander Kuleshov\" \nSent: Wednesday, November 26, 2014 3:53 AM\n>>\n>> None of these warrant the code churn, I would say.\n>\n> Sorry, english is not my first language, what did you mean when\n> saying:\n> \"code churn\"? Code duplication or something else?\n> --\nHi Alexander,\n\nThe term 'churn' is originally from British butter making.\n\nChurn:\nverb\n 1.shake (milk or cream) in a machine in order to produce butter.\n \"the cream is ripened before it is churned\"\n synonyms: stir, agitate;\n\n 2.(with reference to liquid) move or cause to move about vigorously.\n \"the seas churned\".\n\n\nFor Code (used in a somewhat negative sense), it means that lots of bits \nare moved around a great deal for\nlittle apparent benefit.\n\nIn the sense Junio used it, I believe it's suggesting that the balance \nbetween the amount of change and usefulness of the change had gone \nfurther than hoped. (Though Junio is usually open to receiving a well \nargued case)\n\nPhilip\nsee also http://en.wikipedia.org/wiki/Churn_rate\n"}]}