Re: [PATCH 2/3] setup_path(): Free temporary buffer
On lun, 2011-03-14 at 13:14 -0700, Junio C Hamano wrote:
Show 9 quoted lines
> Carlos Martín Nieto <cmn@elego.de> writes:
> [...]
>
> Update only one caller setup_path() to follow the new API, but leave
> other callers to leak even in normal cases. The caller in git.c exits
> immediately after using it, so we don't care about the leak there
> anyway. Also help.c has a few calls to it but the number of calls to
> the function is small and bounded, so the leak is small and we don't
> care.
Oops. I blindly tested the path only on the test case where it was
triggering an error without it and completely missed the other uses,
which is embarrassing.
Show 10 quoted lines
>
>
> And then reviewers can agree or disagree if the small leaks in git.c (just
> one string allocation that immediately is followed by exit after its use)
> and help.c (in list_commands() and load_commands_list(), neither of which
> is called millions of times anyway) are OK to accept.
>
> I tend to think they are Ok, but then I also tend to think one leak of
> exec-path return value in setup_path() is perfectly fine for the same
> reason, so in that sense I don't see a point in this patch...
Which brings us to the matter of whether we actually care about memory
leaks, as the processes are short-lived and the system is going to clean
up after us. Do we, unless the leaks are huge? As there is built-in
valgrind support in the test suite, I went in with the assumption that
we did. It seems however that hardly any code paths free their memory,
other than when using strbuf.
In case we don't, valgrind should be told not to bother reporting leaks
(and maybe mention in some document that small leaks are not an issue).
Show 34 quoted lines
>
> > diff --git a/exec_cmd.c b/exec_cmd.c
> > index 38545e8..c16c3d4 100644
> > --- a/exec_cmd.c
> > +++ b/exec_cmd.c
> > @@ -73,11 +73,11 @@ const char *git_exec_path(void)
> > const char *env;
> >
> > if (argv_exec_path)
> > - return argv_exec_path;
> > + return xstrdup(argv_exec_path);
> >
> > env = getenv(EXEC_PATH_ENVIRONMENT);
> > if (env && *env) {
> > - return env;
> > + return xstrdup(env);
> > }
> >
> > return system_path(GIT_EXEC_PATH);
> > @@ -99,10 +99,13 @@ void setup_path(void)
> > {
> > const char *old_path = getenv("PATH");
> > struct strbuf new_path = STRBUF_INIT;
> > + char *exec_path = (char *) git_exec_path();
> >
> > - add_path(&new_path, git_exec_path());
> > + add_path(&new_path, exec_path);
> > add_path(&new_path, argv0_path);
> >
> > + free(exec_path);
> > +
> > if (old_path)
> > strbuf_addstr(&new_path, old_path);
> > elseCarlos Martín Nieto <cmn@elego.de>