bug: conflicting core.bare setting causes segfault during bare clone

bug: conflicting core.bare setting causes segfault during bare clone

[Vusich, Joseph]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

$ env | grep GIT

$ git config --list
user.email=jvusich@amazon.com
user.name=Joseph Vusich
core.bare=false

$ git clone --bare https://github.com/josephvusich/fixture
Cloning into bare repository 'fixture.git'...
zsh: segmentation fault  /opt/local/bin/git clone --bare https://github.com/josephvusich/fixture

What did you expect to happen? (Expected behavior)

"git clone --bare" should clone a bare repository, regardless of the core.bare setting in the global config

What happened instead? (Actual behavior)

"git clone --bare" causes a segfault if the global gitconfig has core.bare=false

Anything else you want to add:

* Non-bare clones still work when core.bare=false
* Bare clones only work when core.bare=true, or if core.bare is absent from gitconfig
* Started failing with Git 2.30.0, also fails on 2.30.1, but succeeds on 2.29.2
* Issue reproduced on multiple machines, running Ubuntu, Mac OS, and Alpine Linux
* Running with all TRACEs enabled shows only one additional line prior to "Cloning"
    15:38:08.567292 git.c:444  trace: built-in: git clone --bare https://github.com/josephvusich/fixture
* The fixture.git directory is created prior to the segfault, the fixture.git/config file content is below:
  [core]
    repositoryformatversion = 0
    filemode = true
    bare = false
    logallrefupdates = true

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.30.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Darwin 19.6.0 Darwin Kernel Version 19.6.0: Tue Jan 12 22:13:05 PST 2021; root:xnu-6153.141.16~1/RELEASE_X86_64 x86_64
compiler info: clang: 12.0.0 (clang-1200.0.32.2)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]
not run from a git repository - no hooks to show

Re: bug: conflicting core.bare setting causes segfault during bare clone

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]

On 2021-03-03 at 00:06:01, Vusich, Joseph wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
> 
> What did you do before the bug happened? (Steps to reproduce your issue)
> 
> $ env | grep GIT
> 
> $ git config --list
> user.email=jvusich@amazon.com
> user.name=Joseph Vusich
> core.bare=false
> 
> $ git clone --bare https://github.com/josephvusich/fixture
> Cloning into bare repository 'fixture.git'...
> zsh: segmentation fault  /opt/local/bin/git clone --bare https://github.com/josephvusich/fixture
> 
> What did you expect to happen? (Expected behavior)
> 
> "git clone --bare" should clone a bare repository, regardless of the core.bare setting in the global config
> 
> What happened instead? (Actual behavior)
> 
> "git clone --bare" causes a segfault if the global gitconfig has core.bare=false

I appreciate the report, and I can reproduce with 45526154a5 from next.
We should definitely not segfault in this case.  I completely agree that
segfaulting is the wrong behavior (because it's always the wrong
behavior for a command-line tool).

I do, however, think we should either ignore core.bare in the global
configuration, if we don't already, or produce an error.  The
documentation says, "If true this repository is assumed to be bare" and
much like we ignore certain other settings outside of .git/config, we
should ignore this one.

I also agree that "git clone --bare" should clone a bare repository
since we'd ignore the global setting.

I'm probably not going to get a chance to look at this before the
weekend, so anyone is free to come up with a patch in the meantime, but
if nobody has gotten to it then, I'll try to send out a patch.

I will admit being a bit interested in how this was discovered, since it
seems like an odd configuration to have, so if you can share, I'd
appreciate it, if only to satisfy my curiosity.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: bug: conflicting core.bare setting causes segfault during bare clone

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I will admit being a bit interested in how this was discovered, since it
> seems like an odd configuration to have, so if you can share, I'd
> appreciate it, if only to satisfy my curiosity.

I had the same reaction.  Forcing everything to be bare is quite
unusual.

Re: bug: conflicting core.bare setting causes segfault during bare clone

[Vusich, Joseph]

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I will admit being a bit interested in how this was discovered, since it
> seems like an odd configuration to have, so if you can share, I'd
> appreciate it, if only to satisfy my curiosity.

Quite by accident. A machine-generated .gitconfig with some "default"
values, which inadvertently included core.bare=false.


> "Junio C Hamano" <gitster@pobox.com> wrote:

> I had the same reaction.  Forcing everything to be bare is quite
> unusual.

Technically non-bare in this case, although the field seems to be ignored
in the global config, at least prior to Git 2.30.0. In any case, it never
had any observable effect, and was therefore never noticed.

[PATCH] builtin/init-db: handle bare clones when core.bare set to false

[brian m. carlson]

In 552955ed7f ("clone: use more conventional config/option layering",
2020-10-01), clone learned to read configuration options earlier in its
execution, before creating the new repository.  However, that led to a
problem: if the core.bare setting is set to false in the global config,
cloning a bare repository segfaults.  This happens because the
repository is falsely thought to be non-bare, but clone has set the work
tree to NULL, which is then dereferenced.

