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@xxxxxxxx>
> ---
> 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
