Stefan Beller [off-list ref] writes:
On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
[off-list ref] wrote:
quoted
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 [off-list ref]
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 [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
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)
On Sun, Jan 08, 2017 at 05:25:00PM -0800, Junio C Hamano wrote:
quoted
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 <redacted>
---
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
Hi,
On Sun, 8 Jan 2017, Junio C Hamano wrote:
How about explaining it like this then?
(only the log message has been corrected; diff is from the original).
commit c9bb5d101ca657fa466afa8c4368c43ea7b7aca8
Author: Johannes Schindelin [off-list ref]
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 [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
Sounds good to me.
Ciao,
Dscho
On Sun, Jan 8, 2017 at 5:25 PM, Junio C Hamano [off-list ref] wrote:
So with the above, are you saying "Dscho said 'hopefully', and I
confirm that this change does squelch misdiagnosis by Coverity"?
I could not find the coverity issue any more.
(It really misses easy access to "recently fixed" problems)
commit c9bb5d101ca657fa466afa8c4368c43ea7b7aca8
Author: Johannes Schindelin [off-list ref]
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 [off-list ref]
Signed-off-by: Junio C Hamano [off-list ref]
Sounds good to me,
Thanks,
Stefan