Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()

Steffen Prohaska [off-list ref] writes:

On Nov 22, 2007, at 3:34 AM, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

Does this not have a fundamental issue?  When you call other git
programs
with run_command(), you _need_ GIT_DIR to be set, no?

It is much worse.  set_git_dir() does not just setenv() but does
setup_git_env() as well.

What do your comments mean?

My understanding is that set_git_dir() sets the environment and
then calls setup_git_env() to cache all pointers.  This call
updates dangling pointer if they have been cached earlier.

Well, I was agreeing with you.  "Worse" was about what the
current code does _not_ do.

If there are earlier calls that obtain locations relative to the
earlier definition of GIT_DIR, the locations they obtained are
not just stored in memory that is "dangling" (which was what
your proposed log message described) but they are also
inconsistent with the updated definition of GIT_DIR.

I suspect Johannes mistook set_git_dir() was only local
(i.e. per calling process) matter without noticing that it has
its own setenv() when he made that comment, hence my response to
point out that the current code only calls setenv(), but
set_git_dir() does setup_git_env() too, which should hide the
inconsistency problem.

HOWEVER.

I suspect that if there are even earlier callers than these
early parts in the codepaths (handle_options, enter_repo, and
setup_git_directory_gently), maybe these earlier callers are
doing something wrong.  Logically, if you are somewhere very
early in the codepath that you can still change the value of
GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR
and cached locations relative to that directory, no?  What are
the problematic callers?  What values do they access and why?

On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:

Steffen Prohaska [off-list ref] writes:

quoted

On Nov 22, 2007, at 3:34 AM, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

Does this not have a fundamental issue?  When you call other git
programs
with run_command(), you _need_ GIT_DIR to be set, no?

It is much worse.  set_git_dir() does not just setenv() but does
setup_git_env() as well.

What do your comments mean?

My understanding is that set_git_dir() sets the environment and
then calls setup_git_env() to cache all pointers.  This call
updates dangling pointer if they have been cached earlier.

Well, I was agreeing with you.  "Worse" was about what the
current code does _not_ do.

If there are earlier calls that obtain locations relative to the
earlier definition of GIT_DIR, the locations they obtained are
not just stored in memory that is "dangling" (which was what
your proposed log message described) but they are also
inconsistent with the updated definition of GIT_DIR.

I suspect Johannes mistook set_git_dir() was only local
(i.e. per calling process) matter without noticing that it has
its own setenv() when he made that comment, hence my response to
point out that the current code only calls setenv(), but
set_git_dir() does setup_git_env() too, which should hide the
inconsistency problem.

HOWEVER.

I suspect that if there are even earlier callers than these
early parts in the codepaths (handle_options, enter_repo, and
setup_git_directory_gently), maybe these earlier callers are
doing something wrong.  Logically, if you are somewhere very
early in the codepath that you can still change the value of
GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR
and cached locations relative to that directory, no?  What are
the problematic callers?  What values do they access and why?

I thought about these questions, too.  But only very briefly.
I did not analyze the code path that lead to calls of getenv().

I'm not sure if it's really necessary.  Calling set_git_dir()
looks more sensible too me than the old code.  I believe using
set_git_dir() is the safer choice, and should not do any harm.
So I stopped analyzing too much, and instead proposed to use
set_git_dir().

Interesting, though, is to find out if we have other potentially
dangerous calls to getenv() that are not removed by this patch.

	Steffen

Steffen Prohaska schrieb:

On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:

quoted

I suspect that if there are even earlier callers than these
early parts in the codepaths (handle_options, enter_repo, and
setup_git_directory_gently), maybe these earlier callers are
doing something wrong.  Logically, if you are somewhere very
early in the codepath that you can still change the value of
GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR
and cached locations relative to that directory, no?  What are
the problematic callers?  What values do they access and why?

I thought about these questions, too.  But only very briefly.
I did not analyze the code path that lead to calls of getenv().

