Re: [PATCH] setup.c: reset candidate->work_tree after freeing it
On Fri, Mar 30, 2018 at 8:32 PM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano 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
Re: [PATCH] setup.c: reset candidate->work_tree after freeing it
Duy Nguyen writes: > On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano 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
On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> Dangling pointers are usually bad news. Reset it back to NULL. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- > > 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
Nguyễn Thái Ngọc Duy writes: > Dangling pointers are usually bad news. Reset it back to NULL. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- 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;