Re: [PATCH] exec_cmd: system_path memory leak fix

From: Alex Kuleshov <kuleshovmail@gmail.com>
Date: Nov 24, 2014, 07:02 UTC
Message-ID: <87mw7haxdp.fsf@gmail.com>
In-Reply-To: <xmqq7fyly3xj.fsf@gitster.dls.corp.google.com>

Jeff King:

Show 7 quoted lines

>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.

Show 7 quoted lines

>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