{"thread":{"id":"58211","subject":"git write-tree segfault with core.untrackedCache true and nonexistent index","startedAt":"2022-07-22T17:32:52Z","lastAt":"2022-07-22T22:00:28Z","messageCount":6,"participants":["Joey Hess","Martin Ågren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"459783","messageId":"YtrdPguYs3a3xekv@kitenet.net","threadId":"58211","inReplyTo":null,"subject":"git write-tree segfault with core.untrackedCache true and nonexistent index","fromName":"Joey Hess","fromEmail":"id@joeyh.name","sentAt":"2022-07-22T17:24:14Z","receivedAt":"2022-07-22T17:32:52Z","isPatch":false,"sender":{"key":"id@joeyh.name","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"\tjoey@darkstar:/tmp>git init emptyrepo\n\tInitialized empty Git repository in /tmp/emptyrepo/.git/\n\tjoey@darkstar:/tmp>cd emptyrepo/\n\tjoey@darkstar:/tmp/emptyrepo>git config core.untrackedCache true\n\tjoey@darkstar:/tmp/emptyrepo>git write-tree\n\tSegmentation fault\n\nI'm seeing this with git 2.37, 2.37.1, and HEAD. \n2.36.1 does not have the problem. \n\nNote that the index file does not exist prior to git write-tree in the above\ntest case. When the index does exist, it doesn't segfault. Before, it would\njust generate the empty tree when the index did not exist.\n\nBisecting, e6a653554bb49c26d105f3b478cbdbb1c0648f65 is the first bad commit\ncommit e6a653554bb49c26d105f3b478cbdbb1c0648f65\nAuthor: Tao Klerks \nDate: Thu Mar 31 16:02:15 2022 +0000\n\n untracked-cache: support '--untracked-files=all' if configured\n\n-- \nsee shy jo\n"},{"id":"459786","messageId":"20220722192559.718264-1-martin.agren@gmail.com","threadId":"58211","inReplyTo":"YtrdPguYs3a3xekv@kitenet.net","subject":"Re: git write-tree segfault with core.untrackedCache true and nonexistent index","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2022-07-22T19:25:59Z","receivedAt":"2022-07-22T19:26:31Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 22 Jul 2022 at 20:20, Joey Hess wrote:\n>\n> joey@darkstar:/tmp>git init emptyrepo\n> Initialized empty Git repository in /tmp/emptyrepo/.git/\n> joey@darkstar:/tmp>cd emptyrepo/\n> joey@darkstar:/tmp/emptyrepo>git config core.untrackedCache true\n> joey@darkstar:/tmp/emptyrepo>git write-tree\n> Segmentation fault\n>\n[...]\n>\n> Bisecting, e6a653554bb49c26d105f3b478cbdbb1c0648f65 is the first bad commit\n> commit e6a653554bb49c26d105f3b478cbdbb1c0648f65\n> Author: Tao Klerks \n> Date: Thu Mar 31 16:02:15 2022 +0000\n>\n> untracked-cache: support '--untracked-files=all' if configured\n\nThanks for a clear description, and for bisecting.\n\n`repo` is NULL in `new_untracked_cache_flags()` and we're not prepared\nfor that. The diff below fixes this in the sense that your reproducer\nstops failing, but I'm not sure it's the best approach.\n\nI can't help but think that e6a653554b was just unlucky enough to\ndereference `istate->repo` and that the real issue is that we're missing\n\n\tif (!istate->repo)\n\t\tistate->repo = the_repository;\n\nin some strategic place a fair bit earlier. It seems to me like the diff\nbelow is just papering over the real bug. It's not obvious to me where\nthat check would want to go, though. Tao, do you have an idea?\n\nMartin\n\n--- a/dir.c\n+++ b/dir.c\n@@ -2752,6 +2752,9 @@ static unsigned new_untracked_cache_flags(struct index_state *istate)\n \tstruct repository *repo = istate->repo;\n \tchar *val;\n \n+\tif (!repo)\n+\t\trepo = the_repository;\n+\n \t/*\n \t * This logic is coordinated with the setting of these flags in\n \t * wt-status.c#wt_status_collect_untracked(), and the evaluation\n-- \n2.37.1.455.g008518b4e5\n\n"},{"id":"459787","messageId":"xmqq35etc4vm.fsf@gitster.g","threadId":"58211","inReplyTo":"20220722192559.718264-1-martin.agren@gmail.com","subject":"Re: git write-tree segfault with core.untrackedCache true and nonexistent index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-22T19:41:01Z","receivedAt":"2022-07-22T19:41:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> I can't help but think that e6a653554b was just unlucky enough to\n> dereference `istate->repo` and that the real issue is that we're missing\n>\n> \tif (!istate->repo)\n> \t\tistate->repo = the_repository;\n>\n> in some strategic place a fair bit earlier. It seems to me like the diff\n> below is just papering over the real bug. It's not obvious to me where\n> that check would want to go, though. Tao, do you have an idea?\n\nI am not Tao, but thanks for starting to analyze the real issue.\n\nIt seems that there are two public entry points to dir.c API that\nend up calling new_untracked_cache_flags().\n\nOne is read_directory(), which is the only caller of\nvalidate_untracked_cache() that calls new_untracked_cache_flags().\nThe callers of read_directory() are supposed to give istate, and it\nis quite unlikely they are throwing an istate with NULL in\nistate->repo, simply because read_directory() already makes abundant\nuse of istate->repo.\n\nThe other one is add_untracked_cache().\n\nPerhaps backtrace to see where the istate came from would quickly\nreveal where the real issue lies?\n\nThanks.\n\n\n"},{"id":"459796","messageId":"20220722212232.833188-1-martin.agren@gmail.com","threadId":"58211","inReplyTo":"xmqq35etc4vm.fsf@gitster.g","subject":"[PATCH] read-cache: make `do_read_index()` always set up `istate->repo`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2022-07-22T21:22:32Z","receivedAt":"2022-07-22T21:23:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 22 Jul 2022 at 21:41, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> > I can't help but think that e6a653554b was just unlucky enough to\n> > dereference `istate->repo` and that the real issue is that we're missing\n> >\n> > if (!istate->repo)\n> > istate->repo = the_repository;\n> >\n> > in some strategic place a fair bit earlier. It seems to me like the diff\n>\n> Perhaps backtrace to see where the istate came from would quickly\n> reveal where the real issue lies?\n\nHere's an attempt at finding a proper spot for such a check. We end up\nwith a small amount of duplicated code, but I think it should be ok,\nespecially for a bugfix.\n\nThis is on top of tk/untracked-cache-with-uall and conflicts with\n491df5f679 (\"read-cache: set sparsity when index is new\", 2022-05-10).\nThe conflict could be resolved by taking my hunk before Victoria's\n[after which her helper function could be simplified accordingly] -- at\nleast that passes all the tests.\n\nI'm a bit out of my depth here. Hopefully one of the area experts can\nsay more about this approach.\n\nMartin\n\n-- >8 --\n\nIf there is no index file, e.g., because the repository has just been\ncreated, we return zero early (unless `must_exist` makes us die\ninstead.)\n\nThis early return means we do not set up `istate->repo`. With\n`core.untrackedCache=true`, the recent e6a653554b (\"untracked-cache:\nsupport '--untracked-files=all' if configured\", 2022-03-31) will\neventually pass down `istate->repo` as a null pointer to\n`repo_config_get_string()`, causing a segmentation fault.\n\nIf we do hit this early return, set up `istate->repo` similar to when we\nactually read the index.\n\nReported-by: Joey Hess \nSigned-off-by: Martin Ågren \n---\n read-cache.c | 5 ++++-\n t/t7063-status-untracked-cache.sh | 6 ++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 3e0e7d4183..68ed65035b 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2268,8 +2268,11 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n \tistate->timestamp.nsec = 0;\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (!must_exist && errno == ENOENT)\n+\t\tif (!must_exist && errno == ENOENT) {\n+\t\t\tif (!istate->repo)\n+\t\t\t\tistate->repo = the_repository;\n \t\t\treturn 0;\n+\t\t}\n \t\tdie_errno(_(\"%s: index file open failed\"), path);\n \t}\n \ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 9936cc329e..7dc5631f3d 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -985,4 +985,10 @@ test_expect_success '\"status\" after file replacement should be clean with UC=fal\n \tstatus_is_clean\n '\n \n+test_expect_success 'empty repo (no index) and core.untrackedCache' '\n+\tgit init emptyrepo &&\n+\tcd emptyrepo/ &&\n+\tgit -c core.untrackedCache=true write-tree\n+'\n+\n test_done\n-- \n2.37.1.455.g008518b4e5\n\n"},{"id":"459797","messageId":"xmqqtu78bz25.fsf@gitster.g","threadId":"58211","inReplyTo":"20220722212232.833188-1-martin.agren@gmail.com","subject":"Re: [PATCH] read-cache: make `do_read_index()` always set up `istate->repo`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-22T21:46:42Z","receivedAt":"2022-07-22T21:46:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> If there is no index file, e.g., because the repository has just been\n> created, we return zero early (unless `must_exist` makes us die\n> instead.)\n>\n> This early return means we do not set up `istate->repo`. With\n> `core.untrackedCache=true`, the recent e6a653554b (\"untracked-cache:\n> support '--untracked-files=all' if configured\", 2022-03-31) will\n> eventually pass down `istate->repo` as a null pointer to\n> `repo_config_get_string()`, causing a segmentation fault.\n>\n> If we do hit this early return, set up `istate->repo` similar to when we\n> actually read the index.\n>\n> Reported-by: Joey Hess \n> Signed-off-by: Martin Ågren \n> ---\n> read-cache.c | 5 ++++-\n> t/t7063-status-untracked-cache.sh | 6 ++++++\n> 2 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 3e0e7d4183..68ed65035b 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2268,8 +2268,11 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)\n> \tistate->timestamp.nsec = 0;\n> \tfd = open(path, O_RDONLY);\n> \tif (fd < 0) {\n> -\t\tif (!must_exist && errno == ENOENT)\n> +\t\tif (!must_exist && errno == ENOENT) {\n> +\t\t\tif (!istate->repo)\n> +\t\t\t\tistate->repo = the_repository;\n> \t\t\treturn 0;\n> +\t\t}\n\nMakes sense.\n\n> \t\tdie_errno(_(\"%s: index file open failed\"), path);\n> \t}\n> \n> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\n> index 9936cc329e..7dc5631f3d 100755\n> --- a/t/t7063-status-untracked-cache.sh\n> +++ b/t/t7063-status-untracked-cache.sh\n> @@ -985,4 +985,10 @@ test_expect_success '\"status\" after file replacement should be clean with UC=fal\n> \tstatus_is_clean\n> '\n> \n> +test_expect_success 'empty repo (no index) and core.untrackedCache' '\n> +\tgit init emptyrepo &&\n> +\tcd emptyrepo/ &&\n> +\tgit -c core.untrackedCache=true write-tree\n> +'\n\nI'll tweak this with \"-C emptyrepo\" so that future developers do not\nhave to get bitten when they add more tests to this script.\n\nTHanks for a quick fix. Very much appreciated.\n\n> test_done\n"},{"id":"459798","messageId":"CAN0heSqQLhM=mGhOVKyR+fqM3hm3na+dZhx+HPnq+UJFaGudVA@mail.gmail.com","threadId":"58211","inReplyTo":"xmqqtu78bz25.fsf@gitster.g","subject":"Re: [PATCH] read-cache: make `do_read_index()` always set up `istate->repo`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2022-07-22T22:00:11Z","receivedAt":"2022-07-22T22:00:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 22 Jul 2022 at 23:46, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> > +test_expect_success 'empty repo (no index) and core.untrackedCache' '\n> > + git init emptyrepo &&\n> > + cd emptyrepo/ &&\n> > + git -c core.untrackedCache=true write-tree\n> > +'\n>\n> I'll tweak this with \"-C emptyrepo\" so that future developers do not\n> have to get bitten when they add more tests to this script.\n\nYikes. I should have known better. Thanks for catching that. If a v2 is\nneeded for any reason, I'll include this change.\n\nMartin\n"}]}