Re: [PATCH 09/10] set_git_dir: handle feeding gitdir to itself

[Brandon Williams <bmwill@google.com>]

On 09/05, Jeff King wrote:
> Ideally we'd free the existing gitdir field before assigning
> the new one, to avoid a memory leak. But we can't do so
> safely because some callers do the equivalent of:
> 
>   set_git_dir(get_git_dir());
> 
> We can detect that case as a noop, but there are even more
> complicated cases like:
> 
>   set_git_dir(remove_leading_path(worktree, get_git_dir());
> 
> where we really do need to do some work, but the original
> string must remain valid.
> 
> Rather than put the burden on callers to make a copy of the
> string (only to free it later, since we'll make a copy of it
> ourselves), let's solve the problem inside set_git_dir(). We
> can make a copy of the pointer for the old gitdir, and then
> avoid freeing it until after we've made our new copy.
> 

This patch and the one before it look good to me.  Thanks for fixing
this issue!

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  repository.c | 10 +++-------
>  setup.c      |  5 -----
>  2 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/repository.c b/repository.c
> index 52f1821c6b..97c732bd48 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -56,16 +56,12 @@ static void repo_setup_env(struct repository *repo)
>  void repo_set_gitdir(struct repository *repo, const char *path)
>  {
>  	const char *gitfile = read_gitfile(path);
> +	char *old_gitdir = repo->gitdir;
>  
> -	/*
> -	 * NEEDSWORK: Eventually we want to be able to free gitdir and the rest
> -	 * of the environment before reinitializing it again, but we have some
> -	 * crazy code paths where we try to set gitdir with the current gitdir
> -	 * and we don't want to free gitdir before copying the passed in value.
> -	 */
>  	repo->gitdir = xstrdup(gitfile ? gitfile : path);
> -
>  	repo_setup_env(repo);
> +
> +	free(old_gitdir);
>  }
>  
>  /*
> diff --git a/setup.c b/setup.c
> index 23950173fc..6d8380acd2 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -399,11 +399,6 @@ void setup_work_tree(void)
>  	if (getenv(GIT_WORK_TREE_ENVIRONMENT))
>  		setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
>  
> -	/*
> -	 * NEEDSWORK: this call can essentially be set_git_dir(get_git_dir())
> -	 * which can cause some problems when trying to free the old value of
> -	 * gitdir.
> -	 */
>  	set_git_dir(remove_leading_path(git_dir, work_tree));
>  	initialized = 1;
>  }
> -- 
> 2.14.1.721.gc5bc1565f1
> 

-- 
Brandon Williams