{"thread":{"id":"48518","subject":"[PATCH 0/2] fix a segfault in get_main_ref_store()","startedAt":"2018-05-18T22:25:12Z","lastAt":"2018-05-18T23:32:47Z","messageCount":6,"participants":["Jeff King","Taylor Blau","Stefan Beller","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"348010","messageId":"20180518222506.GA9527@sigill.intra.peff.net","threadId":"48518","inReplyTo":null,"subject":"[PATCH 0/2] fix a segfault in get_main_ref_store()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-18T22:25:06Z","receivedAt":"2018-05-18T22:25:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I stumbled across a BUG() today. But interestingly, in the current tip\nof master it actually segfaults instead! This fixes the segfault (back\ninto a BUG(), and then fixes the caller to avoid the BUG() in the first\nplace).\n\n  [1/2]: get_main_ref_store: BUG() when outside a repository\n  [2/2]: config: die when --blob is used outside a repository\n\n builtin/config.c       | 3 +++\n refs.c                 | 3 +++\n t/t1307-config-blob.sh | 4 ++++\n 3 files changed, 10 insertions(+)\n\n-Peff\n"},{"id":"348011","messageId":"20180518222552.GA9623@sigill.intra.peff.net","threadId":"48518","inReplyTo":"20180518222506.GA9527@sigill.intra.peff.net","subject":"[PATCH 1/2] get_main_ref_store: BUG() when outside a repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-18T22:25:53Z","receivedAt":"2018-05-18T22:25:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we don't have a repository, then we can't initialize the\nref store.  Prior to 64a741619d (refs: store the main ref\nstore inside the repository struct, 2018-04-11), we'd try to\naccess get_git_dir(), and outside a repository that would\ntrigger a BUG(). After that commit, though, we directly use\nthe_repository->git_dir; if it's NULL we'll just segfault.\n\nLet's catch this case and restore the BUG() behavior.\nObviously we don't ever want to hit this code, but a BUG()\nis a lot more helpful than a segfault if we do.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 64aadd14c9..2e4a42f459 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1668,6 +1668,9 @@ struct ref_store *get_main_ref_store(struct repository *r)\n \tif (r->refs)\n \t\treturn r->refs;\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 }\n-- \n2.17.0.1052.g7d69f75dbf\n\n"},{"id":"348012","messageId":"20180518222704.GB9623@sigill.intra.peff.net","threadId":"48518","inReplyTo":"20180518222506.GA9527@sigill.intra.peff.net","subject":"[PATCH 2/2] config: die when --blob is used outside a repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-18T22:27:04Z","receivedAt":"2018-05-18T22:27:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If you run \"config --blob\" outside of a repository, then we\neventually try to resolve the blob name and hit a BUG().\nLet's catch this earlier and provide a useful message.\n\nNote that we could also catch this much lower in the stack,\nin git_config_from_blob_ref(). That might cover other\ncallsites, too, but it's unclear whether those ones would\nactually be bugs or not. So let's leave the low-level\nfunctions to assume the caller knows what it's doing (and\nBUG() if it turns out it doesn't).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/config.c       | 3 +++\n t/t1307-config-blob.sh | 4 ++++\n 2 files changed, 7 insertions(+)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 69e7270356..4155734f4a 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -602,6 +602,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \tif (use_local_config && nongit)\n \t\tdie(_(\"--local can only be used inside a git repository\"));\n \n+\tif (given_config_source.blob && nongit)\n+\t\tdie(_(\"--blob can only be used inside a git repository\"));\n+\n \tif (given_config_source.file &&\n \t\t\t!strcmp(given_config_source.file, \"-\")) {\n \t\tgiven_config_source.file = NULL;\ndiff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh\nindex eed31ffa30..37dc689d8c 100755\n--- a/t/t1307-config-blob.sh\n+++ b/t/t1307-config-blob.sh\n@@ -73,4 +73,8 @@ test_expect_success 'can parse blob ending with CR' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'config --blob outside of a repository is an error' '\n+\ttest_must_fail nongit git config --blob=foo --list\n+'\n+\n test_done\n-- \n2.17.0.1052.g7d69f75dbf\n"},{"id":"348014","messageId":"20180518223134.GA56823@syl.local","threadId":"48518","inReplyTo":"20180518222704.GB9623@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] config: die when --blob is used outside a repository","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-05-18T22:31:34Z","receivedAt":"2018-05-18T22:31:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, May 18, 2018 at 03:27:04PM -0700, Jeff King wrote:\n> If you run \"config --blob\" outside of a repository, then we\n> eventually try to resolve the blob name and hit a BUG().\n> Let's catch this earlier and provide a useful message.\n\nI think that this approach is sensible. Let's (1) fix a SIGSEGV that\nshould be a BUG(), and (2) make sure that we never get there in the\nfirst place.\n\nThis looks fine to me.\n\n> Note that we could also catch this much lower in the stack,\n> in git_config_from_blob_ref(). That might cover other\n> callsites, too, but it's unclear whether those ones would\n> actually be bugs or not. So let's leave the low-level\n> functions to assume the caller knows what it's doing (and\n> BUG() if it turns out it doesn't).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nThanks,\nTaylor\n"},{"id":"348015","messageId":"CAGZ79kbE5bdeFnQw=EiRw2iTw4mAs5=RefAh9oxYn9+yjy7YRw@mail.gmail.com","threadId":"48518","inReplyTo":"20180518222552.GA9623@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] get_main_ref_store: BUG() when outside a repository","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-18T22:51:17Z","receivedAt":"2018-05-18T22:51:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 18, 2018 at 3:25 PM, Jeff King <peff@peff.net> wrote:\n> If we don't have a repository, then we can't initialize the\n> ref store.  Prior to 64a741619d (refs: store the main ref\n> store inside the repository struct, 2018-04-11), we'd try to\n> access get_git_dir(), and outside a repository that would\n> trigger a BUG(). After that commit, though, we directly use\n> the_repository->git_dir; if it's NULL we'll just segfault.\n>\n> Let's catch this case and restore the BUG() behavior.\n> Obviously we don't ever want to hit this code, but a BUG()\n> is a lot more helpful than a segfault if we do.\n\nThat is true and an immediate bandaid; an alternative would\nbe to do:\n\n  if (!r->gitdir)\n    /* not in a git directory ? */\n    return NULL;\n    /* We signal the caller the absence of a git repo by NULL ness\n      of the ref store */\n\nHowever that we would need to catch at all callers of\nget_main_ref_store and error out accordingly.\n\nThen the trade off would be, how many callers to the main ref\nstore do we have compared to options that rely on a ref store\npresent? (I assume a patch like the second patch would show\nup in more cases now for all BUGs that we discover via this patch.\nThe tradeoff is just if we want to report all these early by checking\nthe config and system state, or wait until we get NULL ref store\nand then bail)\n\nI think checking early makes sense; I am not so sure about this\npatch; for the time being it makes sense, though in the long run,\nwe rather want to catch this at a higher level:\n\nr->gitdir is supposedly never NULL, so we shall not\nproduce this state. Maybe we want to set the_repository to NULL\nif set_git_dir fails (via repo_set_gitdir in setup_git_env, which both\nreturn void today).\n\nEnough of my rambling, the patches look good!\nStefan\n"},{"id":"348016","messageId":"nycvar.QRO.7.76.6.1805190132170.77@tvgsbejvaqbjf.bet","threadId":"48518","inReplyTo":"20180518222506.GA9527@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] fix a segfault in get_main_ref_store()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-18T23:32:35Z","receivedAt":"2018-05-18T23:32:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 18 May 2018, Jeff King wrote:\n\n> I stumbled across a BUG() today. But interestingly, in the current tip\n> of master it actually segfaults instead! This fixes the segfault (back\n> into a BUG(), and then fixes the caller to avoid the BUG() in the first\n> place).\n> \n>   [1/2]: get_main_ref_store: BUG() when outside a repository\n>   [2/2]: config: die when --blob is used outside a repository\n> \n>  builtin/config.c       | 3 +++\n>  refs.c                 | 3 +++\n>  t/t1307-config-blob.sh | 4 ++++\n>  3 files changed, 10 insertions(+)\n\nBoth patches look obviously correct to me.\n\nThanks,\nDscho\n"}]}