Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2017, 01:25 UTC
Message-ID: <xmqqy3ylyqhf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kZ--jp08pK+xwn1N2VQQr8bA5+DveE2HsoY90R1gR6c_A@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 12 quoted lines

> On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> Technically, it is correct that git_exec_path() returns a possibly
>> malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
>> let's just use a static variable to make it a singleton. That'll shut
>> Coverity up, hopefully.
>
> I picked up this patch and applied it to the coverity branch
> that I maintain at github/stefanbeller/git.
>
> I'd love to see this patch upstream as it reduces my maintenance
> burden of the coverity branch by a patch.

So with the above, are you saying "Dscho said 'hopefully', and I confirm that this change does squelch misdiagnosis by Coverity"?

> If this patch is only to appease coverity (as the commit message eludes
> to) I think this may be a bad idea for upstream.  If this patch fixes an
> actual problem, then the commit message needs to spell that out.

That is true, and I see Peff pointed out another possible issue around getenv(), but I think from the "one step at a time" point of view, it is an improvement to call system_path() just once and cache it in "static char *".

How about explaining it like this then?

(only the log message has been corrected; diff is from the original).

commit c9bb5d101ca657fa466afa8c4368c43ea7b7aca8
Author: Johannes Schindelin <johannes.schindelin@gmx.de>
Date:   Mon Jan 2 17:22:33 2017 +0100

    git_exec_path: avoid Coverity warning about unfree()d result
    
    Technically, it is correct that git_exec_path() returns a possibly
    malloc()ed string returned from system_path(), and it is sometimes
    not allocated.  Cache the result in a static variable and make sure
    that we call system_path() only once, which plugs a potential leak.
    
    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/exec_cmd.c b/exec_cmd.c
index 9d5703a157..eae56fefba 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -69,6 +69,7 @@ void git_set_argv_exec_path(const char *exec_path)
 const char *git_exec_path(void)
 {
 	const char *env;
+	static char *system_exec_path;
 
 	if (argv_exec_path)
 		return argv_exec_path;
@@ -78,7 +79,9 @@ const char *git_exec_path(void)
 		return env;
 	}
 
-	return system_path(GIT_EXEC_PATH);
+	if (!system_exec_path)
+		system_exec_path = system_path(GIT_EXEC_PATH);
+	return system_exec_path;
 }
 
 static void add_path(struct strbuf *out, const char *path)