The code to initialize the repository already considers the fact that a
user might want to override the --bare option for git init, but it
doesn't take into account clone, which uses a different option.  Let's
just check that the work tree is not NULL, since that's how clone
indicates that the repository is bare.  This is also the case for git
init, so we won't be regressing that case.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
It appears from my reading of the init code that we did intentionally
already honor core.bare in some case, so I have not implemented ignoring
that here and instead went for the limited approach.

 builtin/init-db.c        | 4 ++--
 t/t5606-clone-options.sh | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index dcc45bef51..f82efe4aff 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -212,6 +212,7 @@ static int create_default_files(const char *template_path,
 	int reinit;
 	int filemode;
 	struct strbuf err = STRBUF_INIT;
+	const char *work_tree = get_git_work_tree();
 
 	/* Just look for `init.templatedir` */
 	init_db_template_dir = NULL; /* re-set in case it was set before */
@@ -235,7 +236,7 @@ static int create_default_files(const char *template_path,
 	 * We must make sure command-line options continue to override any
 	 * values we might have just re-read from the config.
 	 */
-	is_bare_repository_cfg = init_is_bare_repository;
+	is_bare_repository_cfg = init_is_bare_repository || !work_tree;
 	if (init_shared_repository != -1)
 		set_shared_repository(init_shared_repository);
 
@@ -299,7 +300,6 @@ static int create_default_files(const char *template_path,
 	if (is_bare_repository())
 		git_config_set("core.bare", "true");
 	else {
-		const char *work_tree = get_git_work_tree();
 		git_config_set("core.bare", "false");
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == LOG_REFS_UNSET)
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 52e5789fb0..9b62708d76 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -104,6 +104,13 @@ test_expect_success 'redirected clone -v does show progress' '
 
 '
 
+test_expect_success 'clone does not segfault with --bare and core.bare=false' '
+	test_config_global core.bare false &&
+	git clone --bare "file://$(pwd)/parent" clone-bare &&
+	git -C clone-bare rev-parse --is-bare-repository >is-bare &&
+	test "$(cat is-bare)" = true
+'
+
 test_expect_success 'chooses correct default initial branch name' '
 	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
 	git -c init.defaultBranch=foo init --bare empty &&

Re: [PATCH] builtin/init-db: handle bare clones when core.bare set to false

[Eric Sunshine]

On Mon, Mar 8, 2021 at 8:18 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> In 552955ed7f ("clone: use more conventional config/option layering",
> 2020-10-01), clone learned to read configuration options earlier in its
> execution, before creating the new repository.  However, that led to a
> problem: if the core.bare setting is set to false in the global config,
> cloning a bare repository segfaults.  This happens because the
> repository is falsely thought to be non-bare, but clone has set the work
> tree to NULL, which is then dereferenced.
> [...]
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---

Perhaps this deserves a:

    Reported-by: Joseph Vusich <jvusich@amazon.com>

> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> @@ -104,6 +104,13 @@ test_expect_success 'redirected clone -v does show progress' '
> +test_expect_success 'clone does not segfault with --bare and core.bare=false' '
> +       test_config_global core.bare false &&
> +       git clone --bare "file://$(pwd)/parent" clone-bare &&

Can this be done more simply as:

    git clone --bare parent clone-bare &&

or even:

    git clone --bare . clone-bare &&

without mucking about with $(pwd)?

> +       git -C clone-bare rev-parse --is-bare-repository >is-bare &&
> +       test "$(cat is-bare)" = true

These days, we'd probably say:

    echo true >expect &&
    git -C clone-bare rev-parse --is-bare-repository >actual &&
    test_cmp expect actual

but it's subjective and minor; not at all worth a re-roll.

Re: [PATCH] builtin/init-db: handle bare clones when core.bare set to false

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

On 2021-03-08 at 16:43:58, Eric Sunshine wrote:
> On Mon, Mar 8, 2021 at 8:18 AM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > In 552955ed7f ("clone: use more conventional config/option layering",
> > 2020-10-01), clone learned to read configuration options earlier in its
> > execution, before creating the new repository.  However, that led to a
> > problem: if the core.bare setting is set to false in the global config,
> > cloning a bare repository segfaults.  This happens because the
> > repository is falsely thought to be non-bare, but clone has set the work
> > tree to NULL, which is then dereferenced.
> > [...]
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> 
> Perhaps this deserves a:
> 
>     Reported-by: Joseph Vusich <jvusich@amazon.com>

Good point.  Will fix.

> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> > @@ -104,6 +104,13 @@ test_expect_success 'redirected clone -v does show progress' '
> > +test_expect_success 'clone does not segfault with --bare and core.bare=false' '
> > +       test_config_global core.bare false &&
> > +       git clone --bare "file://$(pwd)/parent" clone-bare &&
> 
> Can this be done more simply as:
> 
>     git clone --bare parent clone-bare &&
> 
> or even:
> 
>     git clone --bare . clone-bare &&
> 
> without mucking about with $(pwd)?

Sure.  I pulled the line from the test above, but I agree that's nicer.

> > +       git -C clone-bare rev-parse --is-bare-repository >is-bare &&
> > +       test "$(cat is-bare)" = true
> 
> These days, we'd probably say:
> 
>     echo true >expect &&
>     git -C clone-bare rev-parse --is-bare-repository >actual &&
>     test_cmp expect actual
> 
> but it's subjective and minor; not at all worth a re-roll.

There's enough nits to warrant a v2, so I can do one.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[PATCH v2] builtin/init-db: handle bare clones when core.bare set to false

[brian m. carlson]

In 552955ed7f ("clone: use more conventional config/option layering",
2020-10-01), clone learned to read configuration options earlier in its
execution, before creating the new repository.  However, that led to a
problem: if the core.bare setting is set to false in the global config,
cloning a bare repository segfaults.  This happens because the
repository is falsely thought to be non-bare, but clone has set the work
tree to NULL, which is then dereferenced.

The code to initialize the repository already considers the fact that a
user might want to override the --bare option for git init, but it
doesn't take into account clone, which uses a different option.  Let's
just check that the work tree is not NULL, since that's how clone
indicates that the repository is bare.  This is also the case for git
init, so we won't be regressing that case.

Reported-by: Joseph Vusich <jvusich@amazon.com>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/init-db.c        | 4 ++--
 t/t5606-clone-options.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index dcc45bef51..f82efe4aff 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -212,6 +212,7 @@ static int create_default_files(const char *template_path,
 	int reinit;
 	int filemode;
 	struct strbuf err = STRBUF_INIT;
+	const char *work_tree = get_git_work_tree();
 
 	/* Just look for `init.templatedir` */
 	init_db_template_dir = NULL; /* re-set in case it was set before */
@@ -235,7 +236,7 @@ static int create_default_files(const char *template_path,
 	 * We must make sure command-line options continue to override any
 	 * values we might have just re-read from the config.
 	 */
-	is_bare_repository_cfg = init_is_bare_repository;
+	is_bare_repository_cfg = init_is_bare_repository || !work_tree;
 	if (init_shared_repository != -1)
 		set_shared_repository(init_shared_repository);
 
@@ -299,7 +300,6 @@ static int create_default_files(const char *template_path,
 	if (is_bare_repository())
 		git_config_set("core.bare", "true");
 	else {
-		const char *work_tree = get_git_work_tree();
 		git_config_set("core.bare", "false");
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == LOG_REFS_UNSET)
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 52e5789fb0..c2b71e78c5 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -104,6 +104,14 @@ test_expect_success 'redirected clone -v does show progress' '
 
 '
 
+test_expect_success 'clone does not segfault with --bare and core.bare=false' '
+	test_config_global core.bare false &&
+	git clone --bare parent clone-bare &&
+	echo true >expect &&
+	git -C clone-bare rev-parse --is-bare-repository >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'chooses correct default initial branch name' '
 	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
 	git -c init.defaultBranch=foo init --bare empty &&

Re: [PATCH v2] builtin/init-db: handle bare clones when core.bare set to false

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index 52e5789fb0..c2b71e78c5 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -104,6 +104,14 @@ test_expect_success 'redirected clone -v does show progress' '
>  
>  '
>  
> +test_expect_success 'clone does not segfault with --bare and core.bare=false' '
> +	test_config_global core.bare false &&
> +	git clone --bare parent clone-bare &&

Because "git clone" does so many different things from the normal
codepath in the "local" codepath, I'd prefer to see this one done
with the "--no-local" option (or alternatively, we could test both,
but that may be overkill).

> +	echo true >expect &&
> +	git -C clone-bare rev-parse --is-bare-repository >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'chooses correct default initial branch name' '
>  	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
>  	git -c init.defaultBranch=foo init --bare empty &&

Re: [PATCH v2] builtin/init-db: handle bare clones when core.bare set to false

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

On 2021-03-10 at 23:09:32, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> > index 52e5789fb0..c2b71e78c5 100755
> > --- a/t/t5606-clone-options.sh
> > +++ b/t/t5606-clone-options.sh
> > @@ -104,6 +104,14 @@ test_expect_success 'redirected clone -v does show progress' '
> >  
> >  '
> >  
> > +test_expect_success 'clone does not segfault with --bare and core.bare=false' '
> > +	test_config_global core.bare false &&
> > +	git clone --bare parent clone-bare &&
> 
> Because "git clone" does so many different things from the normal
> codepath in the "local" codepath, I'd prefer to see this one done
> with the "--no-local" option (or alternatively, we could test both,
> but that may be overkill).

I can reroll with that.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]