Re: [PATCH, maint] setup: make sure git_dir path is in a permanent buffer, getenv(3) case

From: Kirill Smelkov <kirr@mns.spb.ru>
Date: Nov 12, 2010, 14:03 UTC
Message-ID: <20101112140329.GA29604@tugrik.mns.mnsspb.ru>
In-Reply-To: <20101111181728.GF16972@burratino>

On Thu, Nov 11, 2010 at 12:17:28PM -0600, Jonathan Nieder wrote:

Show 6 quoted lines

> Kirill Smelkov wrote:
> 
> > getenv(3) returns not-permanent buffer which may be changed by e.g.
> > putenv(3) call (*).
> 
> Yikes.  Thanks for the example.

Nevermind. However it was not so fun to debug :)

Show 59 quoted lines

> > --- a/environment.c
> > +++ b/environment.c
> > @@ -88,6 +88,7 @@ const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {
> >  static void setup_git_env(void)
> >  {
> >  	git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > +	git_dir = git_dir ? xstrdup(git_dir) : NULL;
> >  	if (!git_dir) {
> >  		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> >  		git_dir = git_dir ? xstrdup(git_dir) : NULL;
> 
> Maybe we can avoid (some) repetition like this?
> 
> diff --git a/environment.c b/environment.c
> index de5581f..942f1e4 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -87,25 +87,31 @@ const char * const local_repo_env[LOCAL_REPO_ENV_SIZE + 1] = {
>  static void setup_git_env(void)
>  {
>  	git_dir = getenv(GIT_DIR_ENVIRONMENT);
> -	if (!git_dir) {
> -		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> -		git_dir = git_dir ? xstrdup(git_dir) : NULL;
> -	}
>  	if (!git_dir)
> +		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> +	if (git_dir)
> +		git_dir = xstrdup(git_dir);
> +	else
>  		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> +
>  	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 (git_object_dir)
> +		git_object_dir = xstrdup(git_object_dir);
> +	else
> +		git_object_dir = git_pathdup("objects");
> +
>  	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);
> -	}
> +	if (git_index_file)
> +		git_index_file = xstrdup(git_index_file);
> +	else
> +		git_index_file = git_pathdup("index");
> +
>  	git_graft_file = getenv(GRAFT_ENVIRONMENT);
> -	if (!git_graft_file)
> +	if (git_graft_file)
> +		git_graft_file = xstrdup(git_graft_file);
> +	else
>  		git_graft_file = git_pathdup("info/grafts");
> +

To me it gets hairy and we don't cover all and even most getenv cases. Look e.g. in commit.c:

    static void determine_author_info(void)
    {
            char *name, *email, *date;
    
            name = getenv("GIT_AUTHOR_NAME");
            email = getenv("GIT_AUTHOR_EMAIL");
            date = getenv("GIT_AUTHOR_DATE");

            /* ... */

            if (signoff) {
                    struct strbuf sob = STRBUF_INIT;
                    int i;
    
                    strbuf_addstr(&sob, sign_off_header);
                    strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
                                                 getenv("GIT_COMMITTER_EMAIL")));

            /* ... */

notes.c:

    struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
    {
            struct notes_rewrite_cfg *c = xmalloc(sizeof(struct notes_rewrite_cfg));
            const char *rewrite_mode_env = getenv(GIT_NOTES_REWRITE_MODE_ENVIRONMENT);
            const char *rewrite_refs_env = getenv(GIT_NOTES_REWRITE_REF_ENVIRONMENT);

editor.c:

    const char *git_editor(void)
    {
            const char *editor = getenv("GIT_EDITOR");
            const char *terminal = getenv("TERM");

http-backend.c:

    static void run_service(const char **argv)
    {
            const char *encoding = getenv("HTTP_CONTENT_ENCODING");
            const char *user = getenv("REMOTE_USER");
            const char *host = getenv("REMOTE_ADDR");

etc...

To me, it's very unfortunate that subsequent getenv() could overwrite previous getenv() result, but according to `man 3 getenv` all these places are buggy.

Maybe we'll need something like our own xgetenv() which will keep vars in some kind of hash tab so that get/put on other vars do not interfere with what was originally returned by xgetenv().

I don't know.

Unfortunately I can't afford myself to dive into all this, so please choose what you like more.

Thanks, Kirill