Re: [PATCH 2/3] setup_path(): Free temporary buffer

["Carlos Martín Nieto" <cmn@elego.de>]

On lun, 2011-03-14 at 16:09 -0400, Jeff King wrote:
> On Mon, Mar 14, 2011 at 08:18:37PM +0100, Carlos Martín Nieto wrote:
> 
> > Make sure the pointer git_exec_path() returns is in the heap and free
> > it after it's no longer needed.
> 
> Could you explain the motivation a bit more? From looking at the code,
> it looks like the situation is that git_exec_path sometimes returns a
> newly allocated string and sometimes not, so the caller may leak in the
> former case.

 I mostly wanted valgrind to shut up, but it seems there are a lot of
small leaks we don't really care about.

> 
> That seems to be the fault of system_path, which sometimes allocates and
> sometimes does not. Should we also be fixing system_path, which is a
> source of leaks? Or instead converting system_path to use a static
> buffer?

 Converting system_path() to use a static buffer would also work, and
would probably be an easier solution. I'll look at that tomorrow and
resend.

> 
> > @@ -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();
> 
> Ick. If it is now returning an allocated buffer that the caller is
> responsible for free-ing, then its declaration should drop the const in
> the return value.

 Yeah... I meant to delete that. It seems I've completely botched this
patch.

> 
> What about other callers of git_exec_path? Aren't load_command_list and
> list_commands leaking, too (and possibly worse, since we now always
> allocated memory)?

 Test-suite-induced tunnel vision. I only tested what reported a failure
(under valgrind) without the patch.

   cmn