setup.c: reset candidate->work_tree after freeing it
PATCH 1/1 setup.c: reset candidate->work_tree after freeing it
Dangling pointers are usually bad news. Reset it back to NULL.
--- setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
setup.c +1 -1
@@ -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 ;
Signature · 1 line
2.17.0.rc2.408.g9d2a3d914e
Quoted 4 lines · 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 <[email protected]>---
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?
Quoted 16 lines
setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)setup.c +1
-1@@ -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 ;
Quoted 17 lines · Junio C Hamano
Quoted 4 lines · 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 <[email protected]>---
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_fmtdown the codepath so theoretically the caller could peek into repo_fmt.work_tree after this codepath, which may be bad. But ifcandidate->work_treewere 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?
Quoted 2 lines
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.
Quoted 3 lines
It seems that
setup_git_directory_gently()usesrepo_fmtwhen 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
Quoted 7 lines
If we want to keep fields of
repo_fmtalive and valid aftercheck_repository_format_gently()and callers of it like setup_*_git_dir() returns, then perhaps the right fix is not to freecandidate->work_treehere, and instead give an interface to clean uprepository_formatinstance, so that the ultimate caller likesetup_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.
Signature · 1 line
Duy
Quoted 7 lines · Duy Nguyen
Quoted 2 lines · Junio C Hamano
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.
Quoted 7 lines
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 "leavegit_work_tree_cfgalone, we don't recognize core.worktree". Once we move settinggit_work_tree_cfgout 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?
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Duy Nguyen
Quoted 2 lines · Junio C Hamano
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().
Quoted 8 lines
If so,
free()is wrong in the first place, andFREE_AND_NULL()is making it even worse, no? We learned there iswork_treeset to somewhere, the original code by Peff beforeabade65b("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.
Quoted 8 lines
Quoted 7 lines
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 "leavegit_work_tree_cfgalone, we don't recognize core.worktree". Once we move settinggit_work_tree_cfgout of this function, this becomes clear.
Quoted 8 lines
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.
Signature · 1 line
Duy