* GIT: [PATCH] exec_cmd: system_path memory leak fix @ 2014-11-23 13:56 0xAX 2014-11-23 13:56 ` 0xAX 2014-11-23 18:28 ` GIT: [PATCH] exec_cmd: system_path memory leak fix Junio C Hamano 0 siblings, 2 replies; 33+ messages in thread From: 0xAX @ 2014-11-23 13:56 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Alex Kuleshov Hello All, I found memory leak at exec_cmd.c in system_path function with valgrind. After this patch valgrind doesn't show any memory leaks, but I'm not sure that it is the best way to solve this problem. There are a couple of places that uses system_path, if this way will be good, i'll make another patches to fix this leak. Waiting for feedback. Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com> ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 13:56 GIT: [PATCH] exec_cmd: system_path memory leak fix 0xAX @ 2014-11-23 13:56 ` 0xAX 2014-11-23 14:01 ` 0xAX 2014-11-23 18:51 ` Junio C Hamano 2014-11-23 18:28 ` GIT: [PATCH] exec_cmd: system_path memory leak fix Junio C Hamano 1 sibling, 2 replies; 33+ messages in thread From: 0xAX @ 2014-11-23 13:56 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, 0xAX Signed-off-by: 0xAX <kuleshovmail@gmail.com> --- exec_cmd.c | 25 ++++++++++++++++--------- exec_cmd.h | 4 ++-- git.c | 12 +++++++++--- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/exec_cmd.c b/exec_cmd.c index 698e752..08f8f80 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -6,7 +6,7 @@ static const char *argv_exec_path; static const char *argv0_path; -const char *system_path(const char *path) +char *system_path(const char *path) { #ifdef RUNTIME_PREFIX static const char *prefix; @@ -14,9 +14,10 @@ const char *system_path(const char *path) static const char *prefix = PREFIX; #endif struct strbuf d = STRBUF_INIT; + char *new_path = NULL; if (is_absolute_path(path)) - return path; + return strdup(path); #ifdef RUNTIME_PREFIX assert(argv0_path); @@ -32,10 +33,13 @@ const char *system_path(const char *path) "Using static fallback '%s'.\n", prefix); } #endif - strbuf_addf(&d, "%s/%s", prefix, path); - path = strbuf_detach(&d, NULL); - return path; + new_path = malloc((strlen(prefix) + strlen(path)) + 2); + sprintf(new_path, "%s/%s", prefix, path); + + strbuf_release(&d); + + return new_path; } const char *git_extract_argv0_path(const char *argv0) @@ -68,16 +72,16 @@ void git_set_argv_exec_path(const char *exec_path) /* Returns the highest-priority, location to look for git programs. */ -const char *git_exec_path(void) +char *git_exec_path(void) { const char *env; if (argv_exec_path) - return argv_exec_path; + return strdup(argv_exec_path); env = getenv(EXEC_PATH_ENVIRONMENT); if (env && *env) { - return env; + return strdup(env); } return system_path(GIT_EXEC_PATH); @@ -96,7 +100,9 @@ void setup_path(void) const char *old_path = getenv("PATH"); struct strbuf new_path = STRBUF_INIT; - add_path(&new_path, git_exec_path()); + char *exec_path = git_exec_path(); + + add_path(&new_path, exec_path); add_path(&new_path, argv0_path); if (old_path) @@ -107,6 +113,7 @@ void setup_path(void) setenv("PATH", new_path.buf, 1); strbuf_release(&new_path); + free(exec_path); } const char **prepare_git_cmd(const char **argv) diff --git a/exec_cmd.h b/exec_cmd.h index e4c9702..03c8599 100644 --- a/exec_cmd.h +++ b/exec_cmd.h @@ -3,12 +3,12 @@ extern void git_set_argv_exec_path(const char *exec_path); extern const char *git_extract_argv0_path(const char *path); -extern const char *git_exec_path(void); +extern char *git_exec_path(void); extern void setup_path(void); extern const char **prepare_git_cmd(const char **argv); extern int execv_git_cmd(const char **argv); /* NULL terminated */ LAST_ARG_MUST_BE_NULL extern int execl_git_cmd(const char *cmd, ...); -extern const char *system_path(const char *path); +extern char *system_path(const char *path); #endif /* GIT_EXEC_CMD_H */ diff --git a/git.c b/git.c index 82d7a1c..499dc2a 100644 --- a/git.c +++ b/git.c @@ -99,13 +99,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) exit(0); } } else if (!strcmp(cmd, "--html-path")) { - puts(system_path(GIT_HTML_PATH)); + char *git_html_path = system_path(GIT_HTML_PATH); + puts(git_html_path); + free(git_html_path); exit(0); } else if (!strcmp(cmd, "--man-path")) { - puts(system_path(GIT_MAN_PATH)); + char *git_man_path = system_path(GIT_MAN_PATH); + puts(git_man_path); + free(git_man_path); exit(0); } else if (!strcmp(cmd, "--info-path")) { - puts(system_path(GIT_INFO_PATH)); + char *git_info_path = system_path(GIT_INFO_PATH); + puts(git_info_path); + free(git_info_path); exit(0); } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { use_pager = 1; -- 2.2.0.rc3.191.gfdea3f0 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 13:56 ` 0xAX @ 2014-11-23 14:01 ` 0xAX 2014-11-23 18:51 ` Junio C Hamano 1 sibling, 0 replies; 33+ messages in thread From: 0xAX @ 2014-11-23 14:01 UTC (permalink / raw) To: git; +Cc: Junio C Hamano For example some testing with valgrind: 1. pu (952f0aaadff4afca1497450911587a973c8cca02) valgrind git help ==27034== Memcheck, a memory error detector ==27034== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. ==27034== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info ==27034== Command: git help ==27034== ==27034== HERE GIT HELP OUTPUT ==27034== ==27034== HEAP SUMMARY: ==27034== in use at exit: 65 bytes in 1 blocks ==27034== total heap usage: 71 allocs, 70 frees, 11,928 bytes allocated ==27034== ==27034== LEAK SUMMARY: ==27034== definitely lost: 65 bytes in 1 blocks ==27034== indirectly lost: 0 bytes in 0 blocks ==27034== possibly lost: 0 bytes in 0 blocks ==27034== still reachable: 0 bytes in 0 blocks ==27034== suppressed: 0 bytes in 0 blocks ==27034== Rerun with --leak-check=full to see details of leaked memory ==27034== ==27034== For counts of detected and suppressed errors, rerun with: -v ==27034== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) 2. With patch: valgrind git help ==28945== Memcheck, a memory error detector ==28945== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. ==28945== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info ==28945== Command: git help ==28945== ==28945== HERE git help OUTPUT ==28945== ==28945== HEAP SUMMARY: ==28945== in use at exit: 0 bytes in 0 blocks ==28945== total heap usage: 72 allocs, 72 frees, 11,956 bytes allocated ==28945== ==28945== All heap blocks were freed -- no leaks are possible ==28945== ==28945== For counts of detected and suppressed errors, rerun with: -v ==28945== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) Of course it is very small leak, but i just started to deep in git source code. Hope i will be useful for community. Thank you. 0xAX <kuleshovmail@gmail.com> @ 2014-11-23 19:56 ALMT: > Signed-off-by: 0xAX <kuleshovmail@gmail.com> > --- > exec_cmd.c | 25 ++++++++++++++++--------- > exec_cmd.h | 4 ++-- > git.c | 12 +++++++++--- > 3 files changed, 27 insertions(+), 14 deletions(-) > > diff --git a/exec_cmd.c b/exec_cmd.c > index 698e752..08f8f80 100644 > --- a/exec_cmd.c > +++ b/exec_cmd.c > @@ -6,7 +6,7 @@ > static const char *argv_exec_path; > static const char *argv0_path; > > -const char *system_path(const char *path) > +char *system_path(const char *path) > { > #ifdef RUNTIME_PREFIX > static const char *prefix; > @@ -14,9 +14,10 @@ const char *system_path(const char *path) > static const char *prefix = PREFIX; > #endif > struct strbuf d = STRBUF_INIT; > + char *new_path = NULL; > > if (is_absolute_path(path)) > - return path; > + return strdup(path); > > #ifdef RUNTIME_PREFIX > assert(argv0_path); > @@ -32,10 +33,13 @@ const char *system_path(const char *path) > "Using static fallback '%s'.\n", prefix); > } > #endif > - > strbuf_addf(&d, "%s/%s", prefix, path); > - path = strbuf_detach(&d, NULL); > - return path; > + new_path = malloc((strlen(prefix) + strlen(path)) + 2); > + sprintf(new_path, "%s/%s", prefix, path); > + > + strbuf_release(&d); > + > + return new_path; > } > > const char *git_extract_argv0_path(const char *argv0) > @@ -68,16 +72,16 @@ void git_set_argv_exec_path(const char *exec_path) > > > /* Returns the highest-priority, location to look for git programs. */ > -const char *git_exec_path(void) > +char *git_exec_path(void) > { > const char *env; > > if (argv_exec_path) > - return argv_exec_path; > + return strdup(argv_exec_path); > > env = getenv(EXEC_PATH_ENVIRONMENT); > if (env && *env) { > - return env; > + return strdup(env); > } > > return system_path(GIT_EXEC_PATH); > @@ -96,7 +100,9 @@ void setup_path(void) > const char *old_path = getenv("PATH"); > struct strbuf new_path = STRBUF_INIT; > > - add_path(&new_path, git_exec_path()); > + char *exec_path = git_exec_path(); > + > + add_path(&new_path, exec_path); > add_path(&new_path, argv0_path); > > if (old_path) > @@ -107,6 +113,7 @@ void setup_path(void) > setenv("PATH", new_path.buf, 1); > > strbuf_release(&new_path); > + free(exec_path); > } > > const char **prepare_git_cmd(const char **argv) > diff --git a/exec_cmd.h b/exec_cmd.h > index e4c9702..03c8599 100644 > --- a/exec_cmd.h > +++ b/exec_cmd.h > @@ -3,12 +3,12 @@ > > extern void git_set_argv_exec_path(const char *exec_path); > extern const char *git_extract_argv0_path(const char *path); > -extern const char *git_exec_path(void); > +extern char *git_exec_path(void); > extern void setup_path(void); > extern const char **prepare_git_cmd(const char **argv); > extern int execv_git_cmd(const char **argv); /* NULL terminated */ > LAST_ARG_MUST_BE_NULL > extern int execl_git_cmd(const char *cmd, ...); > -extern const char *system_path(const char *path); > +extern char *system_path(const char *path); > > #endif /* GIT_EXEC_CMD_H */ > diff --git a/git.c b/git.c > index 82d7a1c..499dc2a 100644 > --- a/git.c > +++ b/git.c > @@ -99,13 +99,19 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) > exit(0); > } > } else if (!strcmp(cmd, "--html-path")) { > - puts(system_path(GIT_HTML_PATH)); > + char *git_html_path = system_path(GIT_HTML_PATH); > + puts(git_html_path); > + free(git_html_path); > exit(0); > } else if (!strcmp(cmd, "--man-path")) { > - puts(system_path(GIT_MAN_PATH)); > + char *git_man_path = system_path(GIT_MAN_PATH); > + puts(git_man_path); > + free(git_man_path); > exit(0); > } else if (!strcmp(cmd, "--info-path")) { > - puts(system_path(GIT_INFO_PATH)); > + char *git_info_path = system_path(GIT_INFO_PATH); > + puts(git_info_path); > + free(git_info_path); > exit(0); > } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { > use_pager = 1; -- Best regards. 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 13:56 ` 0xAX 2014-11-23 14:01 ` 0xAX @ 2014-11-23 18:51 ` Junio C Hamano 2014-11-23 19:06 ` Alex Kuleshov 2014-11-23 19:19 ` Alex Kuleshov 1 sibling, 2 replies; 33+ messages in thread From: Junio C Hamano @ 2014-11-23 18:51 UTC (permalink / raw) To: 0xAX; +Cc: git 0xAX <kuleshovmail@gmail.com> writes: > Signed-off-by: 0xAX <kuleshovmail@gmail.com> > --- The comment on names I've already mentioned elsewhere. You need a better explanation than a "no log message", as you are not doing "system-path memory leak fix". You are doing a lot more. Perhaps the story would start like this: system_path(): make the callers own the returned string The function sometimes returns a newly allocated string and sometimes returns a borrowed string, the latter of which the callers must not free(). The existing callers all assume that the return value belongs to the callee and most of them copy it with strdup() when they want to keep it around. They end up leaking the returned copy when the callee returned a new string. Change the contract between the callers and system_path() to make the returned string owned by the callers; they are responsible for freeing it when done, but they do not have to make their own copy to store it away. This accidentally fixes some unsafe callers as well. For example, ... > exec_cmd.c | 25 ++++++++++++++++--------- > exec_cmd.h | 4 ++-- > git.c | 12 +++++++++--- Even though I said that this changes the contract between the caller and the callee and make things wasteful for some, I personally think it is going in the right direction. The change accidentally fixes some unsafe callers. For example, the first hit from "git grep system_path" is this: attr.c- static const char *system_wide; attr.c- if (!system_wide) attr.c: system_wide = system_path(ETC_GITATTRIBUTES); attr.c- return system_wide; This is obviously unsafe for a volatile return value from the callee and needs to have strdup() on it, but with the patch there no longer is need for such a caller-side strdup(). But this change also introduces new bugs, I think. For example, the second hit from "git grep system_path" is this: builtin/help.c: strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); Now the caller owns and is responsible for freeing the returned value, but without opening the file in question in an editor or a pager we can tell immediately that there is no way this line is not adding a new memory leak. > index 698e752..08f8f80 100644 > --- a/exec_cmd.c > +++ b/exec_cmd.c > @@ -6,7 +6,7 @@ > static const char *argv_exec_path; > static const char *argv0_path; > > -const char *system_path(const char *path) > +char *system_path(const char *path) > { > #ifdef RUNTIME_PREFIX > static const char *prefix; > @@ -14,9 +14,10 @@ const char *system_path(const char *path) > static const char *prefix = PREFIX; > #endif > struct strbuf d = STRBUF_INIT; > + char *new_path = NULL; > > if (is_absolute_path(path)) > - return path; > + return strdup(path); > > #ifdef RUNTIME_PREFIX > assert(argv0_path); > @@ -32,10 +33,13 @@ const char *system_path(const char *path) > "Using static fallback '%s'.\n", prefix); > } > #endif > - > strbuf_addf(&d, "%s/%s", prefix, path); > - path = strbuf_detach(&d, NULL); > - return path; > + new_path = malloc((strlen(prefix) + strlen(path)) + 2); > + sprintf(new_path, "%s/%s", prefix, path); > + > + strbuf_release(&d); > + > + return new_path; Are you duplicating what strbuf_addf() is doing on the strbuf d, manually creating the same in new_path, while discarding what the existing code you did not remove with this patch already computed? Isn't it sufficient to add strdup(path) for the absolute case and do nothing else to this function? I have no idea what you are doing here. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 18:51 ` Junio C Hamano @ 2014-11-23 19:06 ` Alex Kuleshov 2014-11-23 19:19 ` Alex Kuleshov 1 sibling, 0 replies; 33+ messages in thread From: Alex Kuleshov @ 2014-11-23 19:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Junio C Hamano <gitster@pobox.com> @ 2014-11-24 00:51 ALMT: > 0xAX <kuleshovmail@gmail.com> writes: > >> Signed-off-by: 0xAX <kuleshovmail@gmail.com> >> --- > > The comment on names I've already mentioned elsewhere. Yes, i understand about names. > > You need a better explanation than a "no log message", as you are > not doing "system-path memory leak fix". > > You are doing a lot more. Perhaps the story would start like this: > > system_path(): make the callers own the returned string Did it. > > The function sometimes returns a newly allocated string and > sometimes returns a borrowed string, the latter of which the > callers must not free(). > > The existing callers all assume that the return value belongs to > the callee and most of them copy it with strdup() when they want > to keep it around. They end up leaking the returned copy when > the callee returned a new string. > > Change the contract between the callers and system_path() to > make the returned string owned by the callers; they are > responsible for freeing it when done, but they do not have to > make their own copy to store it away. Yes you're right, i just started to read git source code some days ago, and it's hard to understand in some places for the start. Now i see it, thanks for explanation. > > This accidentally fixes some unsafe callers as well. For > example, ... > > >> exec_cmd.c | 25 ++++++++++++++++--------- >> exec_cmd.h | 4 ++-- >> git.c | 12 +++++++++--- > > Even though I said that this changes the contract between the caller > and the callee and make things wasteful for some, I personally think > it is going in the right direction. > > The change accidentally fixes some unsafe callers. For example, the > first hit from "git grep system_path" is this: > > attr.c- static const char *system_wide; > attr.c- if (!system_wide) > attr.c: system_wide = system_path(ETC_GITATTRIBUTES); > attr.c- return system_wide; > > This is obviously unsafe for a volatile return value from the callee > and needs to have strdup() on it, but with the patch there no longer > is need for such a caller-side strdup(). > > But this change also introduces new bugs, I think. For example, the > second hit from "git grep system_path" is this: > > builtin/help.c: strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); > > Now the caller owns and is responsible for freeing the returned > value, but without opening the file in question in an editor or a > pager we can tell immediately that there is no way this line is not > adding a new memory leak. > >> index 698e752..08f8f80 100644 >> --- a/exec_cmd.c >> +++ b/exec_cmd.c >> @@ -6,7 +6,7 @@ >> static const char *argv_exec_path; >> static const char *argv0_path; >> >> -const char *system_path(const char *path) >> +char *system_path(const char *path) >> { >> #ifdef RUNTIME_PREFIX >> static const char *prefix; >> @@ -14,9 +14,10 @@ const char *system_path(const char *path) >> static const char *prefix = PREFIX; >> #endif >> struct strbuf d = STRBUF_INIT; >> + char *new_path = NULL; >> >> if (is_absolute_path(path)) >> - return path; >> + return strdup(path); >> >> #ifdef RUNTIME_PREFIX >> assert(argv0_path); >> @@ -32,10 +33,13 @@ const char *system_path(const char *path) >> "Using static fallback '%s'.\n", prefix); >> } >> #endif >> - >> strbuf_addf(&d, "%s/%s", prefix, path); >> - path = strbuf_detach(&d, NULL); >> - return path; >> + new_path = malloc((strlen(prefix) + strlen(path)) + 2); >> + sprintf(new_path, "%s/%s", prefix, path); >> + >> + strbuf_release(&d); >> + >> + return new_path; > > Are you duplicating what strbuf_addf() is doing on the strbuf d, > manually creating the same in new_path, while discarding what the > existing code you did not remove with this patch already computed? > > Isn't it sufficient to add strdup(path) for the absolute case and do > nothing else to this function? I have no idea what you are doing > here. I have added changes from your previous feedback, how can I attach second (changed) patch to this mail thread with git send-email? -- Best regards. 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 18:51 ` Junio C Hamano 2014-11-23 19:06 ` Alex Kuleshov @ 2014-11-23 19:19 ` Alex Kuleshov 2014-11-23 19:42 ` Jeff King ` (2 more replies) 1 sibling, 3 replies; 33+ messages in thread From: Alex Kuleshov @ 2014-11-23 19:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com> --- exec_cmd.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/exec_cmd.c b/exec_cmd.c index 698e752..7ed9bcc 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -13,7 +13,7 @@ const char *system_path(const char *path) #else static const char *prefix = PREFIX; #endif - struct strbuf d = STRBUF_INIT; + static struct strbuf d = STRBUF_INIT; if (is_absolute_path(path)) return path; @@ -34,8 +34,7 @@ const char *system_path(const char *path) #endif strbuf_addf(&d, "%s/%s", prefix, path); - path = strbuf_detach(&d, NULL); - return path; + return d.buf; } const char *git_extract_argv0_path(const char *argv0) @@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path) void setup_path(void) { const char *old_path = getenv("PATH"); - struct strbuf new_path = STRBUF_INIT; + static struct strbuf new_path = STRBUF_INIT; add_path(&new_path, git_exec_path()); add_path(&new_path, argv0_path); -- 2.2.0.rc3.190.g952f0aa.dirty ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 19:19 ` Alex Kuleshov @ 2014-11-23 19:42 ` Jeff King 2014-11-23 20:07 ` Eric Sunshine 2014-11-23 21:58 ` Junio C Hamano 2 siblings, 0 replies; 33+ messages in thread From: Jeff King @ 2014-11-23 19:42 UTC (permalink / raw) To: Alex Kuleshov; +Cc: Junio C Hamano, git On Mon, Nov 24, 2014 at 01:19:29AM +0600, Alex Kuleshov wrote: > > Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com> > --- > exec_cmd.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/exec_cmd.c b/exec_cmd.c > index 698e752..7ed9bcc 100644 > --- a/exec_cmd.c > +++ b/exec_cmd.c > @@ -13,7 +13,7 @@ const char *system_path(const char *path) > #else > static const char *prefix = PREFIX; > #endif > - struct strbuf d = STRBUF_INIT; > + static struct strbuf d = STRBUF_INIT; > > if (is_absolute_path(path)) > return path; > @@ -34,8 +34,7 @@ const char *system_path(const char *path) > #endif > > strbuf_addf(&d, "%s/%s", prefix, path); > - path = strbuf_detach(&d, NULL); > - return path; > + return d.buf; > } If I am reading this right, calls to system_path() will always reuse the same buffer, even if they are called with another "path" argument. So all callers must make sure to make a copy if they are going to hold on to it for a long time. Grepping for callers shows us saving the result to a static variable in at least git_etc_gitattributes, copy_templates, and get_html_page_path. Don't these all need to learn to xstrdup the return value? -Peff ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 19:19 ` Alex Kuleshov 2014-11-23 19:42 ` Jeff King @ 2014-11-23 20:07 ` Eric Sunshine 2014-11-23 21:58 ` Junio C Hamano 2 siblings, 0 replies; 33+ messages in thread From: Eric Sunshine @ 2014-11-23 20:07 UTC (permalink / raw) To: Alex Kuleshov; +Cc: Junio C Hamano, git On Sun, Nov 23, 2014 at 2:19 PM, Alex Kuleshov <kuleshovmail@gmail.com> wrote: > > Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com> > --- > exec_cmd.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/exec_cmd.c b/exec_cmd.c > index 698e752..7ed9bcc 100644 > --- a/exec_cmd.c > +++ b/exec_cmd.c > @@ -13,7 +13,7 @@ const char *system_path(const char *path) > #else > static const char *prefix = PREFIX; > #endif > - struct strbuf d = STRBUF_INIT; > + static struct strbuf d = STRBUF_INIT; Curious. Did the unit tests pass with this change? In the original code, the strbuf starts out empty each time system_path() is called, and strbuf_addf() populates it. After your change, the strbuf starts empty only on the very first call, and subsequent calls append more content rather than replacing it. At the very least, to fix, you now need to invoke strbuf_reset() before invoking strbuf_addf(). > if (is_absolute_path(path)) > return path; > @@ -34,8 +34,7 @@ const char *system_path(const char *path) > #endif > > strbuf_addf(&d, "%s/%s", prefix, path); > - path = strbuf_detach(&d, NULL); > - return path; > + return d.buf; > } > > const char *git_extract_argv0_path(const char *argv0) > @@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path) > void setup_path(void) > { > const char *old_path = getenv("PATH"); > - struct strbuf new_path = STRBUF_INIT; > + static struct strbuf new_path = STRBUF_INIT; > > add_path(&new_path, git_exec_path()); > add_path(&new_path, argv0_path); Not sure what this change is about. The last couple lines of this function are: setenv("PATH", new_path.buf, 1); strbuf_release(&new_path); which means that the buffer held by the strbuf is being released anyhow, whether static or not. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 19:19 ` Alex Kuleshov 2014-11-23 19:42 ` Jeff King 2014-11-23 20:07 ` Eric Sunshine @ 2014-11-23 21:58 ` Junio C Hamano 2014-11-24 7:02 ` Alex Kuleshov 2 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2014-11-23 21:58 UTC (permalink / raw) To: Alex Kuleshov; +Cc: git Alex Kuleshov <kuleshovmail@gmail.com> writes: > Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com> So this one does not change the contract between the caller and the callee. It does not change the fact that you need to explain your change, though. The story should read something like this: system_path(): always return a volatile result The function sometimes returns a newly allocated string and sometimes returns a borrowed string, the latter of which the callers must not free(). The existing callers all assume that the return value belongs to the callee and most of them copy it with strdup() when they want to keep it around. They end up leaking the returned copy when the callee returned a new string. Make sure all callers receive a volatile result, so that they do not have to be worried about freeing the returned string. There however are existing callers that are already broken, for example, ... Fixing these callers are done as separate patches, that can be applied either before or after this patch. Personally, as I already said, I think the approach the previous version took (but not the implementation) to change the contract between the callers and this function is probably a good idea in the longer term. Leaking a bit by forgetting to convert a few callers to free the returned values will not affect the correctness, but making the returned value consistently more volatile will expose existing breakage more often and will break codepaths that happened to be working by accident. Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 21:58 ` Junio C Hamano @ 2014-11-24 7:02 ` Alex Kuleshov 2014-11-24 7:37 ` Junio C Hamano 0 siblings, 1 reply; 33+ messages in thread From: Alex Kuleshov @ 2014-11-24 7:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Jeff King: >If I am reading this right, calls to system_path() will always reuse the >same buffer, even if they are called with another "path" argument. So >all callers must make sure to make a copy if they are going to hold on >to it for a long time. Grepping for callers shows us saving the result >to a static variable in at least git_etc_gitattributes, copy_templates, >and get_html_page_path. Don't these all need to learn to xstrdup the >return value? Hello Jeff, yes as i wrote in previous message i saw that there some places uses system_path function and I just wanted to find correct way to solve problem with system_path, in near time i'll check and adapt if need all of this places for updated system_path. Eric Sunshine: >Curious. Did the unit tests pass with this change? Yes i launched unit tests and there are the same result as in origin/pu. >Not sure what this change is about. The last couple lines of this function are: > > setenv("PATH", new_path.buf, 1); > strbuf_release(&new_path); > >which means that the buffer held by the strbuf is being released >anyhow, whether static or not. Ah, yes, just a type, will update it. Junio C Hamano: >Fixing these callers are done as separate patches, that can be >applied either before or after this patch. How to do it better? Update this patch, fix all callers which broken and concat this patches to one or make separate patches? Thanks all for feedback. -- Best regards. 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-24 7:02 ` Alex Kuleshov @ 2014-11-24 7:37 ` Junio C Hamano 2014-11-24 8:12 ` Alex Kuleshov ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: Junio C Hamano @ 2014-11-24 7:37 UTC (permalink / raw) To: Alex Kuleshov; +Cc: git, Jeff King, Eric Sunshine [jc: added those who were mentioned but were missing back to Cc] On Sun, Nov 23, 2014 at 11:02 PM, Alex Kuleshov <kuleshovmail@gmail.com> wrote: > > Junio C Hamano: > >>Fixing these callers are done as separate patches, that can be >>applied either before or after this patch. > > How to do it better? Update this patch, fix all callers which broken and > concat this patches to one or make separate patches? As I said, I do not think the approach your v2 takes is better than the original approach to pass the ownership of the returned value to the caller. I'd say that a cleaned up v1 that makes sure it adds a necessary strdup() in the codepath where it returns an absolute pathname given as-is, with necessary changes to callers that do not currently free the received result to free it when they are done, and to callers that currently do strdup() of the received result not to do strdup(), in a single patch, would be the right thing to do. I think I already wrote the bulk of proposed commit message for you for such a change earlier ;-) The one that talks about changing the contract between the system_path() and its callers. Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-24 7:37 ` Junio C Hamano @ 2014-11-24 8:12 ` Alex Kuleshov 2014-11-24 13:11 ` Alexander Kuleshov 2014-11-24 14:00 ` Alex Kuleshov 2 siblings, 0 replies; 33+ messages in thread From: Alex Kuleshov @ 2014-11-24 8:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine Junio C Hamano <gitster@pobox.com> @ 2014-11-24 13:37 ALMT: > [jc: added those who were mentioned but were missing back to Cc] > > On Sun, Nov 23, 2014 at 11:02 PM, Alex Kuleshov <kuleshovmail@gmail.com> wrote: >> >> Junio C Hamano: >> >>>Fixing these callers are done as separate patches, that can be >>>applied either before or after this patch. >> >> How to do it better? Update this patch, fix all callers which broken and >> concat this patches to one or make separate patches? > > As I said, I do not think the approach your v2 takes is better than the original > approach to pass the ownership of the returned value to the caller. I'd say that > a cleaned up v1 that makes sure it adds a necessary strdup() in the codepath > where it returns an absolute pathname given as-is, with necessary changes to > callers that do not currently free the received result to free it when they are > done, and to callers that currently do strdup() of the received result not to do > strdup(), in a single patch, would be the right thing to do. > > I think I already wrote the bulk of proposed commit message for you for such > a change earlier ;-) The one that talks about changing the contract between the > system_path() and its callers. > > Thanks. OK, but i'm little confused now, long thread :) So what's our next strategy? I'll make system_path will return volatile value, so callers must free it by itself, but we have two types of callers: Callers which doesn't need to store system_path return value (like git --man-path and etc.. in git.c), and which need to store it (as in attr.c). We must to free system_path return value in first case, and does not need to create copy of returned string with xstrdup. Am i correct or i'm wrong with this? Thank you. -- Best regards. 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-24 7:37 ` Junio C Hamano 2014-11-24 8:12 ` Alex Kuleshov @ 2014-11-24 13:11 ` Alexander Kuleshov 2014-11-24 14:00 ` Alex Kuleshov 2 siblings, 0 replies; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-24 13:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine Now system_path returns path which is allocated string to callers; It prevents memory leaks in some places. All callers of system_path are owners of path string and they must release it. Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com> --- attr.c | 8 +++--- builtin/config.c | 73 +++++++++++++++++++++++++++++++++++++++++-------------- builtin/help.c | 30 ++++++++++++++++------- builtin/init-db.c | 12 +++++++-- cache.h | 4 +-- config.c | 10 +++++--- exec_cmd.c | 22 ++++++++--------- exec_cmd.h | 4 +-- git.c | 16 +++++++++--- wrapper.c | 4 ++- 10 files changed, 127 insertions(+), 56 deletions(-) diff --git a/attr.c b/attr.c index cd54697..f96ddb4 100644 --- a/attr.c +++ b/attr.c @@ -462,9 +462,9 @@ static void drop_attr_stack(void) } } -static const char *git_etc_gitattributes(void) +static char *git_etc_gitattributes(void) { - static const char *system_wide; + static char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITATTRIBUTES); return system_wide; @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void) attr_stack = elem; if (git_attr_system()) { - elem = read_attr_from_file(git_etc_gitattributes(), 1); + char *etc_attributes = git_etc_gitattributes(); + elem = read_attr_from_file(etc_attributes, 1); + free(etc_attributes); if (elem) { elem->origin = NULL; elem->prev = attr_stack; diff --git a/builtin/config.c b/builtin/config.c index 15a7bea..8ab3179 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (given_config_source.blob) die("editing blobs is not supported"); git_config(git_default_config, NULL); - config_file = xstrdup(given_config_source.file ? - given_config_source.file : git_path("config")); + + if (use_system_config) + config_file = given_config_source.file ? + given_config_source.file : git_path("config"); + else + config_file = xstrdup(given_config_source.file ? + given_config_source.file : git_path("config")); + if (use_global_config) { int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666); if (fd) { @@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (ret == CONFIG_NOTHING_SET) error("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s.", argv[0]); + + if (use_system_config) + free(given_config_source.file); return ret; } else if (actions == ACTION_SET_ALL) { + int ret; check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, argv[2], 0); + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], value, argv[2], 0); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_ADD) { + int ret; check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, - CONFIG_REGEX_NONE, 0); + ret = git_config_set_multivar_in_file(given_config_source.file, argv[0], + value, CONFIG_REGEX_NONE, 0); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_REPLACE_ALL) { + int ret; check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, argv[2], 1); + ret = git_config_set_multivar_in_file(given_config_source.file, argv[0], + value, argv[2], 1); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); @@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix) return get_urlmatch(argv[0], argv[1]); } else if (actions == ACTION_UNSET) { + int ret; check_write(); check_argc(argc, 1, 2); - if (argc == 2) - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], NULL, argv[1], 0); - else - return git_config_set_in_file(given_config_source.file, - argv[0], NULL); + if (argc == 2){ + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], NULL, argv[1], 0); + if (use_system_config) + free(given_config_source.file); + return ret; + } + else{ + ret = git_config_set_in_file(given_config_source.file, + argv[0], NULL); + if (use_system_config) + free(given_config_source.file); + return ret; + } } else if (actions == ACTION_UNSET_ALL) { + int ret; check_write(); check_argc(argc, 1, 2); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], NULL, argv[1], 1); + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], NULL, argv[1], 1); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_RENAME_SECTION) { int ret; check_write(); check_argc(argc, 2, 2); ret = git_config_rename_section_in_file(given_config_source.file, - argv[0], argv[1]); + argv[0], argv[1]); + if (use_system_config) + free(given_config_source.file); if (ret < 0) return ret; if (ret == 0) @@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_argc(argc, 1, 1); ret = git_config_rename_section_in_file(given_config_source.file, argv[0], NULL); + if (use_system_config) + free(given_config_source.file); if (ret < 0) return ret; if (ret == 0) diff --git a/builtin/help.c b/builtin/help.c index b3c818e..20ffbb1 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -326,7 +326,8 @@ static void setup_man_path(void) * old_path, the ':' at the end will let 'man' to try * system-wide paths after ours to find the manual page. If * there is old_path, we need ':' as delimiter. */ - strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); + char *git_man_path = system_path(GIT_MAN_PATH); + strbuf_addstr(&new_path, git_man_path); strbuf_addch(&new_path, ':'); if (old_path) strbuf_addstr(&new_path, old_path); @@ -334,6 +335,7 @@ static void setup_man_path(void) setenv("MANPATH", new_path.buf, 1); strbuf_release(&new_path); + free(git_man_path); } static void exec_viewer(const char *name, const char *page) @@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd) static void show_info_page(const char *git_cmd) { const char *page = cmd_to_page(git_cmd); - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1); + char *git_info_path = system_path(GIT_INFO_PATH); + setenv("INFOPATH", git_info_path, 1); + free(git_info_path); execlp("info", "info", "gitman", page, (char *)NULL); die(_("no info viewer handled the request")); } -static void get_html_page_path(struct strbuf *page_path, const char *page) +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path) { struct stat st; - if (!html_path) - html_path = system_path(GIT_HTML_PATH); /* Check that we have a git documentation directory. */ if (!strstr(html_path, "://")) { if (stat(mkpath("%s/git.html", html_path), &st) - || !S_ISREG(st.st_mode)) - die("'%s': not a documentation directory.", html_path); + || !S_ISREG(st.st_mode)){ + printf("'%s': not a documentation directory.\n", html_path); + free(html_path); + exit(1); + } } strbuf_init(page_path, 0); @@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page) * HTML. */ #ifndef open_html -static void open_html(const char *path) +static void open_html(char *path) { execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL); + free(path); } #endif @@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd) { const char *page = cmd_to_page(git_cmd); struct strbuf page_path; /* it leaks but we exec bellow */ + char* html_path = NULL; + + if (!html_path) + html_path = system_path(GIT_HTML_PATH); - get_html_page_path(&page_path, page); + get_html_page_path(&page_path, page, html_path); open_html(page_path.buf); } @@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix) return 0; } + +// git help --web commit diff --git a/builtin/init-db.c b/builtin/init-db.c index 2619ca5..6f36f3f 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir) template_dir = init_db_template_dir; if (!template_dir) template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR); - if (!template_dir[0]) + if (!template_dir[0]){ + if (!template_dir) + free((char*)template_dir); return; + } template_len = strlen(template_dir); - if (PATH_MAX <= (template_len+strlen("/config"))) + if (PATH_MAX <= (template_len+strlen("/config"))){ + if (!template_dir) + free((char*)template_dir); die(_("insanely long template path %s"), template_dir); + } strcpy(template_path, template_dir); if (template_path[template_len-1] != '/') { template_path[template_len++] = '/'; @@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir) dir = opendir(template_path); if (!dir) { warning(_("templates not found %s"), template_dir); + free((char*)template_dir); return; } @@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir) "a wrong format version %d from '%s'"), repository_format_version, template_dir); + free((char*)template_dir); closedir(dir); return; } diff --git a/cache.h b/cache.h index 1b05115..7048e7a 100644 --- a/cache.h +++ b/cache.h @@ -1354,7 +1354,7 @@ extern int update_server_info(int); struct git_config_source { unsigned int use_stdin:1; - const char *file; + char *file; const char *blob; }; @@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int); extern int git_config_rename_section(const char *, const char *); extern int git_config_rename_section_in_file(const char *, const char *, const char *); -extern const char *git_etc_gitconfig(void); +extern char *git_etc_gitconfig(void); extern int check_repository_format_version(const char *var, const char *value, void *cb); extern int git_env_bool(const char *, int); extern unsigned long git_env_ulong(const char *, unsigned long); diff --git a/config.c b/config.c index ae1398f..0ed8890 100644 --- a/config.c +++ b/config.c @@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn, return git_config_from_blob_sha1(fn, name, sha1, data); } -const char *git_etc_gitconfig(void) +char *git_etc_gitconfig(void) { - static const char *system_wide; + static char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITCONFIG); return system_wide; @@ -1172,13 +1172,15 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config) int ret = 0, found = 0; char *xdg_config = NULL; char *user_config = NULL; + char *git_etc_config = git_etc_gitconfig(); home_config_paths(&user_config, &xdg_config, "config"); - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) { - ret += git_config_from_file(fn, git_etc_gitconfig(), + if (git_etc_config && !access_or_die(git_etc_config, R_OK, 0)) { + ret += git_config_from_file(fn, git_etc_config, data); found += 1; + free(git_etc_config); } if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) { diff --git a/exec_cmd.c b/exec_cmd.c index 698e752..d35ecac 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -6,7 +6,7 @@ static const char *argv_exec_path; static const char *argv0_path; -const char *system_path(const char *path) +char *system_path(const char *path) { #ifdef RUNTIME_PREFIX static const char *prefix; @@ -16,7 +16,7 @@ const char *system_path(const char *path) struct strbuf d = STRBUF_INIT; if (is_absolute_path(path)) - return path; + return strdup(path); #ifdef RUNTIME_PREFIX assert(argv0_path); @@ -34,8 +34,7 @@ const char *system_path(const char *path) #endif strbuf_addf(&d, "%s/%s", prefix, path); - path = strbuf_detach(&d, NULL); - return path; + return d.buf; } const char *git_extract_argv0_path(const char *argv0) @@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path) /* Returns the highest-priority, location to look for git programs. */ -const char *git_exec_path(void) +char *git_exec_path(void) { - const char *env; + char *env; if (argv_exec_path) - return argv_exec_path; + return strdup(argv_exec_path); env = getenv(EXEC_PATH_ENVIRONMENT); - if (env && *env) { - return env; - } + if (env && *env) + return strdup(env); return system_path(GIT_EXEC_PATH); } @@ -96,7 +94,8 @@ void setup_path(void) const char *old_path = getenv("PATH"); struct strbuf new_path = STRBUF_INIT; - add_path(&new_path, git_exec_path()); + char *exec_path = git_exec_path(); + add_path(&new_path, exec_path); add_path(&new_path, argv0_path); if (old_path) @@ -107,6 +106,7 @@ void setup_path(void) setenv("PATH", new_path.buf, 1); strbuf_release(&new_path); + free(exec_path); } const char **prepare_git_cmd(const char **argv) diff --git a/exec_cmd.h b/exec_cmd.h index e4c9702..03c8599 100644 --- a/exec_cmd.h +++ b/exec_cmd.h @@ -3,12 +3,12 @@ extern void git_set_argv_exec_path(const char *exec_path); extern const char *git_extract_argv0_path(const char *path); -extern const char *git_exec_path(void); +extern char *git_exec_path(void); extern void setup_path(void); extern const char **prepare_git_cmd(const char **argv); extern int execv_git_cmd(const char **argv); /* NULL terminated */ LAST_ARG_MUST_BE_NULL extern int execl_git_cmd(const char *cmd, ...); -extern const char *system_path(const char *path); +extern char *system_path(const char *path); #endif /* GIT_EXEC_CMD_H */ diff --git a/git.c b/git.c index 82d7a1c..d01c4f1 100644 --- a/git.c +++ b/git.c @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) if (*cmd == '=') git_set_argv_exec_path(cmd + 1); else { - puts(git_exec_path()); + char *exec_path = git_exec_path(); + puts(exec_path); + free(exec_path); exit(0); } } else if (!strcmp(cmd, "--html-path")) { - puts(system_path(GIT_HTML_PATH)); + char *git_html_path = system_path(GIT_HTML_PATH); + puts(git_html_path); + free(git_html_path); exit(0); } else if (!strcmp(cmd, "--man-path")) { - puts(system_path(GIT_MAN_PATH)); + char *git_man_path = system_path(GIT_MAN_PATH); + puts(git_man_path); + free(git_man_path); exit(0); } else if (!strcmp(cmd, "--info-path")) { - puts(system_path(GIT_INFO_PATH)); + char *git_info_path = system_path(GIT_INFO_PATH); + puts(git_info_path); + free(git_info_path); exit(0); } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { use_pager = 1; diff --git a/wrapper.c b/wrapper.c index 007ec0d..ede9268 100644 --- a/wrapper.c +++ b/wrapper.c @@ -526,8 +526,10 @@ int access_or_warn(const char *path, int mode, unsigned flag) int access_or_die(const char *path, int mode, unsigned flag) { int ret = access(path, mode); - if (ret && !access_error_is_ok(errno, flag)) + if (ret && !access_error_is_ok(errno, flag)){ + free((char*)path); die_errno(_("unable to access '%s'"), path); + } return ret; } -- 2.2.0-rc3 ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] exec_cmd: system_path memory leak fix 2014-11-24 7:37 ` Junio C Hamano 2014-11-24 8:12 ` Alex Kuleshov 2014-11-24 13:11 ` Alexander Kuleshov @ 2014-11-24 14:00 ` Alex Kuleshov 2014-11-24 14:07 ` [PATCH] change contract between system_path and it's callers 0xAX 2 siblings, 1 reply; 33+ messages in thread From: Alex Kuleshov @ 2014-11-24 14:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine Now system_path returns path which is allocated string to callers; It prevents memory leaks in some places. All callers of system_path are owners of path string and they must release it. Added new parameter to wrapper.c/int access_or_die - etc_config, because only etc_config in this case use system_path and we don't know do we need to free path or not. Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com> --- attr.c | 8 +++--- builtin/config.c | 73 +++++++++++++++++++++++++++++++++++++++++-------------- builtin/help.c | 30 ++++++++++++++++------- builtin/init-db.c | 12 +++++++-- cache.h | 4 +-- config.c | 19 ++++++++------- exec_cmd.c | 22 ++++++++--------- exec_cmd.h | 4 +-- git-compat-util.h | 2 +- git.c | 16 +++++++++--- wrapper.c | 7 ++++-- 11 files changed, 134 insertions(+), 63 deletions(-) diff --git a/attr.c b/attr.c index cd54697..f96ddb4 100644 --- a/attr.c +++ b/attr.c @@ -462,9 +462,9 @@ static void drop_attr_stack(void) } } -static const char *git_etc_gitattributes(void) +static char *git_etc_gitattributes(void) { - static const char *system_wide; + static char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITATTRIBUTES); return system_wide; @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void) attr_stack = elem; if (git_attr_system()) { - elem = read_attr_from_file(git_etc_gitattributes(), 1); + char *etc_attributes = git_etc_gitattributes(); + elem = read_attr_from_file(etc_attributes, 1); + free(etc_attributes); if (elem) { elem->origin = NULL; elem->prev = attr_stack; diff --git a/builtin/config.c b/builtin/config.c index 15a7bea..266d42b 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (given_config_source.blob) die("editing blobs is not supported"); git_config(git_default_config, NULL); - config_file = xstrdup(given_config_source.file ? - given_config_source.file : git_path("config")); + + if (use_system_config) + config_file = given_config_source.file ? + given_config_source.file : git_path("config"); + else + config_file = xstrdup(given_config_source.file ? + given_config_source.file : git_path("config")); + if (use_global_config) { int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666); if (fd) { @@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (ret == CONFIG_NOTHING_SET) error("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s.", argv[0]); + + if (use_system_config) + free(given_config_source.file); return ret; } else if (actions == ACTION_SET_ALL) { + int ret; check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, argv[2], 0); + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], value, argv[2], 0); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_ADD) { + int ret; check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, - CONFIG_REGEX_NONE, 0); + ret = git_config_set_multivar_in_file(given_config_source.file, argv[0], + value, CONFIG_REGEX_NONE, 0); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_REPLACE_ALL) { + int ret; check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, argv[2], 1); + ret = git_config_set_multivar_in_file(given_config_source.file, argv[0], + value, argv[2], 1); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); @@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix) return get_urlmatch(argv[0], argv[1]); } else if (actions == ACTION_UNSET) { + int ret; check_write(); check_argc(argc, 1, 2); - if (argc == 2) - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], NULL, argv[1], 0); - else - return git_config_set_in_file(given_config_source.file, - argv[0], NULL); + if (argc == 2){ + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], NULL, argv[1], 0); + if (use_system_config) + free(given_config_source.file); + return ret; + } + else{ + ret = git_config_set_in_file(given_config_source.file, + argv[0], NULL); + if (use_system_config) + free(given_config_source.file); + return ret; + } } else if (actions == ACTION_UNSET_ALL) { + int ret; check_write(); check_argc(argc, 1, 2); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], NULL, argv[1], 1); + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], NULL, argv[1], 1); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_RENAME_SECTION) { int ret; check_write(); check_argc(argc, 2, 2); ret = git_config_rename_section_in_file(given_config_source.file, - argv[0], argv[1]); + argv[0], argv[1]); + if (use_system_config) + free(given_config_source.file); if (ret < 0) return ret; if (ret == 0) @@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_argc(argc, 1, 1); ret = git_config_rename_section_in_file(given_config_source.file, argv[0], NULL); + if (use_system_config) + free(given_config_source.file); if (ret < 0) return ret; if (ret == 0) diff --git a/builtin/help.c b/builtin/help.c index b3c818e..20ffbb1 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -326,7 +326,8 @@ static void setup_man_path(void) * old_path, the ':' at the end will let 'man' to try * system-wide paths after ours to find the manual page. If * there is old_path, we need ':' as delimiter. */ - strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); + char *git_man_path = system_path(GIT_MAN_PATH); + strbuf_addstr(&new_path, git_man_path); strbuf_addch(&new_path, ':'); if (old_path) strbuf_addstr(&new_path, old_path); @@ -334,6 +335,7 @@ static void setup_man_path(void) setenv("MANPATH", new_path.buf, 1); strbuf_release(&new_path); + free(git_man_path); } static void exec_viewer(const char *name, const char *page) @@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd) static void show_info_page(const char *git_cmd) { const char *page = cmd_to_page(git_cmd); - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1); + char *git_info_path = system_path(GIT_INFO_PATH); + setenv("INFOPATH", git_info_path, 1); + free(git_info_path); execlp("info", "info", "gitman", page, (char *)NULL); die(_("no info viewer handled the request")); } -static void get_html_page_path(struct strbuf *page_path, const char *page) +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path) { struct stat st; - if (!html_path) - html_path = system_path(GIT_HTML_PATH); /* Check that we have a git documentation directory. */ if (!strstr(html_path, "://")) { if (stat(mkpath("%s/git.html", html_path), &st) - || !S_ISREG(st.st_mode)) - die("'%s': not a documentation directory.", html_path); + || !S_ISREG(st.st_mode)){ + printf("'%s': not a documentation directory.\n", html_path); + free(html_path); + exit(1); + } } strbuf_init(page_path, 0); @@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page) * HTML. */ #ifndef open_html -static void open_html(const char *path) +static void open_html(char *path) { execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL); + free(path); } #endif @@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd) { const char *page = cmd_to_page(git_cmd); struct strbuf page_path; /* it leaks but we exec bellow */ + char* html_path = NULL; + + if (!html_path) + html_path = system_path(GIT_HTML_PATH); - get_html_page_path(&page_path, page); + get_html_page_path(&page_path, page, html_path); open_html(page_path.buf); } @@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix) return 0; } + +// git help --web commit diff --git a/builtin/init-db.c b/builtin/init-db.c index 2619ca5..6f36f3f 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir) template_dir = init_db_template_dir; if (!template_dir) template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR); - if (!template_dir[0]) + if (!template_dir[0]){ + if (!template_dir) + free((char*)template_dir); return; + } template_len = strlen(template_dir); - if (PATH_MAX <= (template_len+strlen("/config"))) + if (PATH_MAX <= (template_len+strlen("/config"))){ + if (!template_dir) + free((char*)template_dir); die(_("insanely long template path %s"), template_dir); + } strcpy(template_path, template_dir); if (template_path[template_len-1] != '/') { template_path[template_len++] = '/'; @@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir) dir = opendir(template_path); if (!dir) { warning(_("templates not found %s"), template_dir); + free((char*)template_dir); return; } @@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir) "a wrong format version %d from '%s'"), repository_format_version, template_dir); + free((char*)template_dir); closedir(dir); return; } diff --git a/cache.h b/cache.h index 1b05115..7048e7a 100644 --- a/cache.h +++ b/cache.h @@ -1354,7 +1354,7 @@ extern int update_server_info(int); struct git_config_source { unsigned int use_stdin:1; - const char *file; + char *file; const char *blob; }; @@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int); extern int git_config_rename_section(const char *, const char *); extern int git_config_rename_section_in_file(const char *, const char *, const char *); -extern const char *git_etc_gitconfig(void); +extern char *git_etc_gitconfig(void); extern int check_repository_format_version(const char *var, const char *value, void *cb); extern int git_env_bool(const char *, int); extern unsigned long git_env_ulong(const char *, unsigned long); diff --git a/config.c b/config.c index ae1398f..b71c0fb 100644 --- a/config.c +++ b/config.c @@ -122,7 +122,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc path = buf.buf; } - if (!access_or_die(path, R_OK, 0)) { + if (!access_or_die(path, R_OK, 0, 0)) { if (++inc->depth > MAX_INCLUDE_DEPTH) die(include_depth_advice, MAX_INCLUDE_DEPTH, path, cf && cf->name ? cf->name : "the command line"); @@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn, return git_config_from_blob_sha1(fn, name, sha1, data); } -const char *git_etc_gitconfig(void) +char *git_etc_gitconfig(void) { - static const char *system_wide; + static char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITCONFIG); return system_wide; @@ -1172,26 +1172,27 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config) int ret = 0, found = 0; char *xdg_config = NULL; char *user_config = NULL; + char *git_etc_config = git_etc_gitconfig(); home_config_paths(&user_config, &xdg_config, "config"); - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) { - ret += git_config_from_file(fn, git_etc_gitconfig(), - data); + if (git_etc_config && !access_or_die(git_etc_config, R_OK, 0, 1)) { + ret += git_config_from_file(fn, git_etc_config, data); found += 1; + free(git_etc_config); } - if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) { + if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK, 0)) { ret += git_config_from_file(fn, xdg_config, data); found += 1; } - if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) { + if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK, 0)) { ret += git_config_from_file(fn, user_config, data); found += 1; } - if (repo_config && !access_or_die(repo_config, R_OK, 0)) { + if (repo_config && !access_or_die(repo_config, R_OK, 0, 0)) { ret += git_config_from_file(fn, repo_config, data); found += 1; } diff --git a/exec_cmd.c b/exec_cmd.c index 698e752..d35ecac 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -6,7 +6,7 @@ static const char *argv_exec_path; static const char *argv0_path; -const char *system_path(const char *path) +char *system_path(const char *path) { #ifdef RUNTIME_PREFIX static const char *prefix; @@ -16,7 +16,7 @@ const char *system_path(const char *path) struct strbuf d = STRBUF_INIT; if (is_absolute_path(path)) - return path; + return strdup(path); #ifdef RUNTIME_PREFIX assert(argv0_path); @@ -34,8 +34,7 @@ const char *system_path(const char *path) #endif strbuf_addf(&d, "%s/%s", prefix, path); - path = strbuf_detach(&d, NULL); - return path; + return d.buf; } const char *git_extract_argv0_path(const char *argv0) @@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path) /* Returns the highest-priority, location to look for git programs. */ -const char *git_exec_path(void) +char *git_exec_path(void) { - const char *env; + char *env; if (argv_exec_path) - return argv_exec_path; + return strdup(argv_exec_path); env = getenv(EXEC_PATH_ENVIRONMENT); - if (env && *env) { - return env; - } + if (env && *env) + return strdup(env); return system_path(GIT_EXEC_PATH); } @@ -96,7 +94,8 @@ void setup_path(void) const char *old_path = getenv("PATH"); struct strbuf new_path = STRBUF_INIT; - add_path(&new_path, git_exec_path()); + char *exec_path = git_exec_path(); + add_path(&new_path, exec_path); add_path(&new_path, argv0_path); if (old_path) @@ -107,6 +106,7 @@ void setup_path(void) setenv("PATH", new_path.buf, 1); strbuf_release(&new_path); + free(exec_path); } const char **prepare_git_cmd(const char **argv) diff --git a/exec_cmd.h b/exec_cmd.h index e4c9702..03c8599 100644 --- a/exec_cmd.h +++ b/exec_cmd.h @@ -3,12 +3,12 @@ extern void git_set_argv_exec_path(const char *exec_path); extern const char *git_extract_argv0_path(const char *path); -extern const char *git_exec_path(void); +extern char *git_exec_path(void); extern void setup_path(void); extern const char **prepare_git_cmd(const char **argv); extern int execv_git_cmd(const char **argv); /* NULL terminated */ LAST_ARG_MUST_BE_NULL extern int execl_git_cmd(const char *cmd, ...); -extern const char *system_path(const char *path); +extern char *system_path(const char *path); #endif /* GIT_EXEC_CMD_H */ diff --git a/git-compat-util.h b/git-compat-util.h index 400e921..fcc88db 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -813,7 +813,7 @@ int remove_or_warn(unsigned int mode, const char *path); */ #define ACCESS_EACCES_OK (1U << 0) int access_or_warn(const char *path, int mode, unsigned flag); -int access_or_die(const char *path, int mode, unsigned flag); +int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config); /* Warn on an inaccessible file that ought to be accessible */ void warn_on_inaccessible(const char *path); diff --git a/git.c b/git.c index 82d7a1c..d01c4f1 100644 --- a/git.c +++ b/git.c @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) if (*cmd == '=') git_set_argv_exec_path(cmd + 1); else { - puts(git_exec_path()); + char *exec_path = git_exec_path(); + puts(exec_path); + free(exec_path); exit(0); } } else if (!strcmp(cmd, "--html-path")) { - puts(system_path(GIT_HTML_PATH)); + char *git_html_path = system_path(GIT_HTML_PATH); + puts(git_html_path); + free(git_html_path); exit(0); } else if (!strcmp(cmd, "--man-path")) { - puts(system_path(GIT_MAN_PATH)); + char *git_man_path = system_path(GIT_MAN_PATH); + puts(git_man_path); + free(git_man_path); exit(0); } else if (!strcmp(cmd, "--info-path")) { - puts(system_path(GIT_INFO_PATH)); + char *git_info_path = system_path(GIT_INFO_PATH); + puts(git_info_path); + free(git_info_path); exit(0); } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { use_pager = 1; diff --git a/wrapper.c b/wrapper.c index 007ec0d..f97adbc 100644 --- a/wrapper.c +++ b/wrapper.c @@ -523,11 +523,14 @@ int access_or_warn(const char *path, int mode, unsigned flag) return ret; } -int access_or_die(const char *path, int mode, unsigned flag) +int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config) { int ret = access(path, mode); - if (ret && !access_error_is_ok(errno, flag)) + if (ret && !access_error_is_ok(errno, flag)){ + if (etc_config) + free((char*)path); die_errno(_("unable to access '%s'"), path); + } return ret; } -- 2.2.0.rc3.191.g70a3888.dirty ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH] change contract between system_path and it's callers 2014-11-24 14:00 ` Alex Kuleshov @ 2014-11-24 14:07 ` 0xAX 2014-11-24 19:33 ` Re*: " Junio C Hamano 0 siblings, 1 reply; 33+ messages in thread From: 0xAX @ 2014-11-24 14:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine, 0xAX Now system_path returns path which is allocated string to callers; It prevents memory leaks in some places. All callers of system_path are owners of path string and they must release it. Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com> --- attr.c | 8 +++--- builtin/config.c | 73 +++++++++++++++++++++++++++++++++++++++++-------------- builtin/help.c | 30 ++++++++++++++++------- builtin/init-db.c | 12 +++++++-- cache.h | 4 +-- config.c | 19 ++++++++------- exec_cmd.c | 22 ++++++++--------- exec_cmd.h | 4 +-- git-compat-util.h | 2 +- git.c | 16 +++++++++--- wrapper.c | 7 ++++-- 11 files changed, 134 insertions(+), 63 deletions(-) diff --git a/attr.c b/attr.c index cd54697..f96ddb4 100644 --- a/attr.c +++ b/attr.c @@ -462,9 +462,9 @@ static void drop_attr_stack(void) } } -static const char *git_etc_gitattributes(void) +static char *git_etc_gitattributes(void) { - static const char *system_wide; + static char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITATTRIBUTES); return system_wide; @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void) attr_stack = elem; if (git_attr_system()) { - elem = read_attr_from_file(git_etc_gitattributes(), 1); + char *etc_attributes = git_etc_gitattributes(); + elem = read_attr_from_file(etc_attributes, 1); + free(etc_attributes); if (elem) { elem->origin = NULL; elem->prev = attr_stack; diff --git a/builtin/config.c b/builtin/config.c index 15a7bea..266d42b 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (given_config_source.blob) die("editing blobs is not supported"); git_config(git_default_config, NULL); - config_file = xstrdup(given_config_source.file ? - given_config_source.file : git_path("config")); + + if (use_system_config) + config_file = given_config_source.file ? + given_config_source.file : git_path("config"); + else + config_file = xstrdup(given_config_source.file ? + given_config_source.file : git_path("config")); + if (use_global_config) { int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666); if (fd) { @@ -600,29 +606,43 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (ret == CONFIG_NOTHING_SET) error("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s.", argv[0]); + + if (use_system_config) + free(given_config_source.file); return ret; } else if (actions == ACTION_SET_ALL) { + int ret; check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, argv[2], 0); + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], value, argv[2], 0); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_ADD) { + int ret; check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, - CONFIG_REGEX_NONE, 0); + ret = git_config_set_multivar_in_file(given_config_source.file, argv[0], + value, CONFIG_REGEX_NONE, 0); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_REPLACE_ALL) { + int ret; check_write(); check_argc(argc, 2, 3); value = normalize_value(argv[0], argv[1]); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], value, argv[2], 1); + ret = git_config_set_multivar_in_file(given_config_source.file, argv[0], + value, argv[2], 1); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); @@ -645,27 +665,42 @@ int cmd_config(int argc, const char **argv, const char *prefix) return get_urlmatch(argv[0], argv[1]); } else if (actions == ACTION_UNSET) { + int ret; check_write(); check_argc(argc, 1, 2); - if (argc == 2) - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], NULL, argv[1], 0); - else - return git_config_set_in_file(given_config_source.file, - argv[0], NULL); + if (argc == 2){ + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], NULL, argv[1], 0); + if (use_system_config) + free(given_config_source.file); + return ret; + } + else{ + ret = git_config_set_in_file(given_config_source.file, + argv[0], NULL); + if (use_system_config) + free(given_config_source.file); + return ret; + } } else if (actions == ACTION_UNSET_ALL) { + int ret; check_write(); check_argc(argc, 1, 2); - return git_config_set_multivar_in_file(given_config_source.file, - argv[0], NULL, argv[1], 1); + ret = git_config_set_multivar_in_file(given_config_source.file, + argv[0], NULL, argv[1], 1); + if (use_system_config) + free(given_config_source.file); + return ret; } else if (actions == ACTION_RENAME_SECTION) { int ret; check_write(); check_argc(argc, 2, 2); ret = git_config_rename_section_in_file(given_config_source.file, - argv[0], argv[1]); + argv[0], argv[1]); + if (use_system_config) + free(given_config_source.file); if (ret < 0) return ret; if (ret == 0) @@ -677,6 +712,8 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_argc(argc, 1, 1); ret = git_config_rename_section_in_file(given_config_source.file, argv[0], NULL); + if (use_system_config) + free(given_config_source.file); if (ret < 0) return ret; if (ret == 0) diff --git a/builtin/help.c b/builtin/help.c index b3c818e..20ffbb1 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -326,7 +326,8 @@ static void setup_man_path(void) * old_path, the ':' at the end will let 'man' to try * system-wide paths after ours to find the manual page. If * there is old_path, we need ':' as delimiter. */ - strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); + char *git_man_path = system_path(GIT_MAN_PATH); + strbuf_addstr(&new_path, git_man_path); strbuf_addch(&new_path, ':'); if (old_path) strbuf_addstr(&new_path, old_path); @@ -334,6 +335,7 @@ static void setup_man_path(void) setenv("MANPATH", new_path.buf, 1); strbuf_release(&new_path); + free(git_man_path); } static void exec_viewer(const char *name, const char *page) @@ -372,22 +374,25 @@ static void show_man_page(const char *git_cmd) static void show_info_page(const char *git_cmd) { const char *page = cmd_to_page(git_cmd); - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1); + char *git_info_path = system_path(GIT_INFO_PATH); + setenv("INFOPATH", git_info_path, 1); + free(git_info_path); execlp("info", "info", "gitman", page, (char *)NULL); die(_("no info viewer handled the request")); } -static void get_html_page_path(struct strbuf *page_path, const char *page) +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path) { struct stat st; - if (!html_path) - html_path = system_path(GIT_HTML_PATH); /* Check that we have a git documentation directory. */ if (!strstr(html_path, "://")) { if (stat(mkpath("%s/git.html", html_path), &st) - || !S_ISREG(st.st_mode)) - die("'%s': not a documentation directory.", html_path); + || !S_ISREG(st.st_mode)){ + printf("'%s': not a documentation directory.\n", html_path); + free(html_path); + exit(1); + } } strbuf_init(page_path, 0); @@ -400,9 +405,10 @@ static void get_html_page_path(struct strbuf *page_path, const char *page) * HTML. */ #ifndef open_html -static void open_html(const char *path) +static void open_html(char *path) { execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL); + free(path); } #endif @@ -410,8 +416,12 @@ static void show_html_page(const char *git_cmd) { const char *page = cmd_to_page(git_cmd); struct strbuf page_path; /* it leaks but we exec bellow */ + char* html_path = NULL; + + if (!html_path) + html_path = system_path(GIT_HTML_PATH); - get_html_page_path(&page_path, page); + get_html_page_path(&page_path, page, html_path); open_html(page_path.buf); } @@ -512,3 +522,5 @@ int cmd_help(int argc, const char **argv, const char *prefix) return 0; } + +// git help --web commit diff --git a/builtin/init-db.c b/builtin/init-db.c index 2619ca5..6f36f3f 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir) template_dir = init_db_template_dir; if (!template_dir) template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR); - if (!template_dir[0]) + if (!template_dir[0]){ + if (!template_dir) + free((char*)template_dir); return; + } template_len = strlen(template_dir); - if (PATH_MAX <= (template_len+strlen("/config"))) + if (PATH_MAX <= (template_len+strlen("/config"))){ + if (!template_dir) + free((char*)template_dir); die(_("insanely long template path %s"), template_dir); + } strcpy(template_path, template_dir); if (template_path[template_len-1] != '/') { template_path[template_len++] = '/'; @@ -139,6 +145,7 @@ static void copy_templates(const char *template_dir) dir = opendir(template_path); if (!dir) { warning(_("templates not found %s"), template_dir); + free((char*)template_dir); return; } @@ -155,6 +162,7 @@ static void copy_templates(const char *template_dir) "a wrong format version %d from '%s'"), repository_format_version, template_dir); + free((char*)template_dir); closedir(dir); return; } diff --git a/cache.h b/cache.h index 1b05115..7048e7a 100644 --- a/cache.h +++ b/cache.h @@ -1354,7 +1354,7 @@ extern int update_server_info(int); struct git_config_source { unsigned int use_stdin:1; - const char *file; + char *file; const char *blob; }; @@ -1386,7 +1386,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int); extern int git_config_rename_section(const char *, const char *); extern int git_config_rename_section_in_file(const char *, const char *, const char *); -extern const char *git_etc_gitconfig(void); +extern char *git_etc_gitconfig(void); extern int check_repository_format_version(const char *var, const char *value, void *cb); extern int git_env_bool(const char *, int); extern unsigned long git_env_ulong(const char *, unsigned long); diff --git a/config.c b/config.c index ae1398f..b71c0fb 100644 --- a/config.c +++ b/config.c @@ -122,7 +122,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc path = buf.buf; } - if (!access_or_die(path, R_OK, 0)) { + if (!access_or_die(path, R_OK, 0, 0)) { if (++inc->depth > MAX_INCLUDE_DEPTH) die(include_depth_advice, MAX_INCLUDE_DEPTH, path, cf && cf->name ? cf->name : "the command line"); @@ -1132,9 +1132,9 @@ static int git_config_from_blob_ref(config_fn_t fn, return git_config_from_blob_sha1(fn, name, sha1, data); } -const char *git_etc_gitconfig(void) +char *git_etc_gitconfig(void) { - static const char *system_wide; + static char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITCONFIG); return system_wide; @@ -1172,26 +1172,27 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config) int ret = 0, found = 0; char *xdg_config = NULL; char *user_config = NULL; + char *git_etc_config = git_etc_gitconfig(); home_config_paths(&user_config, &xdg_config, "config"); - if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) { - ret += git_config_from_file(fn, git_etc_gitconfig(), - data); + if (git_etc_config && !access_or_die(git_etc_config, R_OK, 0, 1)) { + ret += git_config_from_file(fn, git_etc_config, data); found += 1; + free(git_etc_config); } - if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) { + if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK, 0)) { ret += git_config_from_file(fn, xdg_config, data); found += 1; } - if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) { + if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK, 0)) { ret += git_config_from_file(fn, user_config, data); found += 1; } - if (repo_config && !access_or_die(repo_config, R_OK, 0)) { + if (repo_config && !access_or_die(repo_config, R_OK, 0, 0)) { ret += git_config_from_file(fn, repo_config, data); found += 1; } diff --git a/exec_cmd.c b/exec_cmd.c index 698e752..d35ecac 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -6,7 +6,7 @@ static const char *argv_exec_path; static const char *argv0_path; -const char *system_path(const char *path) +char *system_path(const char *path) { #ifdef RUNTIME_PREFIX static const char *prefix; @@ -16,7 +16,7 @@ const char *system_path(const char *path) struct strbuf d = STRBUF_INIT; if (is_absolute_path(path)) - return path; + return strdup(path); #ifdef RUNTIME_PREFIX assert(argv0_path); @@ -34,8 +34,7 @@ const char *system_path(const char *path) #endif strbuf_addf(&d, "%s/%s", prefix, path); - path = strbuf_detach(&d, NULL); - return path; + return d.buf; } const char *git_extract_argv0_path(const char *argv0) @@ -68,17 +67,16 @@ void git_set_argv_exec_path(const char *exec_path) /* Returns the highest-priority, location to look for git programs. */ -const char *git_exec_path(void) +char *git_exec_path(void) { - const char *env; + char *env; if (argv_exec_path) - return argv_exec_path; + return strdup(argv_exec_path); env = getenv(EXEC_PATH_ENVIRONMENT); - if (env && *env) { - return env; - } + if (env && *env) + return strdup(env); return system_path(GIT_EXEC_PATH); } @@ -96,7 +94,8 @@ void setup_path(void) const char *old_path = getenv("PATH"); struct strbuf new_path = STRBUF_INIT; - add_path(&new_path, git_exec_path()); + char *exec_path = git_exec_path(); + add_path(&new_path, exec_path); add_path(&new_path, argv0_path); if (old_path) @@ -107,6 +106,7 @@ void setup_path(void) setenv("PATH", new_path.buf, 1); strbuf_release(&new_path); + free(exec_path); } const char **prepare_git_cmd(const char **argv) diff --git a/exec_cmd.h b/exec_cmd.h index e4c9702..03c8599 100644 --- a/exec_cmd.h +++ b/exec_cmd.h @@ -3,12 +3,12 @@ extern void git_set_argv_exec_path(const char *exec_path); extern const char *git_extract_argv0_path(const char *path); -extern const char *git_exec_path(void); +extern char *git_exec_path(void); extern void setup_path(void); extern const char **prepare_git_cmd(const char **argv); extern int execv_git_cmd(const char **argv); /* NULL terminated */ LAST_ARG_MUST_BE_NULL extern int execl_git_cmd(const char *cmd, ...); -extern const char *system_path(const char *path); +extern char *system_path(const char *path); #endif /* GIT_EXEC_CMD_H */ diff --git a/git-compat-util.h b/git-compat-util.h index 400e921..fcc88db 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -813,7 +813,7 @@ int remove_or_warn(unsigned int mode, const char *path); */ #define ACCESS_EACCES_OK (1U << 0) int access_or_warn(const char *path, int mode, unsigned flag); -int access_or_die(const char *path, int mode, unsigned flag); +int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config); /* Warn on an inaccessible file that ought to be accessible */ void warn_on_inaccessible(const char *path); diff --git a/git.c b/git.c index 82d7a1c..d01c4f1 100644 --- a/git.c +++ b/git.c @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) if (*cmd == '=') git_set_argv_exec_path(cmd + 1); else { - puts(git_exec_path()); + char *exec_path = git_exec_path(); + puts(exec_path); + free(exec_path); exit(0); } } else if (!strcmp(cmd, "--html-path")) { - puts(system_path(GIT_HTML_PATH)); + char *git_html_path = system_path(GIT_HTML_PATH); + puts(git_html_path); + free(git_html_path); exit(0); } else if (!strcmp(cmd, "--man-path")) { - puts(system_path(GIT_MAN_PATH)); + char *git_man_path = system_path(GIT_MAN_PATH); + puts(git_man_path); + free(git_man_path); exit(0); } else if (!strcmp(cmd, "--info-path")) { - puts(system_path(GIT_INFO_PATH)); + char *git_info_path = system_path(GIT_INFO_PATH); + puts(git_info_path); + free(git_info_path); exit(0); } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { use_pager = 1; diff --git a/wrapper.c b/wrapper.c index 007ec0d..f97adbc 100644 --- a/wrapper.c +++ b/wrapper.c @@ -523,11 +523,14 @@ int access_or_warn(const char *path, int mode, unsigned flag) return ret; } -int access_or_die(const char *path, int mode, unsigned flag) +int access_or_die(const char *path, int mode, unsigned flag, unsigned etc_config) { int ret = access(path, mode); - if (ret && !access_error_is_ok(errno, flag)) + if (ret && !access_error_is_ok(errno, flag)){ + if (etc_config) + free((char*)path); die_errno(_("unable to access '%s'"), path); + } return ret; } -- 2.2.0.rc3.191.g70a3888.dirty ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re*: [PATCH] change contract between system_path and it's callers 2014-11-24 14:07 ` [PATCH] change contract between system_path and it's callers 0xAX @ 2014-11-24 19:33 ` Junio C Hamano 2014-11-24 19:53 ` Alex Kuleshov 0 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2014-11-24 19:33 UTC (permalink / raw) To: 0xAX; +Cc: git, Jeff King, Eric Sunshine 0xAX <kuleshovmail@gmail.com> writes: > Now system_path returns path which is allocated string to callers; > It prevents memory leaks in some places. All callers of system_path > are owners of path string and they must release it. > > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com> > --- > -static const char *git_etc_gitattributes(void) > +static char *git_etc_gitattributes(void) Hmph, I think this should keep returning "const char *", as the caller is not expected to free the pointer or write into the memory held by the returned string. The same applies to the change of the type in "struct git_config_source". The change in the semantics of system_path() made the "get once and keep returning it" this function does safer and correct, but this function itself did not change any behaviour from its caller's point of view. > { > - static const char *system_wide; > + static char *system_wide; > if (!system_wide) > system_wide = system_path(ETC_GITATTRIBUTES); > return system_wide; > @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void) > attr_stack = elem; > > if (git_attr_system()) { > - elem = read_attr_from_file(git_etc_gitattributes(), 1); > + char *etc_attributes = git_etc_gitattributes(); > + elem = read_attr_from_file(etc_attributes, 1); > + free(etc_attributes); And freeing here is actively wrong, I think. You are freeing the piece of memory still pointed by "static char *system_wide" in the function git_etc_gitattributes(); when it is called again, the caller will get a pointer into the memory you have freed here. > diff --git a/builtin/config.c b/builtin/config.c > index 15a7bea..266d42b 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix) > if (given_config_source.blob) > die("editing blobs is not supported"); > git_config(git_default_config, NULL); > - config_file = xstrdup(given_config_source.file ? > - given_config_source.file : git_path("config")); > + > + if (use_system_config) > + config_file = given_config_source.file ? > + given_config_source.file : git_path("config"); > + else > + config_file = xstrdup(given_config_source.file ? > + given_config_source.file : git_path("config")); > + Sorry, but I do not understand this change. Why do you need "if use-system-config, do not allocate" here and then later everywhere "if use-system-config, free it"? I would understand if the change were "earlier we had mixture of borrowed and owned strings, but we make sure we always own the string by running xstrdup() in the caller when we used to let this function borrow, so that we can always free() before returning from here", or something. For example, in the original code, use_local_config codepath uses git_pathdup(), which is an allocated piece of memory, to initialize given_config_source.file. Doesn't it need to be freed? Even if it is not in the use_system_config mode, if given_config_source.file is not an absolute path, we store a copy of prefix-filename result to given_config_source.file. Doesn't it need to be freed? I think the implementation strategy you took makes the result unnecessarily messy and error prone. Let's step back a bit. When you have code that sometimes borrows memory and sometimes owns memory, writing the clean-up part like this is error prone: char *var; if (A) var = borrowed memory; else if (B) var = borrowed memory; else if (C) var = xstrdup(borrowed memory); else var = borrowed memory; use var; /* a loong piece of code in reality */ if (C) free(var); because the set-up part can and will change over time as the code evolves. If written this way: char *var, *to_free = NULL; if (A) var = borrowed memory; else if (B) var = borrowed memory; else if (C) to_free = var = xstrdup(borrowed memory); else var = borrowed memory; use var; /* a loong piece of code in reality */ free(to_free); the clean-up part would not have to worry how the set-up part decided when to borrow memory and when to own memory. Another way to do so would obviously be: char *var; if (A) var = xstrdup(borrowed memory); else if (B) var = xstrdup(borrowed memory); else if (C) var = xstrdup(borrowed memory); else var = xstrdup(borrowed memory); use var; /* a loong piece of code in reality */ free(var); to always own the memory, but depending on the original code, it may make the result more inefficient and the patch more noisy. This is especially true when the clean-up part is spread all over the place like your patch does, i.e. the "use var" part of the code looks like if (X) { ... a looong piece of code to use var ... if (C) free(var); } else if (Y) { ... a looong piece of code to use var ... if (C) free(var); } else { ... a looong piece of code to use var ... if (C) free(var); } which is simply unmaintainable. Better to make "use var" and the clean-up part structured more like this, using the to_free trick: if (X) { ... a looong piece of code to use var ... goto cleanup; } else if (Y) { ... a looong piece of code to use var ... goto cleanup; } else { ... a looong piece of code to use var ... goto cleanup; } cleanup: free(to_free); > diff --git a/builtin/help.c b/builtin/help.c > index b3c818e..20ffbb1 100644 > --- a/builtin/help.c > +++ b/builtin/help.c > @@ -326,7 +326,8 @@ static void setup_man_path(void) > * old_path, the ':' at the end will let 'man' to try > * system-wide paths after ours to find the manual page. If > * there is old_path, we need ':' as delimiter. */ > - strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); > + char *git_man_path = system_path(GIT_MAN_PATH); Defile variable in the definition block above to avoid future decl-after-stmt, i.e. const char *old_path = getenv("MANPATH"); +char *git_man_path = system_path(GIT_MAN_PATH); /* We should always put ... > + strbuf_addstr(&new_path, git_man_path); And better free it immediately after you are done, instead of at the end of the function. > strbuf_addch(&new_path, ':'); > if (old_path) > strbuf_addstr(&new_path, old_path); > @@ -334,6 +335,7 @@ static void setup_man_path(void) > setenv("MANPATH", new_path.buf, 1); > > strbuf_release(&new_path); > + free(git_man_path); > } > -static void get_html_page_path(struct strbuf *page_path, const char *page) > +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path) > { > struct stat st; > - if (!html_path) > - html_path = system_path(GIT_HTML_PATH); > > /* Check that we have a git documentation directory. */ > if (!strstr(html_path, "://")) { > if (stat(mkpath("%s/git.html", html_path), &st) > - || !S_ISREG(st.st_mode)) > - die("'%s': not a documentation directory.", html_path); > + || !S_ISREG(st.st_mode)){ > + printf("'%s': not a documentation directory.\n", html_path); > + free(html_path); > + exit(1); This is an unnecessary churn, and an unwarranted change to the externally visible behaviour. die() gives its message to the standard error stream (not to the standard output stream), and exits with 128 (not with 1). We are exiting immediately after; it is not worth changing the API to the caller or external behaviour, only to free(html_path) conditionally. > diff --git a/builtin/init-db.c b/builtin/init-db.c > index 2619ca5..6f36f3f 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir) > template_dir = init_db_template_dir; > if (!template_dir) > template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR); > - if (!template_dir[0]) > + if (!template_dir[0]){ A SP between ) and { > + if (!template_dir) > + free((char*)template_dir); No need for "call free only if it is not NULL"; free(NULL) works just fine. > return; > + } > template_len = strlen(template_dir); > - if (PATH_MAX <= (template_len+strlen("/config"))) > + if (PATH_MAX <= (template_len+strlen("/config"))){ > + if (!template_dir) > + free((char*)template_dir); > die(_("insanely long template path %s"), template_dir); You are freeing and then using it in the message, and it cannot be NULL anyway so "if (!template_dir)" is doubly unnecessary. I wouldn't bother freeing immediately before dying, exiting, or execing. Here is my quick attempt, starting solely from the output of "git grep system_path". How does it look? One thing to note is that this illustration does not consider memory pointed at by the "system_wide" variable here (from attr.c) static const char *git_etc_gitattributes(void) { static const char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITATTRIBUTES); return system_wide; } at the point of process exit as a "leak". -- >8 -- Subject: system_path(): always return free'able memory to the caller The function sometimes returns a newly allocated string and sometimes returns a borrowed string, the latter of which the callers must not free(). The existing callers all assume that the return value belongs to the callee and most of them copy it with strdup() when they want to keep it around. They end up leaking the returned copy when the callee returned a new string because they cannot tell if they should free it. Change the contract between the callers and system_path() to make the returned string owned by the callers; they are responsible for freeing it when done, but they do not have to make their own copy to store it away. Adjust the callers to make sure they do not leak the returned string once they are done, but do not bother freeing it just before dying, exiting or exec'ing other program to avoid unnecessary churn. Reported-by: Alexander Kuleshov <kuleshovmail@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/help.c | 9 +++++++-- builtin/init-db.c | 15 ++++++++++----- exec_cmd.c | 7 +++---- exec_cmd.h | 2 +- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/builtin/help.c b/builtin/help.c index b3c818e..e78c135 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -321,16 +321,18 @@ static void setup_man_path(void) { struct strbuf new_path = STRBUF_INIT; const char *old_path = getenv("MANPATH"); + char *git_man_path = system_path(GIT_MAN_PATH); /* We should always put ':' after our path. If there is no * old_path, the ':' at the end will let 'man' to try * system-wide paths after ours to find the manual page. If * there is old_path, we need ':' as delimiter. */ - strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); + strbuf_addstr(&new_path, git_man_path); strbuf_addch(&new_path, ':'); if (old_path) strbuf_addstr(&new_path, old_path); + free(git_man_path); setenv("MANPATH", new_path.buf, 1); strbuf_release(&new_path); @@ -380,8 +382,10 @@ static void show_info_page(const char *git_cmd) static void get_html_page_path(struct strbuf *page_path, const char *page) { struct stat st; + char *to_free = NULL; + if (!html_path) - html_path = system_path(GIT_HTML_PATH); + html_path = to_free = system_path(GIT_HTML_PATH); /* Check that we have a git documentation directory. */ if (!strstr(html_path, "://")) { @@ -392,6 +396,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page) strbuf_init(page_path, 0); strbuf_addf(page_path, "%s/%s.html", html_path, page); + free(to_free); } /* diff --git a/builtin/init-db.c b/builtin/init-db.c index 2619ca5..9966522 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -119,15 +119,18 @@ static void copy_templates(const char *template_dir) DIR *dir; const char *git_dir = get_git_dir(); int len = strlen(git_dir); + char *to_free = NULL; if (!template_dir) template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT); if (!template_dir) template_dir = init_db_template_dir; if (!template_dir) - template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR); - if (!template_dir[0]) + template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR); + if (!template_dir[0]) { + free(to_free); return; + } template_len = strlen(template_dir); if (PATH_MAX <= (template_len+strlen("/config"))) die(_("insanely long template path %s"), template_dir); @@ -139,7 +142,7 @@ static void copy_templates(const char *template_dir) dir = opendir(template_path); if (!dir) { warning(_("templates not found %s"), template_dir); - return; + goto free_return; } /* Make sure that template is from the correct vintage */ @@ -155,8 +158,7 @@ static void copy_templates(const char *template_dir) "a wrong format version %d from '%s'"), repository_format_version, template_dir); - closedir(dir); - return; + goto close_free_return; } memcpy(path, git_dir, len); @@ -166,7 +168,10 @@ static void copy_templates(const char *template_dir) copy_templates_1(path, len, template_path, template_len, dir); +close_free_return: closedir(dir); +free_return: + free(to_free); } static int git_init_db_config(const char *k, const char *v, void *cb) diff --git a/exec_cmd.c b/exec_cmd.c index 698e752..8ab37b5 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -6,7 +6,7 @@ static const char *argv_exec_path; static const char *argv0_path; -const char *system_path(const char *path) +char *system_path(const char *path) { #ifdef RUNTIME_PREFIX static const char *prefix; @@ -16,7 +16,7 @@ const char *system_path(const char *path) struct strbuf d = STRBUF_INIT; if (is_absolute_path(path)) - return path; + return xstrdup(path); #ifdef RUNTIME_PREFIX assert(argv0_path); @@ -34,8 +34,7 @@ const char *system_path(const char *path) #endif strbuf_addf(&d, "%s/%s", prefix, path); - path = strbuf_detach(&d, NULL); - return path; + return strbuf_detach(&d, NULL); } const char *git_extract_argv0_path(const char *argv0) diff --git a/exec_cmd.h b/exec_cmd.h index e4c9702..93b0c02 100644 --- a/exec_cmd.h +++ b/exec_cmd.h @@ -9,6 +9,6 @@ extern const char **prepare_git_cmd(const char **argv); extern int execv_git_cmd(const char **argv); /* NULL terminated */ LAST_ARG_MUST_BE_NULL extern int execl_git_cmd(const char *cmd, ...); -extern const char *system_path(const char *path); +extern char *system_path(const char *path); #endif /* GIT_EXEC_CMD_H */ ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-24 19:33 ` Re*: " Junio C Hamano @ 2014-11-24 19:53 ` Alex Kuleshov 2014-11-24 20:20 ` Junio C Hamano 2014-11-24 20:50 ` Junio C Hamano 0 siblings, 2 replies; 33+ messages in thread From: Alex Kuleshov @ 2014-11-24 19:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine Junio C Hamano <gitster@pobox.com> @ 2014-11-25 01:33 ALMT: >> -static const char *git_etc_gitattributes(void) >> +static char *git_etc_gitattributes(void) > > Hmph, I think this should keep returning "const char *", as the > caller is not expected to free the pointer or write into the memory > held by the returned string. The same applies to the change of the > type in "struct git_config_source". > > The change in the semantics of system_path() made the "get once and > keep returning it" this function does safer and correct, but this > function itself did not change any behaviour from its caller's point > of view. Understand, will fix it. > >> { >> - static const char *system_wide; >> + static char *system_wide; >> if (!system_wide) >> system_wide = system_path(ETC_GITATTRIBUTES); >> return system_wide; > > >> @@ -489,7 +489,9 @@ static void bootstrap_attr_stack(void) >> attr_stack = elem; >> >> if (git_attr_system()) { >> - elem = read_attr_from_file(git_etc_gitattributes(), 1); >> + char *etc_attributes = git_etc_gitattributes(); >> + elem = read_attr_from_file(etc_attributes, 1); >> + free(etc_attributes); > > And freeing here is actively wrong, I think. You are freeing the > piece of memory still pointed by "static char *system_wide" in the > function git_etc_gitattributes(); when it is called again, the > caller will get a pointer into the memory you have freed here. Why? If i understand correctly we don't use etc_attributes anymore in this function and if we'll call this function again git_etc_gitattributes will create new pointer and system_path alloc memory for it or i'm wrong with it? > >> diff --git a/builtin/config.c b/builtin/config.c >> index 15a7bea..266d42b 100644 >> --- a/builtin/config.c >> +++ b/builtin/config.c >> @@ -575,8 +575,14 @@ int cmd_config(int argc, const char **argv, const char *prefix) >> if (given_config_source.blob) >> die("editing blobs is not supported"); >> git_config(git_default_config, NULL); >> - config_file = xstrdup(given_config_source.file ? >> - given_config_source.file : git_path("config")); >> + >> + if (use_system_config) >> + config_file = given_config_source.file ? >> + given_config_source.file : git_path("config"); >> + else >> + config_file = xstrdup(given_config_source.file ? >> + given_config_source.file : git_path("config")); >> + > > Sorry, but I do not understand this change. > > Why do you need "if use-system-config, do not allocate" here and > then later everywhere "if use-system-config, free it"? I would > understand if the change were "earlier we had mixture of borrowed > and owned strings, but we make sure we always own the string by > running xstrdup() in the caller when we used to let this function > borrow, so that we can always free() before returning from here", > or something. > > For example, in the original code, use_local_config codepath uses > git_pathdup(), which is an allocated piece of memory, to initialize > given_config_source.file. Doesn't it need to be freed? > > Even if it is not in the use_system_config mode, if > given_config_source.file is not an absolute path, we store a copy of > prefix-filename result to given_config_source.file. Doesn't it need > to be freed? > > I think the implementation strategy you took makes the result > unnecessarily messy and error prone. Let's step back a bit. > > When you have code that sometimes borrows memory and sometimes owns > memory, writing the clean-up part like this is error prone: > > char *var; > > if (A) > var = borrowed memory; > else if (B) > var = borrowed memory; > else if (C) > var = xstrdup(borrowed memory); > else > var = borrowed memory; > > use var; /* a loong piece of code in reality */ > > if (C) > free(var); > > because the set-up part can and will change over time as the code > evolves. If written this way: > > char *var, *to_free = NULL; > > if (A) > var = borrowed memory; > else if (B) > var = borrowed memory; > else if (C) > to_free = var = xstrdup(borrowed memory); > else > var = borrowed memory; > > use var; /* a loong piece of code in reality */ > > free(to_free); > > the clean-up part would not have to worry how the set-up part > decided when to borrow memory and when to own memory. Another way > to do so would obviously be: > > char *var; > > if (A) > var = xstrdup(borrowed memory); > else if (B) > var = xstrdup(borrowed memory); > else if (C) > var = xstrdup(borrowed memory); > else > var = xstrdup(borrowed memory); > > use var; /* a loong piece of code in reality */ > > free(var); > > to always own the memory, but depending on the original code, it may > make the result more inefficient and the patch more noisy. > > This is especially true when the clean-up part is spread all over > the place like your patch does, i.e. the "use var" part of the code > looks like > > if (X) { > ... a looong piece of code to use var ... > if (C) > free(var); > } else if (Y) { > ... a looong piece of code to use var ... > if (C) > free(var); > } else { > ... a looong piece of code to use var ... > if (C) > free(var); > } > > which is simply unmaintainable. Better to make "use var" and the > clean-up part structured more like this, using the to_free trick: > > if (X) { > ... a looong piece of code to use var ... > goto cleanup; > } else if (Y) { > ... a looong piece of code to use var ... > goto cleanup; > } else { > ... a looong piece of code to use var ... > goto cleanup; > } > > cleanup: > free(to_free); > Will look again on it tomorrow and hope send new patch (sorry deep night today here), anyway thanks for great explanation. >> diff --git a/builtin/help.c b/builtin/help.c >> index b3c818e..20ffbb1 100644 >> --- a/builtin/help.c >> +++ b/builtin/help.c >> @@ -326,7 +326,8 @@ static void setup_man_path(void) >> * old_path, the ':' at the end will let 'man' to try >> * system-wide paths after ours to find the manual page. If >> * there is old_path, we need ':' as delimiter. */ >> - strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); >> + char *git_man_path = system_path(GIT_MAN_PATH); > > Defile variable in the definition block above to avoid future > decl-after-stmt, i.e. > > const char *old_path = getenv("MANPATH"); > +char *git_man_path = system_path(GIT_MAN_PATH); > > /* We should always put ... > >> + strbuf_addstr(&new_path, git_man_path); > > And better free it immediately after you are done, instead of at the > end of the function. Will fix it. > >> strbuf_addch(&new_path, ':'); >> if (old_path) >> strbuf_addstr(&new_path, old_path); >> @@ -334,6 +335,7 @@ static void setup_man_path(void) >> setenv("MANPATH", new_path.buf, 1); >> >> strbuf_release(&new_path); >> + free(git_man_path); >> } > >> -static void get_html_page_path(struct strbuf *page_path, const char *page) >> +static void get_html_page_path(struct strbuf *page_path, const char *page, char *html_path) >> { >> struct stat st; >> - if (!html_path) >> - html_path = system_path(GIT_HTML_PATH); >> >> /* Check that we have a git documentation directory. */ >> if (!strstr(html_path, "://")) { >> if (stat(mkpath("%s/git.html", html_path), &st) >> - || !S_ISREG(st.st_mode)) >> - die("'%s': not a documentation directory.", html_path); >> + || !S_ISREG(st.st_mode)){ >> + printf("'%s': not a documentation directory.\n", html_path); >> + free(html_path); >> + exit(1); > > This is an unnecessary churn, and an unwarranted change to the > externally visible behaviour. die() gives its message to the > standard error stream (not to the standard output stream), and exits > with 128 (not with 1). > > We are exiting immediately after; it is not worth changing the API > to the caller or external behaviour, only to free(html_path) > conditionally. So how can i free html_path here if when i'll return die instead exit? I can't use free(html_path) before/after die call. > >> diff --git a/builtin/init-db.c b/builtin/init-db.c >> index 2619ca5..6f36f3f 100644 >> --- a/builtin/init-db.c >> +++ b/builtin/init-db.c >> @@ -126,11 +126,17 @@ static void copy_templates(const char *template_dir) >> template_dir = init_db_template_dir; >> if (!template_dir) >> template_dir = system_path(DEFAULT_GIT_TEMPLATE_DIR); >> - if (!template_dir[0]) >> + if (!template_dir[0]){ > > A SP between ) and { > >> + if (!template_dir) >> + free((char*)template_dir); > > No need for "call free only if it is not NULL"; free(NULL) works > just fine. Will fix it. > >> return; >> + } >> template_len = strlen(template_dir); >> - if (PATH_MAX <= (template_len+strlen("/config"))) >> + if (PATH_MAX <= (template_len+strlen("/config"))){ >> + if (!template_dir) >> + free((char*)template_dir); >> die(_("insanely long template path %s"), template_dir); > > You are freeing and then using it in the message, and it cannot be > NULL anyway so "if (!template_dir)" is doubly unnecessary. > > I wouldn't bother freeing immediately before dying, exiting, or > execing. > > Here is my quick attempt, starting solely from the output of "git > grep system_path". How does it look? It looks fantastic :) > > One thing to note is that this illustration does not consider memory > pointed at by the "system_wide" variable here (from attr.c) > > static const char *git_etc_gitattributes(void) > { > static const char *system_wide; > if (!system_wide) > system_wide = system_path(ETC_GITATTRIBUTES); > return system_wide; > } > > at the point of process exit as a "leak". But why? We allocated memory to "system_wide" with system_path, next git will exit somewhere with die, but system_wide didn't free... Or i'm wrong here too? Anyway many thanks for so great feedback, tomorrow I'll reread it and will try to do all things in correct way. Thanky you. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-24 19:53 ` Alex Kuleshov @ 2014-11-24 20:20 ` Junio C Hamano 2014-11-24 20:50 ` Junio C Hamano 1 sibling, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2014-11-24 20:20 UTC (permalink / raw) To: Alex Kuleshov; +Cc: git, Jeff King, Eric Sunshine Alex Kuleshov <kuleshovmail@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> @ 2014-11-25 01:33 ALMT: > ... >>> if (git_attr_system()) { >>> - elem = read_attr_from_file(git_etc_gitattributes(), 1); >>> + char *etc_attributes = git_etc_gitattributes(); >>> + elem = read_attr_from_file(etc_attributes, 1); >>> + free(etc_attributes); >> >> And freeing here is actively wrong, I think. You are freeing the >> piece of memory still pointed by "static char *system_wide" in the >> function git_etc_gitattributes(); when it is called again, the >> caller will get a pointer into the memory you have freed here. > > Why? If i understand correctly we don't use etc_attributes anymore in > this function and if we'll call this function again > git_etc_gitattributes will create new pointer and system_path alloc > memory for it or i'm wrong with it? The function keeps a singleton in "static const char *system_wide" so that it has to call system_path() only once, and keeps the value for second and subsequent calls. From its callers' point of view, they are only peeking the memory it returns. This aspect does not change with your patch. -static const char *git_etc_gitattributes(void) +static char *git_etc_gitattributes(void) { - static const char *system_wide; + static char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITATTRIBUTES); return system_wide; ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-24 19:53 ` Alex Kuleshov 2014-11-24 20:20 ` Junio C Hamano @ 2014-11-24 20:50 ` Junio C Hamano 2014-11-25 6:45 ` Alexander Kuleshov 1 sibling, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2014-11-24 20:50 UTC (permalink / raw) To: Alex Kuleshov; +Cc: git, Jeff King, Eric Sunshine Alex Kuleshov <kuleshovmail@gmail.com> writes: >> One thing to note is that this illustration does not consider memory >> pointed at by the "system_wide" variable here (from attr.c) >> >> static const char *git_etc_gitattributes(void) >> { >> static const char *system_wide; >> if (!system_wide) >> system_wide = system_path(ETC_GITATTRIBUTES); >> return system_wide; >> } >> >> at the point of process exit as a "leak". > > But why? We allocated memory to "system_wide" with system_path, next git > will exit somewhere with die, but system_wide didn't free... Or i'm > wrong here too? It is in the same league as "static const char *git_dir" and friends that appear in the file-scope-static of environment.c. Keeping small things around to be cleaned up by exit() is not a crime. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-24 20:50 ` Junio C Hamano @ 2014-11-25 6:45 ` Alexander Kuleshov 2014-11-25 7:04 ` Alexander Kuleshov 0 siblings, 1 reply; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-25 6:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine Hello Junio, I returned static to git_etc_gitattributes return value, but now i can't understand where to free 'system_wide'. As i understand correctly attr.c has following API for querying attributes: git_attr git_check_attr And as i see in code git_attr doesn't use git_attr_check, so now we have only git_check_attr and git_all_stars functions which are through prepare_attr_stack and bootstrap_attr_stack in it uses path which returned by system_path. So you're right if i free etc_attributes like this and git_etc_gitattributes will return static value we'll have access to freed memory if it will called again. But we have no access to etc_attributes outside bootstrap_attr_stack so where will be best place to free it? Thank you 2014-11-25 2:50 GMT+06:00 Junio C Hamano <gitster@pobox.com>: > Alex Kuleshov <kuleshovmail@gmail.com> writes: > >>> One thing to note is that this illustration does not consider memory >>> pointed at by the "system_wide" variable here (from attr.c) >>> >>> static const char *git_etc_gitattributes(void) >>> { >>> static const char *system_wide; >>> if (!system_wide) >>> system_wide = system_path(ETC_GITATTRIBUTES); >>> return system_wide; >>> } >>> >>> at the point of process exit as a "leak". >> >> But why? We allocated memory to "system_wide" with system_path, next git >> will exit somewhere with die, but system_wide didn't free... Or i'm >> wrong here too? > > It is in the same league as "static const char *git_dir" and friends > that appear in the file-scope-static of environment.c. Keeping small > things around to be cleaned up by exit() is not a crime. -- _________________________ 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-25 6:45 ` Alexander Kuleshov @ 2014-11-25 7:04 ` Alexander Kuleshov 2014-11-25 17:55 ` Junio C Hamano 2014-11-25 17:59 ` Alexander Kuleshov 0 siblings, 2 replies; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-25 7:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine But if we still static const char* for git_etc_gitattributes and will not free it in bootstrap_attr_stack there will no memory leak, just tested it, so i'll remove free for it. 2014-11-25 12:45 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>: > Hello Junio, > > I returned static to git_etc_gitattributes return value, but now i > can't understand where to free 'system_wide'. As i understand > correctly attr.c has following API for querying attributes: > > git_attr > > git_check_attr > > And as i see in code git_attr doesn't use git_attr_check, so now we > have only git_check_attr and git_all_stars functions which are through > prepare_attr_stack and bootstrap_attr_stack in it uses path which > returned > by system_path. So you're right if i free etc_attributes like this > and git_etc_gitattributes will return static value we'll have access > to freed memory if it will called again. But we have no access to > etc_attributes outside bootstrap_attr_stack so where will be best > place to free it? > > Thank you > > 2014-11-25 2:50 GMT+06:00 Junio C Hamano <gitster@pobox.com>: >> Alex Kuleshov <kuleshovmail@gmail.com> writes: >> >>>> One thing to note is that this illustration does not consider memory >>>> pointed at by the "system_wide" variable here (from attr.c) >>>> >>>> static const char *git_etc_gitattributes(void) >>>> { >>>> static const char *system_wide; >>>> if (!system_wide) >>>> system_wide = system_path(ETC_GITATTRIBUTES); >>>> return system_wide; >>>> } >>>> >>>> at the point of process exit as a "leak". >>> >>> But why? We allocated memory to "system_wide" with system_path, next git >>> will exit somewhere with die, but system_wide didn't free... Or i'm >>> wrong here too? >> >> It is in the same league as "static const char *git_dir" and friends >> that appear in the file-scope-static of environment.c. Keeping small >> things around to be cleaned up by exit() is not a crime. > > > > -- > _________________________ > 0xAX -- _________________________ 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-25 7:04 ` Alexander Kuleshov @ 2014-11-25 17:55 ` Junio C Hamano 2014-11-25 18:03 ` Alexander Kuleshov 2014-11-25 17:59 ` Alexander Kuleshov 1 sibling, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2014-11-25 17:55 UTC (permalink / raw) To: Alexander Kuleshov; +Cc: git, Jeff King, Eric Sunshine Alexander Kuleshov <kuleshovmail@gmail.com> writes: > But if we still static const char* for git_etc_gitattributes and will > not free it in bootstrap_attr_stack there will no memory leak, just > tested it, so i'll remove free for it. Leak or no leak, freeing there is wrong. It will free the piece of memory the next caller will obtain from the git_etc_gitattributes() function. In other words, the return value from that function is owned by git_etc_gitattributes(), not by the caller. As to "leak", in the strictest sense of the word, i.e. "do we allocate on the heap and exit without freeing?", it _is_ leaking, and your above "just tested it" probably means "the tool I used did not find/report it". But as I said, it is not something worth bothering about [*1*]. Think of it this way. The git_etc_gitattributes() function goes like this with your patch (and there is no fundamental difference in the original version, which uses "const char *" where appropriate): static char *git_etc_gitattributes(void) { static char *system_wide; if (!system_wide) system_wide = system_path(ETC_GITATTRIBUTES); return system_wide; } If you knew that the pathname for /etc/gitattributes substitute will never be longer than 256 bytes, you may have coded the above like so instead: static char system_wide[256]; static char *git_etc_gitattributes(void) { if (!system_wide[0]) { char *ret = system_path(ETC_GITATTRIBUTES); if (strncpy(system_wide, ret, 256) >= 256) die("ETC_GITATTRIBUTES too long "); } return system_wide; } Even though we used the memory occupied by system_wide[] and did not clean up before exit(), nobody would complain about leaking. The existing code is the moral equivalent of the "up to 256 bytes" illustration, but for a string whose size is not known at compile time. It is using and keeping the memory until program exit. Nobody should complain about leaking. That is, unless (1) the held memory is expected to be very large and (2) you can prove that after the point you decide to insert free(), nobody will ever need that information again. [Footnote] *1* The leaks we care about are of this form: void silly_leaker(void) { printf("%s\n", system_path(ETC_GITATTRIBUTES)); } where each invocation allocates memory, uses it and then loses the reference to it without doing anything to clean-up. You can call such a function unbounded number of times and waste unbounded amount of memory. The implementation of git_etc_gitattributes() is not of that kind. An invocation of it allocates, uses and keeps. The second and subsequent invocation does not allocate. When you call it unbounded number of times, it does not waste unbounded amount of memory. It just keeps what it needs to answer the next caller with. The pattern needs to be made thread-safe, but that is a separate topic. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-25 17:55 ` Junio C Hamano @ 2014-11-25 18:03 ` Alexander Kuleshov 2014-11-25 18:24 ` [PATCH 1/1] " Alexander Kuleshov 2014-11-25 20:20 ` Re*: [PATCH] " Junio C Hamano 0 siblings, 2 replies; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-25 18:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine Ah, you little ahead me, so we do not care about config.c in this way, because it takes git_etc_gitconfig() in the same way as git_etc_gitattributes 2014-11-25 23:55 GMT+06:00 Junio C Hamano <gitster@pobox.com>: > Alexander Kuleshov <kuleshovmail@gmail.com> writes: > >> But if we still static const char* for git_etc_gitattributes and will >> not free it in bootstrap_attr_stack there will no memory leak, just >> tested it, so i'll remove free for it. > > Leak or no leak, freeing there is wrong. It will free the piece of > memory the next caller will obtain from the git_etc_gitattributes() > function. In other words, the return value from that function is > owned by git_etc_gitattributes(), not by the caller. > > As to "leak", in the strictest sense of the word, i.e. "do we > allocate on the heap and exit without freeing?", it _is_ leaking, > and your above "just tested it" probably means "the tool I used did > not find/report it". But as I said, it is not something worth > bothering about [*1*]. > > Think of it this way. > > The git_etc_gitattributes() function goes like this with your patch > (and there is no fundamental difference in the original version, > which uses "const char *" where appropriate): > > static char *git_etc_gitattributes(void) > { > static char *system_wide; > if (!system_wide) > system_wide = system_path(ETC_GITATTRIBUTES); > return system_wide; > } > > If you knew that the pathname for /etc/gitattributes substitute will > never be longer than 256 bytes, you may have coded the above like so > instead: > > static char system_wide[256]; > static char *git_etc_gitattributes(void) > { > if (!system_wide[0]) { > char *ret = system_path(ETC_GITATTRIBUTES); > if (strncpy(system_wide, ret, 256) >= 256) > die("ETC_GITATTRIBUTES too long "); > } > return system_wide; > } > > Even though we used the memory occupied by system_wide[] and did not > clean up before exit(), nobody would complain about leaking. > > The existing code is the moral equivalent of the "up to 256 bytes" > illustration, but for a string whose size is not known at compile > time. It is using and keeping the memory until program exit. > Nobody should complain about leaking. > > That is, unless (1) the held memory is expected to be very large and > (2) you can prove that after the point you decide to insert free(), > nobody will ever need that information again. > > > [Footnote] > > *1* The leaks we care about are of this form: > > void silly_leaker(void) > { > printf("%s\n", system_path(ETC_GITATTRIBUTES)); > } > > where each invocation allocates memory, uses it and then loses > the reference to it without doing anything to clean-up. You can > call such a function unbounded number of times and waste > unbounded amount of memory. > > The implementation of git_etc_gitattributes() is not of that > kind. An invocation of it allocates, uses and keeps. The > second and subsequent invocation does not allocate. When you > call it unbounded number of times, it does not waste unbounded > amount of memory. It just keeps what it needs to answer the > next caller with. > > The pattern needs to be made thread-safe, but that is a separate > topic. -- _________________________ 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/1] change contract between system_path and it's callers 2014-11-25 18:03 ` Alexander Kuleshov @ 2014-11-25 18:24 ` Alexander Kuleshov 2014-11-25 21:13 ` Junio C Hamano 2014-11-25 20:20 ` Re*: [PATCH] " Junio C Hamano 1 sibling, 1 reply; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-25 18:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine Now system_path returns path which is allocated string to callers; It prevents memory leaks in some places. All callers of system_path are owners of path string and they must release it. Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com> --- builtin/help.c | 10 +++++++--- exec_cmd.c | 17 +++++++++-------- exec_cmd.h | 4 ++-- git.c | 16 ++++++++++++---- 4 files changed, 30 insertions(+), 17 deletions(-) diff --git a/builtin/help.c b/builtin/help.c index b3c818e..544d1cc 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -321,12 +321,13 @@ static void setup_man_path(void) { struct strbuf new_path = STRBUF_INIT; const char *old_path = getenv("MANPATH"); - + char *git_man_path = system_path(GIT_MAN_PATH); /* We should always put ':' after our path. If there is no * old_path, the ':' at the end will let 'man' to try * system-wide paths after ours to find the manual page. If * there is old_path, we need ':' as delimiter. */ - strbuf_addstr(&new_path, system_path(GIT_MAN_PATH)); + strbuf_addstr(&new_path, git_man_path); + free(git_man_path); strbuf_addch(&new_path, ':'); if (old_path) strbuf_addstr(&new_path, old_path); @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd) static void show_info_page(const char *git_cmd) { const char *page = cmd_to_page(git_cmd); - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1); + char *git_info_path = system_path(GIT_INFO_PATH); + setenv("INFOPATH", git_info_path, 1); + free(git_info_path); execlp("info", "info", "gitman", page, (char *)NULL); die(_("no info viewer handled the request")); } @@ -392,6 +395,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page) strbuf_init(page_path, 0); strbuf_addf(page_path, "%s/%s.html", html_path, page); + free((char*)html_path); } /* diff --git a/exec_cmd.c b/exec_cmd.c index 698e752..e3ebdd6 100644 --- a/exec_cmd.c +++ b/exec_cmd.c @@ -6,7 +6,7 @@ static const char *argv_exec_path; static const char *argv0_path; -const char *system_path(const char *path) +char *system_path(const char *path) { #ifdef RUNTIME_PREFIX static const char *prefix; @@ -16,7 +16,7 @@ const char *system_path(const char *path) struct strbuf d = STRBUF_INIT; if (is_absolute_path(path)) - return path; + return strdup(path); #ifdef RUNTIME_PREFIX assert(argv0_path); @@ -34,8 +34,7 @@ const char *system_path(const char *path) #endif strbuf_addf(&d, "%s/%s", prefix, path); - path = strbuf_detach(&d, NULL); - return path; + return d.buf; } const char *git_extract_argv0_path(const char *argv0) @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path) /* Returns the highest-priority, location to look for git programs. */ -const char *git_exec_path(void) +char *git_exec_path(void) { const char *env; if (argv_exec_path) - return argv_exec_path; + return strdup(argv_exec_path); env = getenv(EXEC_PATH_ENVIRONMENT); if (env && *env) { - return env; + return strdup(env); } return system_path(GIT_EXEC_PATH); @@ -95,8 +94,10 @@ void setup_path(void) { const char *old_path = getenv("PATH"); struct strbuf new_path = STRBUF_INIT; + char* exec_path = git_exec_path(); - add_path(&new_path, git_exec_path()); + add_path(&new_path, exec_path); + free(exec_path); add_path(&new_path, argv0_path); if (old_path) diff --git a/exec_cmd.h b/exec_cmd.h index e4c9702..03c8599 100644 --- a/exec_cmd.h +++ b/exec_cmd.h @@ -3,12 +3,12 @@ extern void git_set_argv_exec_path(const char *exec_path); extern const char *git_extract_argv0_path(const char *path); -extern const char *git_exec_path(void); +extern char *git_exec_path(void); extern void setup_path(void); extern const char **prepare_git_cmd(const char **argv); extern int execv_git_cmd(const char **argv); /* NULL terminated */ LAST_ARG_MUST_BE_NULL extern int execl_git_cmd(const char *cmd, ...); -extern const char *system_path(const char *path); +extern char *system_path(const char *path); #endif /* GIT_EXEC_CMD_H */ diff --git a/git.c b/git.c index 82d7a1c..d01c4f1 100644 --- a/git.c +++ b/git.c @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) if (*cmd == '=') git_set_argv_exec_path(cmd + 1); else { - puts(git_exec_path()); + char *exec_path = git_exec_path(); + puts(exec_path); + free(exec_path); exit(0); } } else if (!strcmp(cmd, "--html-path")) { - puts(system_path(GIT_HTML_PATH)); + char *git_html_path = system_path(GIT_HTML_PATH); + puts(git_html_path); + free(git_html_path); exit(0); } else if (!strcmp(cmd, "--man-path")) { - puts(system_path(GIT_MAN_PATH)); + char *git_man_path = system_path(GIT_MAN_PATH); + puts(git_man_path); + free(git_man_path); exit(0); } else if (!strcmp(cmd, "--info-path")) { - puts(system_path(GIT_INFO_PATH)); + char *git_info_path = system_path(GIT_INFO_PATH); + puts(git_info_path); + free(git_info_path); exit(0); } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { use_pager = 1; -- 2.2.0.rc3.205.gea90ae3.dirty ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 1/1] change contract between system_path and it's callers 2014-11-25 18:24 ` [PATCH 1/1] " Alexander Kuleshov @ 2014-11-25 21:13 ` Junio C Hamano 2014-11-26 3:53 ` Alexander Kuleshov 0 siblings, 1 reply; 33+ messages in thread From: Junio C Hamano @ 2014-11-25 21:13 UTC (permalink / raw) To: Alexander Kuleshov; +Cc: git, Jeff King, Eric Sunshine Alexander Kuleshov <kuleshovmail@gmail.com> writes: > Now system_path returns path which is allocated string to callers; > It prevents memory leaks in some places. All callers of system_path > are owners of path string and they must release it. > > Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com> > --- Comparing this with what I sent out... > builtin/help.c | 10 +++++++--- > exec_cmd.c | 17 +++++++++-------- > exec_cmd.h | 4 ++-- > git.c | 16 ++++++++++++---- > 4 files changed, 30 insertions(+), 17 deletions(-) > > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd) > static void show_info_page(const char *git_cmd) > { > const char *page = cmd_to_page(git_cmd); > - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1); > + char *git_info_path = system_path(GIT_INFO_PATH); > + setenv("INFOPATH", git_info_path, 1); > + free(git_info_path); We are just about to exec; does this warrant the code churn? > execlp("info", "info", "gitman", page, (char *)NULL); > die(_("no info viewer handled the request")); > @@ -34,8 +34,7 @@ const char *system_path(const char *path) > #endif > > strbuf_addf(&d, "%s/%s", prefix, path); > - path = strbuf_detach(&d, NULL); > - return path; > + return d.buf; These happens to be the same with the current strbuf implementation, but it is a good manner to use strbuf_detach(&d, NULL) here. We don't know what other de-initialization tomorrow's implementation of the strbuf API may have to do in strbuf_detach(). > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path) > > > /* Returns the highest-priority, location to look for git programs. */ > -const char *git_exec_path(void) > +char *git_exec_path(void) > { > const char *env; > > if (argv_exec_path) > - return argv_exec_path; > + return strdup(argv_exec_path); > > env = getenv(EXEC_PATH_ENVIRONMENT); > if (env && *env) { > - return env; > + return strdup(env); > } Now you are making callers of git_exec_path() responsible for freeing the result they receive. git_exec_path() may be called quite a lot, which means we may end up calling system_path() many times during the life of a process without freeing its return value, so this change may be worth doing, but this patch is insufficient, isn't it? You just added load_command_list() in help.c a new leak or two, for example. There probably are other callers of this function but I don't have time to look at all of them myself right now. > @@ -95,8 +94,10 @@ void setup_path(void) > { > const char *old_path = getenv("PATH"); > struct strbuf new_path = STRBUF_INIT; > + char* exec_path = git_exec_path(); > > - add_path(&new_path, git_exec_path()); > + add_path(&new_path, exec_path); > + free(exec_path); > add_path(&new_path, argv0_path); This part by itself is good, provided if we make it the caller's responsiblity to free string returned by git_exec_path(). > diff --git a/git.c b/git.c > index 82d7a1c..d01c4f1 100644 > --- a/git.c > +++ b/git.c > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) > if (*cmd == '=') > git_set_argv_exec_path(cmd + 1); > else { > - puts(git_exec_path()); > + char *exec_path = git_exec_path(); > + puts(exec_path); > + free(exec_path); > exit(0); > } > } else if (!strcmp(cmd, "--html-path")) { > - puts(system_path(GIT_HTML_PATH)); > + char *git_html_path = system_path(GIT_HTML_PATH); > + puts(git_html_path); > + free(git_html_path); > exit(0); > } else if (!strcmp(cmd, "--man-path")) { > - puts(system_path(GIT_MAN_PATH)); > + char *git_man_path = system_path(GIT_MAN_PATH); > + puts(git_man_path); > + free(git_man_path); > exit(0); > } else if (!strcmp(cmd, "--info-path")) { > - puts(system_path(GIT_INFO_PATH)); > + char *git_info_path = system_path(GIT_INFO_PATH); > + puts(git_info_path); > + free(git_info_path); > exit(0); > } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { > use_pager = 1; None of these warrant the code churn, I would say. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/1] change contract between system_path and it's callers 2014-11-25 21:13 ` Junio C Hamano @ 2014-11-26 3:53 ` Alexander Kuleshov 2014-11-26 9:42 ` Alexander Kuleshov ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-26 3:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Eric Sunshine, git > > Comparing this with what I sent out... > > > builtin/help.c | 10 +++++++--- > > exec_cmd.c | 17 +++++++++-------- > > exec_cmd.h | 4 ++-- > > git.c | 16 ++++++++++++---- > > 4 files changed, 30 insertions(+), 17 deletions(-) > > > > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd) > > static void show_info_page(const char *git_cmd) > > { > > const char *page = cmd_to_page(git_cmd); > > - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1); > > + char *git_info_path = system_path(GIT_INFO_PATH); > > + setenv("INFOPATH", git_info_path, 1); > > + free(git_info_path); > > We are just about to exec; does this warrant the code churn? hmm... Can't understand what's the problem here? We get git_info_path from system_path which returns pointer which will need to free, set it in environment var and than free it... > > > execlp("info", "info", "gitman", page, (char *)NULL); > > die(_("no info viewer handled the request")); > > > @@ -34,8 +34,7 @@ const char *system_path(const char *path) > > #endif > > > > strbuf_addf(&d, "%s/%s", prefix, path); > > - path = strbuf_detach(&d, NULL); > > - return path; > > + return d.buf; > > These happens to be the same with the current strbuf implementation, > but it is a good manner to use strbuf_detach(&d, NULL) here. We > don't know what other de-initialization tomorrow's implementation of > the strbuf API may have to do in strbuf_detach(). How to do it in correct way? strbuf_addf(&d, "%s/%s", prefix, path); path = strbuf_detach(&d, NULL); return (char*)path; Or something else? > > > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path) > > > > > > /* Returns the highest-priority, location to look for git programs. */ > > -const char *git_exec_path(void) > > +char *git_exec_path(void) > > { > > const char *env; > > > > if (argv_exec_path) > > - return argv_exec_path; > > + return strdup(argv_exec_path); > > > > env = getenv(EXEC_PATH_ENVIRONMENT); > > if (env && *env) { > > - return env; > > + return strdup(env); > > } > > Now you are making callers of git_exec_path() responsible for > freeing the result they receive. > > git_exec_path() may be called quite a lot, which means we may end up > calling system_path() many times during the life of a process > without freeing its return value, so this change may be worth doing, > but this patch is insufficient, isn't it? > > You just added load_command_list() in help.c a new leak or two, for > example. There probably are other callers of this function but I > don't have time to look at all of them myself right now. Yes, need to do that all git_exec_path() callers free result of git_exec_path. > > > @@ -95,8 +94,10 @@ void setup_path(void) > > { > > const char *old_path = getenv("PATH"); > > struct strbuf new_path = STRBUF_INIT; > > + char* exec_path = git_exec_path(); > > > > - add_path(&new_path, git_exec_path()); > > + add_path(&new_path, exec_path); > > + free(exec_path); > > add_path(&new_path, argv0_path); > > This part by itself is good, provided if we make it the caller's > responsiblity to free string returned by git_exec_path(). > > > diff --git a/git.c b/git.c > > index 82d7a1c..d01c4f1 100644 > > --- a/git.c > > +++ b/git.c > > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) > > if (*cmd == '=') > > git_set_argv_exec_path(cmd + 1); > > else { > > - puts(git_exec_path()); > > + char *exec_path = git_exec_path(); > > + puts(exec_path); > > + free(exec_path); > > exit(0); > > } > > } else if (!strcmp(cmd, "--html-path")) { > > - puts(system_path(GIT_HTML_PATH)); > > + char *git_html_path = system_path(GIT_HTML_PATH); > > + puts(git_html_path); > > + free(git_html_path); > > exit(0); > > } else if (!strcmp(cmd, "--man-path")) { > > - puts(system_path(GIT_MAN_PATH)); > > + char *git_man_path = system_path(GIT_MAN_PATH); > > + puts(git_man_path); > > + free(git_man_path); > > exit(0); > > } else if (!strcmp(cmd, "--info-path")) { > > - puts(system_path(GIT_INFO_PATH)); > > + char *git_info_path = system_path(GIT_INFO_PATH); > > + puts(git_info_path); > > + free(git_info_path); > > exit(0); > > } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { > > use_pager = 1; > > None of these warrant the code churn, I would say. Sorry, english is not my first language, what did you mean when saying: "code churn"? Code duplication or something else? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/1] change contract between system_path and it's callers 2014-11-26 3:53 ` Alexander Kuleshov @ 2014-11-26 9:42 ` Alexander Kuleshov 2014-11-26 14:00 ` Alexander Kuleshov 2014-11-26 17:53 ` Junio C Hamano 2014-11-28 13:09 ` Philip Oakley 2 siblings, 1 reply; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-26 9:42 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Eric Sunshine, git Maybe we will discard this patch, because i looked on it and tested with different places, it brings more leaks than before? 2014-11-26 9:53 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>: >> >> Comparing this with what I sent out... >> >> > builtin/help.c | 10 +++++++--- >> > exec_cmd.c | 17 +++++++++-------- >> > exec_cmd.h | 4 ++-- >> > git.c | 16 ++++++++++++---- >> > 4 files changed, 30 insertions(+), 17 deletions(-) >> > >> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd) >> > static void show_info_page(const char *git_cmd) >> > { >> > const char *page = cmd_to_page(git_cmd); >> > - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1); >> > + char *git_info_path = system_path(GIT_INFO_PATH); >> > + setenv("INFOPATH", git_info_path, 1); >> > + free(git_info_path); >> >> We are just about to exec; does this warrant the code churn? > > hmm... Can't understand what's the problem here? We get git_info_path > from system_path which returns pointer which will need to free, set it in > environment var and than free it... > >> >> > execlp("info", "info", "gitman", page, (char *)NULL); >> > die(_("no info viewer handled the request")); >> >> > @@ -34,8 +34,7 @@ const char *system_path(const char *path) >> > #endif >> > >> > strbuf_addf(&d, "%s/%s", prefix, path); >> > - path = strbuf_detach(&d, NULL); >> > - return path; >> > + return d.buf; >> >> These happens to be the same with the current strbuf implementation, >> but it is a good manner to use strbuf_detach(&d, NULL) here. We >> don't know what other de-initialization tomorrow's implementation of >> the strbuf API may have to do in strbuf_detach(). > > How to do it in correct way? > > > strbuf_addf(&d, "%s/%s", prefix, path); > path = strbuf_detach(&d, NULL); > return (char*)path; > > Or something else? > >> >> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path) >> > >> > >> > /* Returns the highest-priority, location to look for git programs. */ >> > -const char *git_exec_path(void) >> > +char *git_exec_path(void) >> > { >> > const char *env; >> > >> > if (argv_exec_path) >> > - return argv_exec_path; >> > + return strdup(argv_exec_path); >> > >> > env = getenv(EXEC_PATH_ENVIRONMENT); >> > if (env && *env) { >> > - return env; >> > + return strdup(env); >> > } >> >> Now you are making callers of git_exec_path() responsible for >> freeing the result they receive. >> >> git_exec_path() may be called quite a lot, which means we may end up >> calling system_path() many times during the life of a process >> without freeing its return value, so this change may be worth doing, >> but this patch is insufficient, isn't it? >> >> You just added load_command_list() in help.c a new leak or two, for >> example. There probably are other callers of this function but I >> don't have time to look at all of them myself right now. > > Yes, need to do that all git_exec_path() callers free result of git_exec_path. > >> >> > @@ -95,8 +94,10 @@ void setup_path(void) >> > { >> > const char *old_path = getenv("PATH"); >> > struct strbuf new_path = STRBUF_INIT; >> > + char* exec_path = git_exec_path(); >> > >> > - add_path(&new_path, git_exec_path()); >> > + add_path(&new_path, exec_path); >> > + free(exec_path); >> > add_path(&new_path, argv0_path); >> >> This part by itself is good, provided if we make it the caller's >> responsiblity to free string returned by git_exec_path(). >> >> > diff --git a/git.c b/git.c >> > index 82d7a1c..d01c4f1 100644 >> > --- a/git.c >> > +++ b/git.c >> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) >> > if (*cmd == '=') >> > git_set_argv_exec_path(cmd + 1); >> > else { >> > - puts(git_exec_path()); >> > + char *exec_path = git_exec_path(); >> > + puts(exec_path); >> > + free(exec_path); >> > exit(0); >> > } >> > } else if (!strcmp(cmd, "--html-path")) { >> > - puts(system_path(GIT_HTML_PATH)); >> > + char *git_html_path = system_path(GIT_HTML_PATH); >> > + puts(git_html_path); >> > + free(git_html_path); >> > exit(0); >> > } else if (!strcmp(cmd, "--man-path")) { >> > - puts(system_path(GIT_MAN_PATH)); >> > + char *git_man_path = system_path(GIT_MAN_PATH); >> > + puts(git_man_path); >> > + free(git_man_path); >> > exit(0); >> > } else if (!strcmp(cmd, "--info-path")) { >> > - puts(system_path(GIT_INFO_PATH)); >> > + char *git_info_path = system_path(GIT_INFO_PATH); >> > + puts(git_info_path); >> > + free(git_info_path); >> > exit(0); >> > } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { >> > use_pager = 1; >> >> None of these warrant the code churn, I would say. > > Sorry, english is not my first language, what did you mean when saying: > "code churn"? Code duplication or something else? -- _________________________ 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/1] change contract between system_path and it's callers 2014-11-26 9:42 ` Alexander Kuleshov @ 2014-11-26 14:00 ` Alexander Kuleshov 0 siblings, 0 replies; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-26 14:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Eric Sunshine, git What do you think if we create int variable, something like given_config_must_free = 0; and will set up it to 1 in cases where it will be allocated, and than we can free it in the end of cmd_config? I'm reading git source code to understanding git internals and found little memory leak in cat-file, so i fixed it as i described above. 2014-11-26 15:42 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>: > Maybe we will discard this patch, because i looked on it and tested > with different places, it brings more leaks than before? > > 2014-11-26 9:53 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>: >>> >>> Comparing this with what I sent out... >>> >>> > builtin/help.c | 10 +++++++--- >>> > exec_cmd.c | 17 +++++++++-------- >>> > exec_cmd.h | 4 ++-- >>> > git.c | 16 ++++++++++++---- >>> > 4 files changed, 30 insertions(+), 17 deletions(-) >>> > >>> > @@ -372,7 +373,9 @@ static void show_man_page(const char *git_cmd) >>> > static void show_info_page(const char *git_cmd) >>> > { >>> > const char *page = cmd_to_page(git_cmd); >>> > - setenv("INFOPATH", system_path(GIT_INFO_PATH), 1); >>> > + char *git_info_path = system_path(GIT_INFO_PATH); >>> > + setenv("INFOPATH", git_info_path, 1); >>> > + free(git_info_path); >>> >>> We are just about to exec; does this warrant the code churn? >> >> hmm... Can't understand what's the problem here? We get git_info_path >> from system_path which returns pointer which will need to free, set it in >> environment var and than free it... >> >>> >>> > execlp("info", "info", "gitman", page, (char *)NULL); >>> > die(_("no info viewer handled the request")); >>> >>> > @@ -34,8 +34,7 @@ const char *system_path(const char *path) >>> > #endif >>> > >>> > strbuf_addf(&d, "%s/%s", prefix, path); >>> > - path = strbuf_detach(&d, NULL); >>> > - return path; >>> > + return d.buf; >>> >>> These happens to be the same with the current strbuf implementation, >>> but it is a good manner to use strbuf_detach(&d, NULL) here. We >>> don't know what other de-initialization tomorrow's implementation of >>> the strbuf API may have to do in strbuf_detach(). >> >> How to do it in correct way? >> >> >> strbuf_addf(&d, "%s/%s", prefix, path); >> path = strbuf_detach(&d, NULL); >> return (char*)path; >> >> Or something else? >> >>> >>> > @@ -68,16 +67,16 @@ void git_set_argv_exec_path(const char *exec_path) >>> > >>> > >>> > /* Returns the highest-priority, location to look for git programs. */ >>> > -const char *git_exec_path(void) >>> > +char *git_exec_path(void) >>> > { >>> > const char *env; >>> > >>> > if (argv_exec_path) >>> > - return argv_exec_path; >>> > + return strdup(argv_exec_path); >>> > >>> > env = getenv(EXEC_PATH_ENVIRONMENT); >>> > if (env && *env) { >>> > - return env; >>> > + return strdup(env); >>> > } >>> >>> Now you are making callers of git_exec_path() responsible for >>> freeing the result they receive. >>> >>> git_exec_path() may be called quite a lot, which means we may end up >>> calling system_path() many times during the life of a process >>> without freeing its return value, so this change may be worth doing, >>> but this patch is insufficient, isn't it? >>> >>> You just added load_command_list() in help.c a new leak or two, for >>> example. There probably are other callers of this function but I >>> don't have time to look at all of them myself right now. >> >> Yes, need to do that all git_exec_path() callers free result of git_exec_path. >> >>> >>> > @@ -95,8 +94,10 @@ void setup_path(void) >>> > { >>> > const char *old_path = getenv("PATH"); >>> > struct strbuf new_path = STRBUF_INIT; >>> > + char* exec_path = git_exec_path(); >>> > >>> > - add_path(&new_path, git_exec_path()); >>> > + add_path(&new_path, exec_path); >>> > + free(exec_path); >>> > add_path(&new_path, argv0_path); >>> >>> This part by itself is good, provided if we make it the caller's >>> responsiblity to free string returned by git_exec_path(). >>> >>> > diff --git a/git.c b/git.c >>> > index 82d7a1c..d01c4f1 100644 >>> > --- a/git.c >>> > +++ b/git.c >>> > @@ -95,17 +95,25 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) >>> > if (*cmd == '=') >>> > git_set_argv_exec_path(cmd + 1); >>> > else { >>> > - puts(git_exec_path()); >>> > + char *exec_path = git_exec_path(); >>> > + puts(exec_path); >>> > + free(exec_path); >>> > exit(0); >>> > } >>> > } else if (!strcmp(cmd, "--html-path")) { >>> > - puts(system_path(GIT_HTML_PATH)); >>> > + char *git_html_path = system_path(GIT_HTML_PATH); >>> > + puts(git_html_path); >>> > + free(git_html_path); >>> > exit(0); >>> > } else if (!strcmp(cmd, "--man-path")) { >>> > - puts(system_path(GIT_MAN_PATH)); >>> > + char *git_man_path = system_path(GIT_MAN_PATH); >>> > + puts(git_man_path); >>> > + free(git_man_path); >>> > exit(0); >>> > } else if (!strcmp(cmd, "--info-path")) { >>> > - puts(system_path(GIT_INFO_PATH)); >>> > + char *git_info_path = system_path(GIT_INFO_PATH); >>> > + puts(git_info_path); >>> > + free(git_info_path); >>> > exit(0); >>> > } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { >>> > use_pager = 1; >>> >>> None of these warrant the code churn, I would say. >> >> Sorry, english is not my first language, what did you mean when saying: >> "code churn"? Code duplication or something else? > > > > -- > _________________________ > 0xAX -- _________________________ 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/1] change contract between system_path and it's callers 2014-11-26 3:53 ` Alexander Kuleshov 2014-11-26 9:42 ` Alexander Kuleshov @ 2014-11-26 17:53 ` Junio C Hamano 2014-11-28 13:09 ` Philip Oakley 2 siblings, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2014-11-26 17:53 UTC (permalink / raw) To: Alexander Kuleshov; +Cc: Jeff King, Eric Sunshine, git Alexander Kuleshov <kuleshovmail@gmail.com> writes: >> > + setenv("INFOPATH", git_info_path, 1); >> > + free(git_info_path); >> > execlp("info", "info", "gitman", page, (char *)NULL); >> > die(_("no info viewer handled the request")); >> >> We are just about to exec; does this warrant the code churn? > > hmm... Can't understand what's the problem here? Adding a new variable to keep the allocated piece of memory so that you can free after using it, and freeing it. These are not "problem" per-se. But immediately after doing so, execlp() wipes your process and starts a different program afresh. When that happens, all the heap memory you allocated and were using is automatically reclaimed by the system anyway. So it may not be "wrong" to free, but doing anything extra is an unnecessary work. If you did an "assign to a variable to keep allocated memory, use it, free it and then exec" sequence in a new code, I do not think it is worth fixing the code not to do so to reduce that unnecessary work---it is not worth another round of review exchange. But in the same way, make an existing code that is not doing an unnecessary work to do so is not worth it. >> > @@ -34,8 +34,7 @@ const char *system_path(const char *path) >> > #endif >> > >> > strbuf_addf(&d, "%s/%s", prefix, path); >> > - path = strbuf_detach(&d, NULL); >> > - return path; >> > + return d.buf; >> >> These happens to be the same with the current strbuf implementation, >> but it is a good manner to use strbuf_detach(&d, NULL) here. We >> don't know what other de-initialization tomorrow's implementation of >> the strbuf API may have to do in strbuf_detach(). > > How to do it in correct way? Wouldn't "return strbuf_detach(&d, NULL);" work without any assignment to "path"? >> > ... >> > - puts(system_path(GIT_INFO_PATH)); >> > + char *git_info_path = system_path(GIT_INFO_PATH); >> > + puts(git_info_path); >> > + free(git_info_path); >> > exit(0); >> > } else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) { >> > use_pager = 1; >> >> None of these warrant the code churn, I would say. > > Sorry, english is not my first language, what did you mean when saying: > "code churn"? Code duplication or something else? Step back a bit and _think_ why you want to avoid leaks. When a program is leaky, your process will bloat by accumulating unused cruft in your heap. Step back again and think the reason why you want to avoid that bloat. It is because the parts of your program that want to allocate and use more memory will suffer _after_ leak happens, because leakers consume and keep memory that would be otherwise useful if they did not leak and freed instead. The later callers want to get more memory, but cannot get it because the program leaked memory before the control flow got to them. Now, who do these calls to free() you added help? Whose desire to allocate more memory can be harmed by the allocated but not freed piece of memory held by the return values from system_path()? Nobody--exactly because the next thing you do is to exit. It is not wrong per-se to free immediately before exiting; it is just like freeing immediately before execing. It is an unnecessary thing to do, and a change to do so has no or very little value, even if it is not a wrong thing to do. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/1] change contract between system_path and it's callers 2014-11-26 3:53 ` Alexander Kuleshov 2014-11-26 9:42 ` Alexander Kuleshov 2014-11-26 17:53 ` Junio C Hamano @ 2014-11-28 13:09 ` Philip Oakley 2 siblings, 0 replies; 33+ messages in thread From: Philip Oakley @ 2014-11-28 13:09 UTC (permalink / raw) To: Alexander Kuleshov, Junio C Hamano; +Cc: Jeff King, Eric Sunshine, Git List From: "Alexander Kuleshov" <kuleshovmail@gmail.com> Sent: Wednesday, November 26, 2014 3:53 AM >> >> None of these warrant the code churn, I would say. > > Sorry, english is not my first language, what did you mean when > saying: > "code churn"? Code duplication or something else? > -- Hi Alexander, The term 'churn' is originally from British butter making. Churn: verb 1.shake (milk or cream) in a machine in order to produce butter. "the cream is ripened before it is churned" synonyms: stir, agitate; 2.(with reference to liquid) move or cause to move about vigorously. "the seas churned". For Code (used in a somewhat negative sense), it means that lots of bits are moved around a great deal for little apparent benefit. In the sense Junio used it, I believe it's suggesting that the balance between the amount of change and usefulness of the change had gone further than hoped. (Though Junio is usually open to receiving a well argued case) Philip see also http://en.wikipedia.org/wiki/Churn_rate ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-25 18:03 ` Alexander Kuleshov 2014-11-25 18:24 ` [PATCH 1/1] " Alexander Kuleshov @ 2014-11-25 20:20 ` Junio C Hamano 1 sibling, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2014-11-25 20:20 UTC (permalink / raw) To: Alexander Kuleshov; +Cc: git, Jeff King, Eric Sunshine Alexander Kuleshov <kuleshovmail@gmail.com> writes: > Ah, you little ahead me, so we do not care about config.c in this way, > because it takes git_etc_gitconfig() in the same way as > git_etc_gitattributes Yes, but looking at the file, you would notice that the "use_local_config" codepath assigns a string obtained from git_pathdup() to given_config_source.file, which means that somebody later in the code path would not know if given_config_source.file is merely pointing at a piece of memory owned by somebody else (e.g. came from git_etc_gitconfig()) or if it owns the piece of memory (e.g. came from git_pathdup()). In the former case the memory should never be freed, but not freeing in the latter would leak the memory. Assignment to given_config_source.file I see in that function that borrows (i.e. the current code that does not fee is correct) are: - getenv(CONFIG_ENVIRONMENT) - NULL ;-) obviously - git_etc_gitconfig() All the other assignment to given_config_source.file (including the ones obtained from a call to home_config_paths()) makes us responsible to free that piece memory, so they are technically leaks. But cmd_config() is a moral equivalent of main() in a typical program, so it might not be worth worrying about allocating a single piece of memory that is used throughout the lifetime of that function and leaving it without freeing. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Re*: [PATCH] change contract between system_path and it's callers 2014-11-25 7:04 ` Alexander Kuleshov 2014-11-25 17:55 ` Junio C Hamano @ 2014-11-25 17:59 ` Alexander Kuleshov 1 sibling, 0 replies; 33+ messages in thread From: Alexander Kuleshov @ 2014-11-25 17:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Eric Sunshine Hello Junio, I'm working on this patch and builtin/config.c, i have a little question: Look, here - https://github.com/git/git/blob/master/builtin/config.c#L509 and in the next if clauses we get allocated path, but if i test it with given config file with git config -f.... (https://github.com/git/git/blob/master/builtin/config.c#L513) i have prefix NULL and next if clause doesn't execute, so if i'll put free(given_config_source.file) after config action i'll get error because given_config_source.file wasn't allocated. How to be with this? Thank you. 2014-11-25 13:04 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>: > But if we still static const char* for git_etc_gitattributes and will > not free it in bootstrap_attr_stack there will no memory leak, just > tested it, so i'll remove free for it. > > 2014-11-25 12:45 GMT+06:00 Alexander Kuleshov <kuleshovmail@gmail.com>: >> Hello Junio, >> >> I returned static to git_etc_gitattributes return value, but now i >> can't understand where to free 'system_wide'. As i understand >> correctly attr.c has following API for querying attributes: >> >> git_attr >> >> git_check_attr >> >> And as i see in code git_attr doesn't use git_attr_check, so now we >> have only git_check_attr and git_all_stars functions which are through >> prepare_attr_stack and bootstrap_attr_stack in it uses path which >> returned >> by system_path. So you're right if i free etc_attributes like this >> and git_etc_gitattributes will return static value we'll have access >> to freed memory if it will called again. But we have no access to >> etc_attributes outside bootstrap_attr_stack so where will be best >> place to free it? >> >> Thank you >> >> 2014-11-25 2:50 GMT+06:00 Junio C Hamano <gitster@pobox.com>: >>> Alex Kuleshov <kuleshovmail@gmail.com> writes: >>> >>>>> One thing to note is that this illustration does not consider memory >>>>> pointed at by the "system_wide" variable here (from attr.c) >>>>> >>>>> static const char *git_etc_gitattributes(void) >>>>> { >>>>> static const char *system_wide; >>>>> if (!system_wide) >>>>> system_wide = system_path(ETC_GITATTRIBUTES); >>>>> return system_wide; >>>>> } >>>>> >>>>> at the point of process exit as a "leak". >>>> >>>> But why? We allocated memory to "system_wide" with system_path, next git >>>> will exit somewhere with die, but system_wide didn't free... Or i'm >>>> wrong here too? >>> >>> It is in the same league as "static const char *git_dir" and friends >>> that appear in the file-scope-static of environment.c. Keeping small >>> things around to be cleaned up by exit() is not a crime. >> >> >> >> -- >> _________________________ >> 0xAX > > > > -- > _________________________ > 0xAX -- _________________________ 0xAX ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: GIT: [PATCH] exec_cmd: system_path memory leak fix 2014-11-23 13:56 GIT: [PATCH] exec_cmd: system_path memory leak fix 0xAX 2014-11-23 13:56 ` 0xAX @ 2014-11-23 18:28 ` Junio C Hamano 1 sibling, 0 replies; 33+ messages in thread From: Junio C Hamano @ 2014-11-23 18:28 UTC (permalink / raw) To: 0xAX; +Cc: git 0xAX <kuleshovmail@gmail.com> writes: > Hello All, > I found memory leak at exec_cmd.c in system_path function with valgrind. > After this patch valgrind doesn't show any memory leaks, but I'm not sure > that it is the best way to solve this problem. There are a couple of places > that uses system_path, if this way will be good, i'll make another patches > to fix this leak. > > Waiting for feedback. It is a bit sad that the callers of the function are prepared for the returned value to be volatile (that is why the ones that want to do something else between the time they call it and the time they use the value returned do make copies), while other callers that immediately use the returned value do not have to be worried about freeing it, but with the change you have to force everybody to remember freeing the returned value. If you instead limited to your change only to these two points: - make "struct strbuf d" static; - return d.buf instead of the result of strbuf_detach(&d) the updated system_path() will keep the promise all the caller expects from it, i.e. the value out of the function is volatile but the callers do not have to free it, in all cases, no? > Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com> Please have that S-o-b: line (and the same name in GIT_AUTHOR_NAME) on the patches that we'd use "git am" on, not on the cover letter ;-). Nom de guerre nobody else recognizes outside your close friends may be fun, but I feel that it is not quite appropriate to appear in "git shortlog -s" output on a public project like ours. Thanks. ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2014-11-28 13:09 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-23 13:56 GIT: [PATCH] exec_cmd: system_path memory leak fix 0xAX 2014-11-23 13:56 ` 0xAX 2014-11-23 14:01 ` 0xAX 2014-11-23 18:51 ` Junio C Hamano 2014-11-23 19:06 ` Alex Kuleshov 2014-11-23 19:19 ` Alex Kuleshov 2014-11-23 19:42 ` Jeff King 2014-11-23 20:07 ` Eric Sunshine 2014-11-23 21:58 ` Junio C Hamano 2014-11-24 7:02 ` Alex Kuleshov 2014-11-24 7:37 ` Junio C Hamano 2014-11-24 8:12 ` Alex Kuleshov 2014-11-24 13:11 ` Alexander Kuleshov 2014-11-24 14:00 ` Alex Kuleshov 2014-11-24 14:07 ` [PATCH] change contract between system_path and it's callers 0xAX 2014-11-24 19:33 ` Re*: " Junio C Hamano 2014-11-24 19:53 ` Alex Kuleshov 2014-11-24 20:20 ` Junio C Hamano 2014-11-24 20:50 ` Junio C Hamano 2014-11-25 6:45 ` Alexander Kuleshov 2014-11-25 7:04 ` Alexander Kuleshov 2014-11-25 17:55 ` Junio C Hamano 2014-11-25 18:03 ` Alexander Kuleshov 2014-11-25 18:24 ` [PATCH 1/1] " Alexander Kuleshov 2014-11-25 21:13 ` Junio C Hamano 2014-11-26 3:53 ` Alexander Kuleshov 2014-11-26 9:42 ` Alexander Kuleshov 2014-11-26 14:00 ` Alexander Kuleshov 2014-11-26 17:53 ` Junio C Hamano 2014-11-28 13:09 ` Philip Oakley 2014-11-25 20:20 ` Re*: [PATCH] " Junio C Hamano 2014-11-25 17:59 ` Alexander Kuleshov 2014-11-23 18:28 ` GIT: [PATCH] exec_cmd: system_path memory leak fix Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.