Re: [PATCH] setup.c: reset candidate->work_tree after freeing it

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 30, 2018, 17:41 UTC
Message-ID: <CACsJy8DS2pRaDhXgKqFQaqjFft_EW5G=8t9PhNcAgMsSDsS0jA@mail.gmail.com>
In-Reply-To: <xmqq7ept4hhj.fsf@gitster-ct.c.googlers.com>

On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 19 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Dangling pointers are usually bad news. Reset it back to NULL.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>
> Before abade65b ("setup: expose enumerated repo info", 2017-11-12),
> candidate was an on-stack variable local to this function, so there
> was no need to do anything before returning.  After that commit, we
> pass &repo_fmt down the codepath so theoretically the caller could
> peek into repo_fmt.work_tree after this codepath, which may be bad.
> But if candidate->work_tree were not NULL at this point, that would
> mean that such a caller that peeks is getting a WRONG information,
> no?  It thinks there were no core.worktree set but in reality there
> was the configuration set in the repository, no?
>
> Which fields in candidate are safe to peek by the caller?  How can a
> caller tell?

To me, all fields should be valid after check_repository_format_gently(). Right now the caller does not need to read any info from repo_fmt because check_repo... passes the information in another way, via global variables like is_bare_repository_cfg and git_work_tree_cfg.

> It seems that setup_git_directory_gently() uses repo_fmt when
> calling various variants of setup_*_git_dir() and then uses the
> repo_fmt.hash_algo field later.

Yes. Though if we're going to reduce global state further more, then the "if (!has_common)" should be done by the caller, then we need access to all fields in repo_fmt

Show 7 quoted lines

> If we want to keep fields of repo_fmt alive and valid after
> check_repository_format_gently() and callers of it like
> setup_*_git_dir() returns, then perhaps the right fix is not to free
> candidate->work_tree here, and instead give an interface to clean up
> repository_format instance, so that the ultimate caller like
> setup_git_directory_gently() can safely peek into any fields in it
> and then clean it up after it is done?

We still need to free and set NULL here though in addition to a cleanup interface. The reason is, when checking repo config from a worktree, we deliberately ignore core.worktree (which belongs to the main repo only). The implicit line near this free(candidate->work_tree) is "leave git_work_tree_cfg alone, we don't recognize core.worktree". Once we move setting git_work_tree_cfg out of this function, this becomes clear.

I didn't think all of this when I wrote this patch though. It was "hey, it's theoretical bug so let's fix it". Only later on when I refactored more that I realized what it meant.

-- 
Duy