Re: [PATCH 3/3] Replace setenv(GIT_DIR_ENVIRONMENT, ...) with set_git_dir()
Eventually I found some time to investigate this issue ...
On Nov 22, 2007, at 6:56 PM, Steffen Prohaska wrote:
Show 29 quoted lines
>
> 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.
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.
Show 10 quoted lines
>>> 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
Steffen Prohaska
