Re: [PATCH v3 1/4] environment.c: fix potential segfault by get_git_common_dir()
On Sun, Mar 19, 2017 at 12:54 AM, Junio C Hamanowrote: > Nguyễn Thái Ngọc Duy writes: > >> setup_git_env() must be called before this function to initialize >> git_common_dir so that it returns a non NULL string. And it must return >> a non NULL string or segfault can happen because all callers expect so. >> >> Normally if somebody has called get_git_dir(), or set_git_dir() then >> setup_git_env() is already called. But if you do setup_git_directory() >> at top dir (which skips set_git_dir) and never call get_git_dir, you'll >> get NULL here. > > Hmph, and the solution for the problem not being "so let's make sure > get_git_dir() is called even when the command is started at the top > directory" is because...? -EHARDTOPARSE. There's a hidden dependency between get_git_dir() and get_git_common_dir() which is not good. If we lazily call set_git_env(), make sure we do it lazily but consistently at all relevant function calls (i.e. including get_git_common_dir). Alternatively (I was thinking of this but didn't really follow up because this was side issue) we should make sure setup_git_env() is always called at the end of setup_git_dir...() and remove the laziness in get_git_dir(). This may be more in line of recent attempts to catch repo access without calling setup_git_directory..() first. But sadly I haven't read Jeff's series, so I can't say whether it's true. -- Duy
Re: [PATCH v3 1/4] environment.c: fix potential segfault by get_git_common_dir()
Nguyễn Thái Ngọc Duywrites: > setup_git_env() must be called before this function to initialize > git_common_dir so that it returns a non NULL string. And it must return > a non NULL string or segfault can happen because all callers expect so. > > Normally if somebody has called get_git_dir(), or set_git_dir() then > setup_git_env() is already called. But if you do setup_git_directory() > at top dir (which skips set_git_dir) and never call get_git_dir, you'll > get NULL here. Hmph, and the solution for the problem not being "so let's make sure get_git_dir() is called even when the command is started at the top directory" is because...? > test-ref-store.c will hit this problem because it's very lightweight, > just enough initialization to exercise refs code, and get_git_dir() will > never be called until get_worktrees() is, which uses get_git_common_dir(). > --- Missing sign-off. > environment.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/environment.c b/environment.c > index 42dc3106d2..2986ee7200 100644 > --- a/environment.c > +++ b/environment.c > @@ -214,6 +214,8 @@ const char *get_git_dir(void) > > const char *get_git_common_dir(void) > { > + if (!git_dir) > + setup_git_env(); > return git_common_dir; > }
[PATCH v3 1/4] environment.c: fix potential segfault by get_git_common_dir()
setup_git_env() must be called before this function to initialize git_common_dir so that it returns a non NULL string. And it must return a non NULL string or segfault can happen because all callers expect so. Normally if somebody has called get_git_dir(), or set_git_dir() then setup_git_env() is already called. But if you do setup_git_directory() at top dir (which skips set_git_dir) and never call get_git_dir, you'll get NULL here. test-ref-store.c will hit this problem because it's very lightweight, just enough initialization to exercise refs code, and get_git_dir() will never be called until get_worktrees() is, which uses get_git_common_dir(). --- environment.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/environment.c b/environment.c index 42dc3106d2..2986ee7200 100644 --- a/environment.c +++ b/environment.c @@ -214,6 +214,8 @@ const char *get_git_dir(void) const char *get_git_common_dir(void) { + if (!git_dir) + setup_git_env(); return git_common_dir; } -- 2.11.0.157.gd943d85