# [PATCH 0/2] fix a segfault in get_main_ref_store() 6 messages from 2018-05-18 to 2018-05-18. Participants: Jeff King, Taylor Blau, Stefan Beller, Johannes Schindelin. Thread: https://gitlist.dev/t/48518 ## Jeff King, 2018-05-18 22:25 Subject: [PATCH 0/2] fix a segfault in get_main_ref_store() Message-ID: <20180518222506.GA9527@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180518222506.GA9527%40sigill.intra.peff.net ``` I stumbled across a BUG() today. But interestingly, in the current tip of master it actually segfaults instead! This fixes the segfault (back into a BUG(), and then fixes the caller to avoid the BUG() in the first place). [1/2]: get_main_ref_store: BUG() when outside a repository [2/2]: config: die when --blob is used outside a repository builtin/config.c | 3 +++ refs.c | 3 +++ t/t1307-config-blob.sh | 4 ++++ 3 files changed, 10 insertions(+) -Peff ``` ## Jeff King, 2018-05-18 22:25 Subject: [PATCH 1/2] get_main_ref_store: BUG() when outside a repository Message-ID: <20180518222552.GA9623@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180518222552.GA9623%40sigill.intra.peff.net In-Reply-To: <20180518222506.GA9527@sigill.intra.peff.net> ``` If we don't have a repository, then we can't initialize the ref store. Prior to 64a741619d (refs: store the main ref store inside the repository struct, 2018-04-11), we'd try to access get_git_dir(), and outside a repository that would trigger a BUG(). After that commit, though, we directly use the_repository->git_dir; if it's NULL we'll just segfault. Let's catch this case and restore the BUG() behavior. Obviously we don't ever want to hit this code, but a BUG() is a lot more helpful than a segfault if we do. Signed-off-by: Jeff King --- refs.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/refs.c b/refs.c index 64aadd14c9..2e4a42f459 100644 --- a/refs.c +++ b/refs.c @@ -1668,6 +1668,9 @@ struct ref_store *get_main_ref_store(struct repository *r) if (r->refs) return r->refs; + if (!r->gitdir) + BUG("attempting to get main_ref_store outside of repository"); + r->refs = ref_store_init(r->gitdir, REF_STORE_ALL_CAPS); return r->refs; } -- 2.17.0.1052.g7d69f75dbf ``` ## Jeff King, 2018-05-18 22:27 Subject: [PATCH 2/2] config: die when --blob is used outside a repository Message-ID: <20180518222704.GB9623@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180518222704.GB9623%40sigill.intra.peff.net In-Reply-To: <20180518222506.GA9527@sigill.intra.peff.net> ``` If you run "config --blob" outside of a repository, then we eventually try to resolve the blob name and hit a BUG(). Let's catch this earlier and provide a useful message. Note that we could also catch this much lower in the stack, in git_config_from_blob_ref(). That might cover other callsites, too, but it's unclear whether those ones would actually be bugs or not. So let's leave the low-level functions to assume the caller knows what it's doing (and BUG() if it turns out it doesn't). Signed-off-by: Jeff King --- builtin/config.c | 3 +++ t/t1307-config-blob.sh | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/builtin/config.c b/builtin/config.c index 69e7270356..4155734f4a 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -602,6 +602,9 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (use_local_config && nongit) die(_("--local can only be used inside a git repository")); + if (given_config_source.blob && nongit) + die(_("--blob can only be used inside a git repository")); + if (given_config_source.file && !strcmp(given_config_source.file, "-")) { given_config_source.file = NULL; diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh index eed31ffa30..37dc689d8c 100755 --- a/t/t1307-config-blob.sh +++ b/t/t1307-config-blob.sh @@ -73,4 +73,8 @@ test_expect_success 'can parse blob ending with CR' ' test_cmp expect actual ' +test_expect_success 'config --blob outside of a repository is an error' ' + test_must_fail nongit git config --blob=foo --list +' + test_done -- 2.17.0.1052.g7d69f75dbf ``` ## Taylor Blau, 2018-05-18 22:31 Subject: Re: [PATCH 2/2] config: die when --blob is used outside a repository Message-ID: <20180518223134.GA56823@syl.local> URL: https://gitlist.dev/e/20180518223134.GA56823%40syl.local In-Reply-To: <20180518222704.GB9623@sigill.intra.peff.net> ``` On Fri, May 18, 2018 at 03:27:04PM -0700, Jeff King wrote: > If you run "config --blob" outside of a repository, then we > eventually try to resolve the blob name and hit a BUG(). > Let's catch this earlier and provide a useful message. I think that this approach is sensible. Let's (1) fix a SIGSEGV that should be a BUG(), and (2) make sure that we never get there in the first place. This looks fine to me. > Note that we could also catch this much lower in the stack, > in git_config_from_blob_ref(). That might cover other > callsites, too, but it's unclear whether those ones would > actually be bugs or not. So let's leave the low-level > functions to assume the caller knows what it's doing (and > BUG() if it turns out it doesn't). > > Signed-off-by: Jeff King Thanks, Taylor ``` ## Stefan Beller, 2018-05-18 22:51 Subject: Re: [PATCH 1/2] get_main_ref_store: BUG() when outside a repository Message-ID: URL: https://gitlist.dev/e/CAGZ79kbE5bdeFnQw%3DEiRw2iTw4mAs5%3DRefAh9oxYn9%2Byjy7YRw%40mail.gmail.com In-Reply-To: <20180518222552.GA9623@sigill.intra.peff.net> ``` On Fri, May 18, 2018 at 3:25 PM, Jeff King wrote: > If we don't have a repository, then we can't initialize the > ref store. Prior to 64a741619d (refs: store the main ref > store inside the repository struct, 2018-04-11), we'd try to > access get_git_dir(), and outside a repository that would > trigger a BUG(). After that commit, though, we directly use > the_repository->git_dir; if it's NULL we'll just segfault. > > Let's catch this case and restore the BUG() behavior. > Obviously we don't ever want to hit this code, but a BUG() > is a lot more helpful than a segfault if we do. That is true and an immediate bandaid; an alternative would be to do: if (!r->gitdir) /* not in a git directory ? */ return NULL; /* We signal the caller the absence of a git repo by NULL ness of the ref store */ However that we would need to catch at all callers of get_main_ref_store and error out accordingly. Then the trade off would be, how many callers to the main ref store do we have compared to options that rely on a ref store present? (I assume a patch like the second patch would show up in more cases now for all BUGs that we discover via this patch. The tradeoff is just if we want to report all these early by checking the config and system state, or wait until we get NULL ref store and then bail) I think checking early makes sense; I am not so sure about this patch; for the time being it makes sense, though in the long run, we rather want to catch this at a higher level: r->gitdir is supposedly never NULL, so we shall not produce this state. Maybe we want to set the_repository to NULL if set_git_dir fails (via repo_set_gitdir in setup_git_env, which both return void today). Enough of my rambling, the patches look good! Stefan ``` ## Johannes Schindelin, 2018-05-18 23:32 Subject: Re: [PATCH 0/2] fix a segfault in get_main_ref_store() Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1805190132170.77%40tvgsbejvaqbjf.bet In-Reply-To: <20180518222506.GA9527@sigill.intra.peff.net> ``` Hi Peff, On Fri, 18 May 2018, Jeff King wrote: > I stumbled across a BUG() today. But interestingly, in the current tip > of master it actually segfaults instead! This fixes the segfault (back > into a BUG(), and then fixes the caller to avoid the BUG() in the first > place). > > [1/2]: get_main_ref_store: BUG() when outside a repository > [2/2]: config: die when --blob is used outside a repository > > builtin/config.c | 3 +++ > refs.c | 3 +++ > t/t1307-config-blob.sh | 4 ++++ > 3 files changed, 10 insertions(+) Both patches look obviously correct to me. Thanks, Dscho ```