I'm not sure if it's really necessary.  Calling set_git_dir()
looks more sensible too me than the old code.  I believe using
set_git_dir() is the safer choice, and should not do any harm.
So I stopped analyzing too much, and instead proposed to use
set_git_dir().

Junio's point is this: If we stumble over a dangling pointer that getenv() 
produced, then this has obviously happened before setenv(GIT_DIR), and 
caching that pointer is probably the wrong thing to do anyway (because it 
refers to the wrong GIT_DIR) and needs to be fixed.

So the task is to find those traps. Dmitry obviously stumbled over one case, 
but I haven't ever encountered any problems with the current code. But then 
this might be sheer luck. And I'm not a heavy user of export GIT_DIR=foo, 
either. Do *you* know a problematic case?

Interesting, though, is to find out if we have other potentially
dangerous calls to getenv() that are not removed by this patch.

Side note for other readers: This is a Windows specific problem for the 
moment because its getenv() does not behave well.

-- Hannes

On Nov 22, 2007, at 10:58 AM, Johannes Sixt wrote:

Steffen Prohaska schrieb:

quoted

On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:

quoted

I suspect that if there are even earlier callers than these
early parts in the codepaths (handle_options, enter_repo, and
setup_git_directory_gently), maybe these earlier callers are
doing something wrong.  Logically, if you are somewhere very
early in the codepath that you can still change the value of
GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR
and cached locations relative to that directory, no?  What are
the problematic callers?  What values do they access and why?

I thought about these questions, too.  But only very briefly.
I did not analyze the code path that lead to calls of getenv().
I'm not sure if it's really necessary.  Calling set_git_dir()
looks more sensible too me than the old code.  I believe using
set_git_dir() is the safer choice, and should not do any harm.
So I stopped analyzing too much, and instead proposed to use
set_git_dir().

Junio's point is this: If we stumble over a dangling pointer that  
getenv() produced, then this has obviously happened before setenv 
(GIT_DIR), and caching that pointer is probably the wrong thing to  
do anyway (because it refers to the wrong GIT_DIR) and needs to be  
fixed.

I see your point.  It is probably more important to investigate
this than I recognized at a first glance.

So the task is to find those traps. Dmitry obviously stumbled over  
one case, but I haven't ever encountered any problems with the  
current code. But then this might be sheer luck. And I'm not a  
heavy user of export GIT_DIR=foo, either. Do *you* know a  
problematic case?

No.  I only stumbled over the code, when I reviewed differences
between msysgit and mingw.  I rarely use GIT_DIR=foo.  Actually,
I can't remember the last time I did.

quoted

Interesting, though, is to find out if we have other potentially
dangerous calls to getenv() that are not removed by this patch.

Side note for other readers: This is a Windows specific problem for  
the moment because its getenv() does not behave well.

Yes, and apparently even nobody knows how to trigger the problem
on Windows.  At this point, we only know that caching getenv()
calls is unsafe, while on Unix it is safe (at least for BSD
it's documented to be safe).

	Steffen

Hi,

On Thu, 22 Nov 2007, Steffen Prohaska wrote:

Yes, and apparently even nobody knows how to trigger the problem on 
Windows.

A quick and easy way would be to instrument getenv(), unsetenv() and 
setenv(), which would trigger an error.  Something like this (but you 
will have to put in a few "extern called_getenv; called_getenv = 0;", 
since already a simple git-init fails because of setup_path()):

-- snipsnap --
[PATCH] Instrument getenv(), setenv(), unsetenv() and putenv()

... for finding places where a pointer obtained by getenv() could
be invalidated later.

---

 environment.c     |    1 +
 git-compat-util.h |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/environment.c b/environment.c
index ce75e98..027340e 100644
--- a/environment.c
+++ b/environment.c

@@ -9,6 +9,7 @@
  */
 #include "cache.h"
 
+int called_setenv, called_getenv;
 char git_default_email[MAX_GITNAME];
 char git_default_name[MAX_GITNAME];
 int trust_executable_bit = 1;

diff --git a/git-compat-util.h b/git-compat-util.h
index 79eb10e..a41469b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h

@@ -427,4 +427,35 @@ static inline int strtol_i(char const *s, int base, int *result)
 	return 0;
 }
 
