{"thread":{"id":"48186","subject":"[PATCH] setup.c: reset candidate->work_tree after freeing it","startedAt":"2018-03-30T07:08:04Z","lastAt":"2018-03-30T19:11:16Z","messageCount":5,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"343391","messageId":"20180330070744.22466-1-pclouds@gmail.com","threadId":"48186","inReplyTo":null,"subject":"[PATCH] setup.c: reset candidate->work_tree after freeing it","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-30T07:07:44Z","receivedAt":"2018-03-30T07:08:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Dangling pointers are usually bad news. Reset it back to NULL.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n setup.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/setup.c b/setup.c\nindex 7287779642..d193bee192 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -482,7 +482,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n \t\t\tinside_work_tree = -1;\n \t\t}\n \t} else {\n-\t\tfree(candidate->work_tree);\n+\t\tFREE_AND_NULL(candidate->work_tree);\n \t}\n \n \treturn 0;\n-- \n2.17.0.rc2.408.g9d2a3d914e\n\n"},{"id":"343438","messageId":"xmqq7ept4hhj.fsf@gitster-ct.c.googlers.com","threadId":"48186","inReplyTo":"20180330070744.22466-1-pclouds@gmail.com","subject":"Re: [PATCH] setup.c: reset candidate->work_tree after freeing it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T17:10:16Z","receivedAt":"2018-03-30T17:10:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Dangling pointers are usually bad news. Reset it back to NULL.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n\nBefore abade65b (\"setup: expose enumerated repo info\", 2017-11-12),\ncandidate was an on-stack variable local to this function, so there\nwas no need to do anything before returning. After that commit, we\npass &repo_fmt down the codepath so theoretically the caller could\npeek into repo_fmt.work_tree after this codepath, which may be bad.\nBut if candidate->work_tree were not NULL at this point, that would\nmean that such a caller that peeks is getting a WRONG information,\nno? It thinks there were no core.worktree set but in reality there\nwas the configuration set in the repository, no?\n\nWhich fields in candidate are safe to peek by the caller? How can a\ncaller tell?\n\nIt seems that setup_git_directory_gently() uses repo_fmt when\ncalling various variants of setup_*_git_dir() and then uses the\nrepo_fmt.hash_algo field later.\n\nIf we want to keep fields of repo_fmt alive and valid after\ncheck_repository_format_gently() and callers of it like\nsetup_*_git_dir() returns, then perhaps the right fix is not to free\ncandidate->work_tree here, and instead give an interface to clean up\nrepository_format instance, so that the ultimate caller like\nsetup_git_directory_gently() can safely peek into any fields in it\nand then clean it up after it is done?\n\n> setup.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 7287779642..d193bee192 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -482,7 +482,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_\n> \t\t\tinside_work_tree = -1;\n> \t\t}\n> \t} else {\n> -\t\tfree(candidate->work_tree);\n> +\t\tFREE_AND_NULL(candidate->work_tree);\n> \t}\n> \n> \treturn 0;\n"},{"id":"343442","messageId":"CACsJy8DS2pRaDhXgKqFQaqjFft_EW5G=8t9PhNcAgMsSDsS0jA@mail.gmail.com","threadId":"48186","inReplyTo":"xmqq7ept4hhj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] setup.c: reset candidate->work_tree after freeing it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-30T17:41:02Z","receivedAt":"2018-03-30T17:41:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Dangling pointers are usually bad news. Reset it back to NULL.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>\n> Before abade65b (\"setup: expose enumerated repo info\", 2017-11-12),\n> candidate was an on-stack variable local to this function, so there\n> was no need to do anything before returning. After that commit, we\n> pass &repo_fmt down the codepath so theoretically the caller could\n> peek into repo_fmt.work_tree after this codepath, which may be bad.\n> But if candidate->work_tree were not NULL at this point, that would\n> mean that such a caller that peeks is getting a WRONG information,\n> no? It thinks there were no core.worktree set but in reality there\n> was the configuration set in the repository, no?\n>\n> Which fields in candidate are safe to peek by the caller? How can a\n> caller tell?\n\nTo me, all fields should be valid after\ncheck_repository_format_gently(). Right now the caller does not need\nto read any info from repo_fmt because check_repo... passes the\ninformation in another way, via global variables like\nis_bare_repository_cfg and git_work_tree_cfg.\n\n> It seems that setup_git_directory_gently() uses repo_fmt when\n> calling various variants of setup_*_git_dir() and then uses the\n> repo_fmt.hash_algo field later.\n\nYes. Though if we're going to reduce global state further more, then\nthe \"if (!has_common)\" should be done by the caller, then we need\naccess to all fields in repo_fmt\n\n> If we want to keep fields of repo_fmt alive and valid after\n> check_repository_format_gently() and callers of it like\n> setup_*_git_dir() returns, then perhaps the right fix is not to free\n> candidate->work_tree here, and instead give an interface to clean up\n> repository_format instance, so that the ultimate caller like\n> setup_git_directory_gently() can safely peek into any fields in it\n> and then clean it up after it is done?\n\nWe still need to free and set NULL here though in addition to a\ncleanup interface. The reason is, when checking repo config from a\nworktree, we deliberately ignore core.worktree (which belongs to the\nmain repo only). The implicit line near this\nfree(candidate->work_tree) is \"leave git_work_tree_cfg alone, we don't\nrecognize core.worktree\". Once we move setting git_work_tree_cfg out\nof this function, this becomes clear.\n\nI didn't think all of this when I wrote this patch though. It was\n\"hey, it's theoretical bug so let's fix it\". Only later on when I\nrefactored more that I realized what it meant.\n-- \nDuy\n"},{"id":"343446","messageId":"xmqqo9j52z4m.fsf@gitster-ct.c.googlers.com","threadId":"48186","inReplyTo":"CACsJy8DS2pRaDhXgKqFQaqjFft_EW5G=8t9PhNcAgMsSDsS0jA@mail.gmail.com","subject":"Re: [PATCH] setup.c: reset candidate->work_tree after freeing it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T18:32:09Z","receivedAt":"2018-03-30T18:32:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano wrote:\n>\n>> Which fields in candidate are safe to peek by the caller? How can a\n>> caller tell?\n>\n> To me, all fields should be valid after\n> check_repository_format_gently().\n\nIf so, free() is wrong in the first place, and FREE_AND_NULL() is\nmaking it even worse, no? We learned there is work_tree set to\nsomewhere, the original code by Peff before abade65b (\"setup: expose\nenumerated repo info\", 2017-11-12) freed it because the code no\nlonger needed that piece of information. If we are passing all we\nlearned back to the caller, we should not free the field in the\nfunction at all. But it seems (below) the codepath is messier than\nthat.\n\n> We still need to free and set NULL here though in addition to a\n> cleanup interface. The reason is, when checking repo config from a\n> worktree, we deliberately ignore core.worktree (which belongs to the\n> main repo only). The implicit line near this\n> free(candidate->work_tree) is \"leave git_work_tree_cfg alone, we don't\n> recognize core.worktree\". Once we move setting git_work_tree_cfg out\n> of this function, this becomes clear.\n\nSo in other words, there is a code that looks at the field and it\n_wants_ to see NULL there---otherwise that brittle code misbehaves\nand FREE_AND_NULL() is a bad-aid to work it around?\n\nThen proposed log message \"leaving it dangling is unsanitary\" is\n*not* what is going on here, and the real reason why the code should\nbe like so deserve to be described both in the log message and in a\nlarge in-code comment, no?\n"},{"id":"343464","messageId":"CACsJy8BL6JbXj9in3CsRQcxG5Ru8GCBquaspEMvVUN6kq+FNzg@mail.gmail.com","threadId":"48186","inReplyTo":"xmqqo9j52z4m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] setup.c: reset candidate->work_tree after freeing it","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-03-30T19:10:39Z","receivedAt":"2018-03-30T19:11:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 30, 2018 at 8:32 PM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano wrote:\n>>\n>>> Which fields in candidate are safe to peek by the caller? How can a\n>>> caller tell?\n>>\n>> To me, all fields should be valid after\n>> check_repository_format_gently().\n>\n> If so, free() is wrong in the first place, and FREE_AND_NULL() is\n> making it even worse, no? We learned there is work_tree set to\n> somewhere, the original code by Peff before abade65b (\"setup: expose\n> enumerated repo info\", 2017-11-12) freed it because the code no\n> longer needed that piece of information. If we are passing all we\n> learned back to the caller, we should not free the field in the\n> function at all. But it seems (below) the codepath is messier than\n> that.\n\nActually no, NULL is the right value. I was trying to say that this\nmysterious code was about _deliberately_ ignore core.worktree. By\nkeeping repo_fmt->worktree as NULL we tell the caller \"core.worktree\nis not set\". The current code also does that but in a different way:\nit sets git_work_tree_cfg based on candidate->worktree, but only for\nthe \"!has_common\" block.\n\n>> We still need to free and set NULL here though in addition to a\n>> cleanup interface. The reason is, when checking repo config from a\n>> worktree, we deliberately ignore core.worktree (which belongs to the\n>> main repo only). The implicit line near this\n>> free(candidate->work_tree) is \"leave git_work_tree_cfg alone, we don't\n>> recognize core.worktree\". Once we move setting git_work_tree_cfg out\n>> of this function, this becomes clear.\n>\n> So in other words, there is a code that looks at the field and it\n> _wants_ to see NULL there---otherwise that brittle code misbehaves\n> and FREE_AND_NULL() is a bad-aid to work it around?\n>\n> Then proposed log message \"leaving it dangling is unsanitary\" is\n> *not* what is going on here, and the real reason why the code should\n> be like so deserve to be described both in the log message and in a\n> large in-code comment, no?\n\nLet's drop this for now. I'm a bit further along in refactoring this\ncode that I thought I could. It'll be clearer when the caller is also\nupdated to show what's wrong.\n-- \nDuy\n"}]}