{"thread":{"id":"53202","subject":"[BUG] segmentation fault in git-diff","startedAt":"2020-04-09T22:22:50Z","lastAt":"2020-04-10T05:39:27Z","messageCount":10,"participants":["Érico Rolim","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"395146","messageId":"CAFDeuWO+2JEGudtnHZvSsSUOVRR83U9ziLEjSoDyMWxYhvDMKg@mail.gmail.com","threadId":"53202","inReplyTo":null,"subject":"[BUG] segmentation fault in git-diff","fromName":"Érico Rolim","fromEmail":"erico.erc@gmail.com","sentAt":"2020-04-09T22:22:34Z","receivedAt":"2020-04-09T22:22:50Z","isPatch":false,"sender":{"key":"erico.erc@gmail.com","avatar":"https://gravatar.com/avatar/51213287b982c6a816313e6e9a4dd223404c2429f23a8ef763d9216ae6628605?d=mp&s=160"},"body":"Hey there!\n\nI have found a bug in the git-diff utility, which is reproducible in\nthe next branch. In any repository, if I run\n\n git diff :/any/path/\n\n(The important part is the trailing forward slash. No slash will\ngenerate either a valid diff or an error message about the path not\nbeing known. \":/\" also works without issue)\n\nit will trigger a SIGSEV. I have traced that back to the\nrefs_read_raw_ref() function, where it seems the ref_store parameter\npassed to it is 0x0 (according to GDB). It's always possible to include\na null-check in that function to fix the issue, but I don't think\nthat'd be the best solution. I can attempt to fix it, but I don't know\nwhat (and where) the proper solution would be, because I don't know what\nthe expected behavior is here, nor where exactly it should fail. Do you\nthink this could also warrant the creation of a test?\n\nI don't know what the best debug resources (valgrind output, core file\nfrom gdb) would be, but I can provide them if necessary.\n\nThanks,\n\nÉrico Nogueira\n"},{"id":"395147","messageId":"xmqqh7xsmg7j.fsf@gitster.c.googlers.com","threadId":"53202","inReplyTo":"CAFDeuWO+2JEGudtnHZvSsSUOVRR83U9ziLEjSoDyMWxYhvDMKg@mail.gmail.com","subject":"Re: [BUG] segmentation fault in git-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-09T22:45:04Z","receivedAt":"2020-04-09T22:45:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Érico Rolim writes:\n\n> I have found a bug in the git-diff utility, which is reproducible in\n> the next branch. In any repository, if I run\n>\n> git diff :/any/path/\n>\n> (The important part is the trailing forward slash. No slash will\n> generate either a valid diff or an error message about the path not\n> being known. \":/\" also works without issue)\n\nThis is 'next' running for me:\n\n $ git diff :/any/path\n fatal: ambiguous argument ':/any/path': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions, like this:\n 'git [...] -- [...]'\n\nIf looking for a commit that has a string \"any/path\" in its commit\nmessage may be consuming stack resources that are proportional to\nthe depth of the history, however, I can imagine that it might run\nout of the memory and die, though. Another thing that may be\ninteresting to try is to use \"git rev-parse :/any/path\" in the same\nrepository you are having trouble with. If the \"string :/any/path to\na revision\" logic is where it is dying, it would die the same way.\n\nThanks.\n\n\n\n"},{"id":"395148","messageId":"xmqqd08gmg3s.fsf@gitster.c.googlers.com","threadId":"53202","inReplyTo":"xmqqh7xsmg7j.fsf@gitster.c.googlers.com","subject":"Re: [BUG] segmentation fault in git-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-09T22:47:19Z","receivedAt":"2020-04-09T22:47:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This is 'next' running for me:\n\nScratch all that---sorry, but I did see the note about trailing\nslash, but somehow managed to forget adding it when I tried it.\n"},{"id":"395151","messageId":"xmqq4ktsmfnn.fsf@gitster.c.googlers.com","threadId":"53202","inReplyTo":"xmqqd08gmg3s.fsf@gitster.c.googlers.com","subject":"Re: [BUG] segmentation fault in git-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-09T22:57:00Z","receivedAt":"2020-04-09T22:57:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> This is 'next' running for me:\n>\n> Scratch all that---sorry, but I did see the note about trailing\n> slash, but somehow managed to forget adding it when I tried it.\n\n\n$ git checkout v2.22.0 && make && ./git-rev-parse :/any/path/\n\nsegfaults, while\n\n$ git checkout v2.21.0 && make && ./git-rev-parse :/any/path/\n\nis OK. We should be able to bisect this fairly straightforward\nbetween these two.\n\n\n"},{"id":"395154","messageId":"xmqqzhbkkz84.fsf@gitster.c.googlers.com","threadId":"53202","inReplyTo":"xmqq4ktsmfnn.fsf@gitster.c.googlers.com","subject":"Re: [BUG] segmentation fault in git-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-09T23:37:15Z","receivedAt":"2020-04-09T23:37:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The problem bisects down to c931ba4e (sha1-name.c: remove the_repo\nfrom handle_one_ref(), 2019-04-16), which did this:\n\n\n\n-\t\t\tfor_each_ref(handle_one_ref, &list);\n-\t\t\thead_ref(handle_one_ref, &list);\n+\t\t\tcb.repo = repo;\n+\t\t\tcb.list = &list;\n+\t\t\trefs_for_each_ref(repo->refs, handle_one_ref, &cb);\n+\t\t\trefs_head_ref(repo->refs, handle_one_ref, &cb);\n\nThe old code used the helper for_each_ref(). This is a thin wrapper\naround refs_for_each_ref() and allows the default repository object\nto be used implicitly by the caller. It is understandable that the\ncode wanted to work on arbitrary repository object, and replaced the\nfor_each_ref() helper with refs_for_each_ref() helper that takes any\nref store object.\n\nBut there is a small mistake. for_each_ref() makes sure that the\nref store is initialized; the new code blindly assumes it has\nalready been initialized.\n\n int for_each_ref(each_ref_fn fn, void *cb_data)\n {\n return refs_for_each_ref(get_main_ref_store(the_repository), fn, cb_data);\n }\n\nSo, I think the fix is simple. With the attached one liner on top\nof c931ba4e (sha1-name.c: remove the_repo from handle_one_ref(),\n2019-04-16),\n\n\t$ git rev-parse :/any/path/\n\nno longer segfaults. I think it would also work just fine when\nmerged to more modern codebase, but I haven't tried it (yet).\n\n\n\n\n sha1-name.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex d9050776dd..3aba62938f 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1771,7 +1771,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \n \t\t\tcb.repo = repo;\n \t\t\tcb.list = &list;\n-\t\t\trefs_for_each_ref(repo->refs, handle_one_ref, &cb);\n+\t\t\trefs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb);\n \t\t\trefs_head_ref(repo->refs, handle_one_ref, &cb);\n \t\t\tcommit_list_sort_by_date(&list);\n \t\t\treturn get_oid_oneline(repo, name + 2, oid, list);\n\n\n\n"},{"id":"395156","messageId":"20200409234152.GA42330@coredump.intra.peff.net","threadId":"53202","inReplyTo":"xmqq4ktsmfnn.fsf@gitster.c.googlers.com","subject":"Re: [BUG] segmentation fault in git-diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-09T23:41:52Z","receivedAt":"2020-04-09T23:41:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2020 at 03:57:00PM -0700, Junio C Hamano wrote:\n\n> $ git checkout v2.22.0 && make && ./git-rev-parse :/any/path/\n> \n> segfaults, while\n> \n> $ git checkout v2.21.0 && make && ./git-rev-parse :/any/path/\n> \n> is OK. We should be able to bisect this fairly straightforward\n> between these two.\n\nIndeed. The culprit is c931ba4e78 (sha1-name.c: remove the_repo from\nhandle_one_ref(), 2019-04-16), which swapped out for_each_ref() for\nrefs_for_each_ref(repo->refs). But it misses the access method for\nrepo->refs which lazy-initializes the pointer.\n\nSo the immediate fix is:\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex d9050776dd..c679e246cd 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1771,8 +1771,8 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \n \t\t\tcb.repo = repo;\n \t\t\tcb.list = &list;\n-\t\t\trefs_for_each_ref(repo->refs, handle_one_ref, &cb);\n-\t\t\trefs_head_ref(repo->refs, handle_one_ref, &cb);\n+\t\t\trefs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb);\n+\t\t\trefs_head_ref(get_main_ref_store(repo), handle_one_ref, &cb);\n \t\t\tcommit_list_sort_by_date(&list);\n \t\t\treturn get_oid_oneline(repo, name + 2, oid, list);\n \t\t}\n\nBut there are a bunch of other commits around the same time replacing\nthe_repository, and it seems like an easy mistake to make. Perhaps we\nshould rename the \"refs\" member of \"struct repository\" to something more\nclearly private, which would force callers to use the access method.\n\nI also wonder if there should be a repo_for_each_ref() which does it for\nus, though I guess there a bazillion variants (like head_ref()) that\nwould need similar treatment. Asking each caller to use\nget_main_ref_store() isn't too bad.\n\n-Peff\n"},{"id":"395158","messageId":"xmqqv9m8kxzy.fsf_-_@gitster.c.googlers.com","threadId":"53202","inReplyTo":"20200409234152.GA42330@coredump.intra.peff.net","subject":"Re* [BUG] segmentation fault in git-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-10T00:03:45Z","receivedAt":"2020-04-10T00:03:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But there are a bunch of other commits around the same time replacing\n> the_repository, and it seems like an easy mistake to make. Perhaps we\n> should rename the \"refs\" member of \"struct repository\" to something more\n> clearly private, which would force callers to use the access method.\n\nHere is the final version that I am going to apply and merge to\n'jch' branch. This is an ancient regression in Git timescale, so\nits fix is not all that urgent, though.\n\n-- >8 --\nSubject: [PATCH] sha1-name: do not assume that the ref store is initialized\n\nc931ba4e (sha1-name.c: remove the_repo from handle_one_ref(),\n2019-04-16) replaced the use of for_each_ref() helper, which works\nwith the main ref store of the default repository instance, with\nrefs_for_each_ref(), which can work on any ref store instance, by\nassuming that the repository instance the function is given has its\nref store already initialized.\n\nBut it is possible that nobody has initialized it, in which case,\nthe code ends up dereferencing a NULL pointer.\n\nReported-by: Érico Rolim \nSigned-off-by: Junio C Hamano \n---\n sha1-name.c | 2 +-\n t/t4208-log-magic-pathspec.sh | 4 ++++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex d9050776dd..3aba62938f 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1771,7 +1771,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \n \t\t\tcb.repo = repo;\n \t\t\tcb.list = &list;\n-\t\t\trefs_for_each_ref(repo->refs, handle_one_ref, &cb);\n+\t\t\trefs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb);\n \t\t\trefs_head_ref(repo->refs, handle_one_ref, &cb);\n \t\t\tcommit_list_sort_by_date(&list);\n \t\t\treturn get_oid_oneline(repo, name + 2, oid, list);\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 4c8f3b8e1b..6cdbe4747a 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -55,6 +55,10 @@ test_expect_success '\"git log -- :/a\" should not be ambiguous' '\n \tgit log -- :/a\n '\n \n+test_expect_success '\"git log :/any/path/\" should not segfault' '\n+\ttest_must_fail git log :/any/path/\n+'\n+\n # This differs from the \":/a\" check above in that :/in looks like a pathspec,\n # but doesn't match an actual file.\n test_expect_success '\"git log :/in\" should not be ambiguous' '\n-- \n2.26.0-106-g9fadedd637\n\n"},{"id":"395164","messageId":"CAFDeuWMpsZe-qUZD4=f0SLqfuPUhdER57WOhnpDH-TP-h2jArA@mail.gmail.com","threadId":"53202","inReplyTo":"xmqqv9m8kxzy.fsf_-_@gitster.c.googlers.com","subject":"Re: Re* [BUG] segmentation fault in git-diff","fromName":"Érico Rolim","fromEmail":"erico.erc@gmail.com","sentAt":"2020-04-10T01:42:32Z","receivedAt":"2020-04-10T01:42:46Z","isPatch":false,"sender":{"key":"erico.erc@gmail.com","avatar":"https://gravatar.com/avatar/51213287b982c6a816313e6e9a4dd223404c2429f23a8ef763d9216ae6628605?d=mp&s=160"},"body":"You guys sure are fast! Happy it could be solved somewhat easily.\n\nThanks!\n"},{"id":"395166","messageId":"20200410030411.GA48173@coredump.intra.peff.net","threadId":"53202","inReplyTo":"xmqqv9m8kxzy.fsf_-_@gitster.c.googlers.com","subject":"Re: Re* [BUG] segmentation fault in git-diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-10T03:04:11Z","receivedAt":"2020-04-10T03:04:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 09, 2020 at 05:03:45PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But there are a bunch of other commits around the same time replacing\n> > the_repository, and it seems like an easy mistake to make. Perhaps we\n> > should rename the \"refs\" member of \"struct repository\" to something more\n> > clearly private, which would force callers to use the access method.\n> \n> Here is the final version that I am going to apply and merge to\n> 'jch' branch. This is an ancient regression in Git timescale, so\n> its fix is not all that urgent, though.\n\nAgreed. The patch looks good to me.\n\nI prepared the patch below on top. I mostly wanted it as an auditing\ntool to find similar cases, but there were none. It may still be worth\napplying to protect ourselves in the future.\n\n-- >8 --\nSubject: [PATCH] repository: mark the \"refs\" pointer as private\n\nThe \"refs\" pointer in a struct repository starts life as NULL, but then\nis lazily initialized when it is accessed via get_main_ref_store().\nHowever, it's easy for calling code to forget this and access it\ndirectly, leading to code which works _some_ of the time, but fails if\nit is called before anybody else accesses the refs.\n\nThis was the cause of the bug fixed by 5ff4b920eb (sha1-name: do not\nassume that the ref store is initialized, 2020-04-09). In order to\nprevent similar bugs, let's more clearly mark the \"refs\" field as\nprivate.\n\nIn addition to helping future code, the name change will help us audit\nany existing direct uses. Besides get_main_ref_store() itself, it turns\nout there is only one. But we know it's OK as it is on the line directly\nafter the fix from 5ff4b920eb, which will have initialized the pointer.\nHowever it's still a good idea for it to model the proper use of the\naccessing function, so we'll convert it.\n\nSigned-off-by: Jeff King \n---\n refs.c | 8 ++++----\n repository.h | 8 ++++++--\n sha1-name.c | 2 +-\n 3 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1ab0bb54d3..b8759116cd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1852,14 +1852,14 @@ static struct ref_store *ref_store_init(const char *gitdir,\n \n struct ref_store *get_main_ref_store(struct repository *r)\n {\n-\tif (r->refs)\n-\t\treturn r->refs;\n+\tif (r->refs_private)\n+\t\treturn r->refs_private;\n \n \tif (!r->gitdir)\n \t\tBUG(\"attempting to get main_ref_store outside of repository\");\n \n-\tr->refs = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);\n-\treturn r->refs;\n+\tr->refs_private = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS);\n+\treturn r->refs_private;\n }\n \n /*\ndiff --git a/repository.h b/repository.h\nindex 040057dea6..6534fbb7b3 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -67,8 +67,12 @@ struct repository {\n \t */\n \tstruct parsed_object_pool *parsed_objects;\n \n-\t/* The store in which the refs are held. */\n-\tstruct ref_store *refs;\n+\t/*\n+\t * The store in which the refs are held. This should generally only be\n+\t * accessed via get_main_ref_store(), as that will lazily initialize\n+\t * the ref object.\n+\t */\n+\tstruct ref_store *refs_private;\n \n \t/*\n \t * Contains path to often used file names.\ndiff --git a/sha1-name.c b/sha1-name.c\nindex 878553b132..fccc97fa7a 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1816,7 +1816,7 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\t\tcb.repo = repo;\n \t\t\tcb.list = &list;\n \t\t\trefs_for_each_ref(get_main_ref_store(repo), handle_one_ref, &cb);\n-\t\t\trefs_head_ref(repo->refs, handle_one_ref, &cb);\n+\t\t\trefs_head_ref(get_main_ref_store(repo), handle_one_ref, &cb);\n \t\t\tcommit_list_sort_by_date(&list);\n \t\t\treturn get_oid_oneline(repo, name + 2, oid, list);\n \t\t}\n-- \n2.26.0.414.ge3a6455e3d\n\n"},{"id":"395168","messageId":"xmqqa73jlx0z.fsf@gitster.c.googlers.com","threadId":"53202","inReplyTo":"20200410030411.GA48173@coredump.intra.peff.net","subject":"Re: Re* [BUG] segmentation fault in git-diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-10T05:39:24Z","receivedAt":"2020-04-10T05:39:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I prepared the patch below on top. I mostly wanted it as an auditing\n> tool to find similar cases, but there were none. It may still be worth\n> applying to protect ourselves in the future.\n\nMakes perfect sense.\n\nWill queue. Thanks.\n"}]}