+extern int called_setenv, called_getenv;
+static inline char *test_getenv(const char *name)
+{
+	if (!called_setenv)
+		warning ("called test_getenv %s", name);
+	called_getenv = 1;
+	return getenv(name);
+}
+static inline int test_setenv(const char *name, const char *value, int overwrite)
+{
+	if (!called_setenv && called_getenv)
+		die ("getenv was called before setenv(%s, %s, %d)",
+				name, value, overwrite);
+	return setenv(name, value, overwrite);
+}
+static inline int test_unsetenv(const char *name)
+{
+	if (!called_setenv && called_getenv)
+		die ("getenv was called before unsetenv(%s)", name);
+	return unsetenv(name);
+}
+static inline int test_putenv(char *string)
+{
+	if (!called_setenv && called_getenv)
+		die ("getenv was called before putenv(%s)", string);
+	return putenv(string);
+}
+#define getenv test_getenv
+#define setenv test_setenv
+#define unsetenv test_unsetenv
+
 #endif

Eventually I found some time to investigate this issue ...


On Nov 22, 2007, at 6:56 PM, Steffen Prohaska wrote:

On Nov 22, 2007, at 10:58 AM, Johannes Sixt wrote:

quoted

Steffen Prohaska schrieb:

quoted

On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:

quoted

I suspect that if there are even earlier callers than these
early parts in the codepaths (handle_options, enter_repo, and
setup_git_directory_gently), maybe these earlier callers are
doing something wrong.  Logically, if you are somewhere very
early in the codepath that you can still change the value of
GIT_DIR, you shouldn't have assumed the unknown value of GIT_DIR
and cached locations relative to that directory, no?  What are
the problematic callers?  What values do they access and why?

I thought about these questions, too.  But only very briefly.
I did not analyze the code path that lead to calls of getenv().
I'm not sure if it's really necessary.  Calling set_git_dir()
looks more sensible too me than the old code.  I believe using
set_git_dir() is the safer choice, and should not do any harm.
So I stopped analyzing too much, and instead proposed to use
set_git_dir().

Junio's point is this: If we stumble over a dangling pointer that  
getenv() produced, then this has obviously happened before setenv 
(GIT_DIR), and caching that pointer is probably the wrong thing to  
do anyway (because it refers to the wrong GIT_DIR) and needs to be  
fixed.

I see your point.  It is probably more important to investigate
this than I recognized at a first glance.

I instrumented the code to verify if setenv(GIT_DIR) is called after
setup_git_env().  This is not the case for all tests.

I also searched for problematic code paths.

setup_git_directory_gently() looks correct.  It explicitly calls
getenv(GIT_DIR_ENVIRONMENT); but uses the value returned in a
safe manner.  It does not cache the result and the only code path
that calls set_git_dir() does not access the return value of the
getenv() call after the call to set_git_dir().

setup_work_tree() looks correct, too.  Here, get_git_dir() is
called, which implicitly results in caching the pointer returned
from getenv(GIT_DIR_ENVIRONMENT).  But the result of get_git_dir() is
neither cached nor used after a subsequent call to set_git_dir().

So, I don't find any obvious problems.

quoted

quoted

Interesting, though, is to find out if we have other potentially
dangerous calls to getenv() that are not removed by this patch.

Side note for other readers: This is a Windows specific problem  
for the moment because its getenv() does not behave well.

