From: Steffen Prohaska <prohaska@zib.de>
Date: Thu, 22 Nov 2007 17:56:23 GMT
Subject: Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()
Message-ID: <3B6B19E6-255F-4D8F-B6A3-255A9E8E0AB0@zib.de>
In-Reply-To: <474552D6.5060305@viscovery.net>


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

> Steffen Prohaska schrieb:
>> On Nov 22, 2007, at 8:52 AM, Junio C Hamano wrote:
>>> 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.


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