Re: GIT: [PATCH] exec_cmd: system_path memory leak fix

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2014, 18:28 UTC
Message-ID: <xmqqmw7hydoe.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1416750981-24446-1-git-send-email-kuleshovmail@gmail.com>

0xAX <kuleshovmail@gmail.com> writes:

Show 8 quoted lines

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