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

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

[Nguyễn Thái Ngọc Duy]

Dangling pointers are usually bad news. Reset it back to NULL.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 7287779642..d193bee192 100644
--- a/setup.c
+++ b/setup.c
@@ -482,7 +482,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
 			inside_work_tree = -1;
 		}
 	} else {
-		free(candidate->work_tree);
+		FREE_AND_NULL(candidate->work_tree);
 	}
 
 	return 0;
-- 
2.17.0.rc2.408.g9d2a3d914e

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

[Junio C Hamano]

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?

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.

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?

>  setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 7287779642..d193bee192 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -482,7 +482,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
>  			inside_work_tree = -1;
>  		}
>  	} else {
> -		free(candidate->work_tree);
> +		FREE_AND_NULL(candidate->work_tree);
>  	}
>  
>  	return 0;

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

[Duy Nguyen]

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

> 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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> 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().

If so, free() is wrong in the first place, and FREE_AND_NULL() is
making it even worse, no?  We learned there is work_tree set to
somewhere, the original code by Peff before abade65b ("setup: expose
enumerated repo info", 2017-11-12) freed it because the code no
longer needed that piece of information.  If we are passing all we
learned back to the caller, we should not free the field in the
function at all.  But it seems (below) the codepath is messier than
that.

> 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.

So in other words, there is a code that looks at the field and it
_wants_ to see NULL there---otherwise that brittle code misbehaves
and FREE_AND_NULL() is a bad-aid to work it around?

Then proposed log message "leaving it dangling is unsanitary" is
*not* what is going on here, and the real reason why the code should
be like so deserve to be described both in the log message and in a
large in-code comment, no?

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

[Duy Nguyen]

On Fri, Mar 30, 2018 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> 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().
>
> If so, free() is wrong in the first place, and FREE_AND_NULL() is
> making it even worse, no?  We learned there is work_tree set to
> somewhere, the original code by Peff before abade65b ("setup: expose
> enumerated repo info", 2017-11-12) freed it because the code no
> longer needed that piece of information.  If we are passing all we
> learned back to the caller, we should not free the field in the
> function at all.  But it seems (below) the codepath is messier than
> that.

Actually no, NULL is the right value. I was trying to say that this
mysterious code was about _deliberately_ ignore core.worktree. By
keeping repo_fmt->worktree as NULL we tell the caller "core.worktree
is not set". The current code also does that but in a different way:
it sets git_work_tree_cfg based on candidate->worktree, but only for
the "!has_common" block.

>> 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.
>
> So in other words, there is a code that looks at the field and it
> _wants_ to see NULL there---otherwise that brittle code misbehaves
> and FREE_AND_NULL() is a bad-aid to work it around?
>
> Then proposed log message "leaving it dangling is unsanitary" is
> *not* what is going on here, and the real reason why the code should
> be like so deserve to be described both in the log message and in a
> large in-code comment, no?

Let's drop this for now. I'm a bit further along in refactoring this
code that I thought I could. It'll be clearer when the caller is also
updated to show what's wrong.
-- 
Duy