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

[Jeff King <peff@peff.net>]

On Sun, Jan 08, 2017 at 05:25:00PM -0800, Junio C Hamano wrote:

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

Yep, I don't think it's a big deal to do it on top, like this:

-- >8 --
Subject: git_exec_path: do not return the result of getenv()

The result of getenv() is not guaranteed by POSIX to last
beyond another call to getenv(), or setenv(), etc.  We
should duplicate the string before returning to the caller
to avoid any surprises.

We already keep a cached pointer to avoid repeatedly leaking
the result of system_path(). We can use the same pointer
here to avoid allocating and leaking for each call.

Signed-off-by: Jeff King <peff@peff.net>
---

To be honest, I do not know how big a problem this is. I looked at the
code paths that call git_exec_path(), and the most likely problem case
is calling a second getenv() is via the strbuf functions, which call
xmalloc(), which checks $GIT_ALLOC_LIMIT. We do cache that value, but
it would be a potential problem if this is the first xmalloc call in
the program.

But we are not really solving that here, as xstrdup() would have the
same problem.  This _is_ safer, in that we've better contained the
length of time that we expect the result to be valid.

I have no idea what platforms, if any, use a single static buffer for
the getenv() return. I don't know that we've ever gotten a bug report
about it (I only knew about it because somebody pointed it out in one
of my patches a few years ago, so I have it in the back of my mind as
a potential problem).

So I don't mind if this is dropped as "too esoteric" until somebody
actually reports a bug about it.

 exec_cmd.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 587bd7eb4..fb94aeba9 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -64,20 +64,19 @@ void git_set_argv_exec_path(const char *exec_path)
 /* Returns the highest-priority, location to look for git programs. */
 const char *git_exec_path(void)
 {
-	const char *env;
-	static char *system_exec_path;
+	static char *cached_exec_path;
 
 	if (argv_exec_path)
 		return argv_exec_path;
 
-	env = getenv(EXEC_PATH_ENVIRONMENT);
-	if (env && *env) {
-		return env;
+	if (!cached_exec_path) {
+		const char *env = getenv(EXEC_PATH_ENVIRONMENT);
+		if (env && *env)
+			cached_exec_path = xstrdup(env);
+		else
+			cached_exec_path = system_path(GIT_EXEC_PATH);
 	}
-
-	if (!system_exec_path)
-		system_exec_path = system_path(GIT_EXEC_PATH);
-	return system_exec_path;
+	return cached_exec_path;
 }
 
 static void add_path(struct strbuf *out, const char *path)
-- 
2.11.0.531.ge85397315