Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
- From
Jeff King <peff@peff.net>
- Date
- Jun 24, 2014, 20:58 UTC
- Message-ID
- <20140624205815.GA28724@sigill.intra.peff.net>
- In-Reply-To
- <CACsJy8B-zQUH++U_RKq16_M+6FF5bmHXA100xM3uO42TUj3kJg@mail.gmail.com>
On Tue, Jun 24, 2014 at 08:30:26PM +0700, Duy Nguyen wrote:
Show 17 quoted lines
> On Fri, Jun 20, 2014 at 4:28 AM, Jeff King <peff@peff.net> wrote:
> > diff --git a/environment.c b/environment.c
> > index 4dac5e9..4de7b81 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -135,15 +135,11 @@ static void setup_git_env(void)
> > gitfile = read_gitfile(git_dir);
> > git_dir = xstrdup(gitfile ? gitfile : git_dir);
> > git_object_dir = getenv(DB_ENVIRONMENT);
> > - if (!git_object_dir) {
> > - git_object_dir = xmalloc(strlen(git_dir) + 9);
> > - sprintf(git_object_dir, "%s/objects", git_dir);
> > - }
>
> If DB_ENVIRONMENT is set, we should xstrdup(git_object_dir) because
> getenv's return value is not guaranteed persistent. Since you're touch
> this area, perhaps do it too (in this, or another patch)?Here's a replacement patch that handles this (and just drops the ugly mallocs as a side effect).
-- >8 -- Subject: [PATCH] setup_git_env: copy getenv return value
The return value of getenv is not guaranteed to survive across further invocations of setenv or even getenv. When we are assigning it to globals that last the lifetime of the program, we should make our own copy.
Signed-off-by: Jeff King <peff@peff.net> --- environment.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/environment.c b/environment.c index 4dac5e9..efb2fa0 100644 --- a/environment.c +++ b/environment.c @@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace) return strbuf_detach(&buf, NULL); } +static char *git_path_from_env(const char *envvar, const char *path) +{ + const char *value = getenv(envvar); + return value ? xstrdup(value) : git_pathdup(path); +} + static void setup_git_env(void) { const char *gitfile; @@ -134,19 +140,9 @@ static void setup_git_env(void) git_dir = DEFAULT_GIT_DIR_ENVIRONMENT; gitfile = read_gitfile(git_dir); git_dir = xstrdup(gitfile ? gitfile : git_dir); - git_object_dir = getenv(DB_ENVIRONMENT); - if (!git_object_dir) { - git_object_dir = xmalloc(strlen(git_dir) + 9); - sprintf(git_object_dir, "%s/objects", git_dir); - } - git_index_file = getenv(INDEX_ENVIRONMENT); - if (!git_index_file) { - git_index_file = xmalloc(strlen(git_dir) + 7); - sprintf(git_index_file, "%s/index", git_dir); - } - git_graft_file = getenv(GRAFT_ENVIRONMENT); - if (!git_graft_file) - git_graft_file = git_pathdup("info/grafts"); + git_object_dir = git_path_from_env(DB_ENVIRONMENT, "objects"); + git_index_file = git_path_from_env(INDEX_ENVIRONMENT, "index"); + git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, "info/grafts"); if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT)) check_replace_refs = 0; namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));
-- 2.0.0.566.gfe3e6b2