Yes, and apparently even nobody knows how to trigger the problem
on Windows.  At this point, we only know that caching getenv()
calls is unsafe, while on Unix it is safe (at least for BSD
it's documented to be safe).

In conclusion, using setenv() as in the original code instead of
set_git_dir() should be safe and this patch is not needed.

I tend to revert the changes in msysgit and see if we hit any
problems.  But I'll wait until 1.5.4 is released.

	Steffen

On Jan 1, 2008 10:52 AM, Steffen Prohaska [off-list ref] wrote:

In conclusion, using setenv() as in the original code instead of
set_git_dir() should be safe and this patch is not needed.

I tend to revert the changes in msysgit and see if we hit any
problems.  But I'll wait until 1.5.4 is released.

       Steffen

Please don't revert this change. I've made it in response to git clone
failing, commit 855f254b2b5b083a63fc8d7709a42e2cbdc5a136.

-- 
- Dmitry

On Jan 3, 2008, at 5:07 AM, Dmitry Kakurin wrote:

On Jan 1, 2008 10:52 AM, Steffen Prohaska [off-list ref] wrote:

quoted

In conclusion, using setenv() as in the original code instead of
set_git_dir() should be safe and this patch is not needed.

I tend to revert the changes in msysgit and see if we hit any
problems.  But I'll wait until 1.5.4 is released.

       Steffen

Please don't revert this change. I've made it in response to git clone
failing, commit 855f254b2b5b083a63fc8d7709a42e2cbdc5a136.

I know.  But I cannot reproduce the error.

Do you have a test case that demonstrates the problem?

I either want to see the patch upstream in official git or revert
it in msysgit.  But I cannot answer the questions that were
raised after I sent the patch (see earlier in this thread).  And
I can't see the problem that your patch solves, even after
spending some time on reading and instrumenting code.

	Steffen

On Jan 2, 2008 10:02 PM, Steffen Prohaska [off-list ref] wrote:

On Jan 3, 2008, at 5:07 AM, Dmitry Kakurin wrote:

quoted

On Jan 1, 2008 10:52 AM, Steffen Prohaska [off-list ref] wrote:

quoted

In conclusion, using setenv() as in the original code instead of
set_git_dir() should be safe and this patch is not needed.

I tend to revert the changes in msysgit and see if we hit any
problems.  But I'll wait until 1.5.4 is released.

       Steffen

Please don't revert this change. I've made it in response to git clone
failing, commit 855f254b2b5b083a63fc8d7709a42e2cbdc5a136.

I know.  But I cannot reproduce the error.

Do you have a test case that demonstrates the problem?

I either want to see the patch upstream in official git or revert
it in msysgit.  But I cannot answer the questions that were
raised after I sent the patch (see earlier in this thread).  And
I can't see the problem that your patch solves, even after
spending some time on reading and instrumenting code.

I remember that the problem was as simple as git clone or git clone
--bare failing.
Also I'm not sure if it matters but I'm running Vista.
There is also a chance that code has changed since then and this
problem went away.

-- 
- Dmitry

On Jan 3, 2008, at 7:26 AM, Dmitry Kakurin wrote:

On Jan 2, 2008 10:02 PM, Steffen Prohaska [off-list ref] wrote:

quoted

On Jan 3, 2008, at 5:07 AM, Dmitry Kakurin wrote:

quoted

On Jan 1, 2008 10:52 AM, Steffen Prohaska [off-list ref] wrote:

quoted

In conclusion, using setenv() as in the original code instead of
set_git_dir() should be safe and this patch is not needed.

I tend to revert the changes in msysgit and see if we hit any
problems.  But I'll wait until 1.5.4 is released.

       Steffen

Please don't revert this change. I've made it in response to git  
clone
failing, commit 855f254b2b5b083a63fc8d7709a42e2cbdc5a136.

I know.  But I cannot reproduce the error.

Do you have a test case that demonstrates the problem?

I either want to see the patch upstream in official git or revert
it in msysgit.  But I cannot answer the questions that were
raised after I sent the patch (see earlier in this thread).  And
I can't see the problem that your patch solves, even after
spending some time on reading and instrumenting code.

I remember that the problem was as simple as git clone or git clone
--bare failing.

This is what I understood from the commit message.

I need a script that I can run to see the error.  All tests that
come with git pass (on my machine).

Also I'm not sure if it matters but I'm running Vista.

It only matters if you see an error on Vista that I don't see on
XP.  If this was the case I'd debug on Vista.

There is also a chance that code has changed since then and this
problem went away.

Maybe.

	Steffen