diff mbox series

scalar: avoid segfault in reconfigure --all

Message ID pull.1724.git.1714496333004.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series scalar: avoid segfault in reconfigure --all | expand

Commit Message

Derrick Stolee April 30, 2024, 4:58 p.m. UTC 
From: Derrick Stolee <stolee@gmail.com>

During the latest v2.45.0 update, 'scalar reconfigure --all' started to
segfault on my machine. Breaking it down via the debugger, it was
faulting on a NULL reference to the_hash_algo, which is a macro pointing
to the_repository->hash_algo.

This NULL reference appears to be due to the way the loop is abusing the
the_repository pointer, pointing it to a local repository struct after
discovering that the current directory is a valid Git repository. This
repo-swapping bit was in the original implementation from 4582676075
(scalar: teach 'reconfigure' to optionally handle all registered
enlistments, 2021-12-03), but only recently started segfaulting while
trying to parse the HEAD reference. This also only happens on the
_second_ repository in the list, so does not reproduce if there is only
one registered repo.

My first inclination was to try to refactor cmd_reconfigure() to execute
'git for-each-repo' instead of this loop. In addition to the difficulty
of executing 'scalar reconfigure' within 'git for-each-repo', it would
be difficult to perform the clean-up logic for non-existent repos if we
relied on that child process.

Instead, I chose to move the temporary repo to be within the loop and
reinstate the_repository to its old value after we are done performing
logic on the current array item.

Add a test to t9210-scalar.sh to test 'scalar reconfigure --all' with
multiple registered repos, as a precaution. Unfortunately, I was unable
to reproduce the segfault using this test, so there is some coverage
left to be desired. What exactly causes my setup to hit this bug but not
this test structure? Unclear.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
    scalar: avoid segfault in reconfigure --all
    
    I noticed this while validating the v2.45.0 release (specifically the
    microsoft/git fork, but this applies to the core project).
    
    I couldn't figure out if or why this changed during this cycle. There
    were some changes in setup.c (30b7c4bdca (setup: notice more types of
    implicit bare repositories, 2024-03-09) and 45bb916248 (setup: allow
    cwd=.git w/ bareRepository=explicit, 2024-01-20)) but they seemed
    innocuous and were not in the stack trace of the segfault.
    
    After asking around, it seems like this kind of issue has been happening
    in older versions, but users were ignoring the error during the
    installer or was hidden by a silent installer.
    
    -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1724%2Fderrickstolee%2Fscalar-reconfigure-repo-handle-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1724/derrickstolee/scalar-reconfigure-repo-handle-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1724

 scalar.c          | 10 +++++++---
 t/t9210-scalar.sh | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)


base-commit: e326e520101dcf43a0499c3adc2df7eca30add2d

Comments

Patrick Steinhardt May 2, 2024, 6:46 a.m. UTC | #1 
On Tue, Apr 30, 2024 at 04:58:52PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
> 
> During the latest v2.45.0 update, 'scalar reconfigure --all' started to
> segfault on my machine. Breaking it down via the debugger, it was
> faulting on a NULL reference to the_hash_algo, which is a macro pointing
> to the_repository->hash_algo.
> 
> This NULL reference appears to be due to the way the loop is abusing the
> the_repository pointer, pointing it to a local repository struct after
> discovering that the current directory is a valid Git repository. This
> repo-swapping bit was in the original implementation from 4582676075
> (scalar: teach 'reconfigure' to optionally handle all registered
> enlistments, 2021-12-03), but only recently started segfaulting while
> trying to parse the HEAD reference. This also only happens on the
> _second_ repository in the list, so does not reproduce if there is only
> one registered repo.

Interesting. This also has some overlap with my patch series that aims
to drop the default-SHA1 fallback that we have in place for
`the_repository` [1].

> Add a test to t9210-scalar.sh to test 'scalar reconfigure --all' with
> multiple registered repos, as a precaution. Unfortunately, I was unable
> to reproduce the segfault using this test, so there is some coverage
> left to be desired. What exactly causes my setup to hit this bug but not
> this test structure? Unclear.

One way to trigger _a_ BUG is to use an "includeIf.onbranch:foobar.path"
config. This will cause Git to try and look up the "HEAD" reference, but
because we have a partially-configured repository, only, that will crash
with:

    BUG: refs.c:2123: reference backend is unknown

Whether that bug is the one that you have seen I cannot tell. It at
least does not sound like it.

    test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '
        repos="two three four" &&
        for num in $repos
        do
            git init $num/src &&
            scalar register $num/src &&
            git -C $num/src config includeif."onbranch:foo".path something &&
            git -C $num/src config core.preloadIndex false || return 1
        done &&

        scalar reconfigure --all &&

        for num in $repos
        do
            test true = "$(git -C $num/src config core.preloadIndex)" || return 1
        done
    '

Another issue, which is likely the one you report here, is if you have a
repository with detached "HEAD". In that case we use `get_oid_hex()` to
parse "HEAD", which will implicitly use `the_hash_algo`. But because you
unset it in the second round this will fail with a segfault when you try
to access it:

    ./test-lib.sh: line 1069: 583995 Segmentation fault      (core dumped) scalar reconfigure --all

This is the following testcase:

    test_expect_success 'scalar reconfigure --all with detached HEADs' '
        repos="two three four" &&
        for num in $repos
        do
            rm -rf $num/src &&
            git init $num/src &&
            scalar register $num/src &&
            git -C $num/src config core.preloadIndex false &&
            test_commit -C $num/src initial &&
            git -C $num/src switch --detach HEAD || return 1
        done &&

        scalar reconfigure --all &&

        for num in $repos
        do
            test true = "$(git -C $num/src config core.preloadIndex)" || return 1
        done
    '

This issue should be fixed by my patch series in [1] because we start to
use `get_oid_hex_any()` to parse detached HEADs.

Anyway, your fix is indeed effective because with `repo_init()` we now
properly configure the repository.

[1]: https://lore.kernel.org/git/cover.1714371422.git.ps@pks.im/

Patrick
Derrick Stolee May 4, 2024, 1:57 p.m. UTC | #2 
On 5/2/24 2:46 AM, Patrick Steinhardt wrote:
> On Tue, Apr 30, 2024 at 04:58:52PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>
>>
>> During the latest v2.45.0 update, 'scalar reconfigure --all' started to
>> segfault on my machine. Breaking it down via the debugger, it was
>> faulting on a NULL reference to the_hash_algo, which is a macro pointing
>> to the_repository->hash_algo.
>>
>> This NULL reference appears to be due to the way the loop is abusing the
>> the_repository pointer, pointing it to a local repository struct after
>> discovering that the current directory is a valid Git repository. This
>> repo-swapping bit was in the original implementation from 4582676075
>> (scalar: teach 'reconfigure' to optionally handle all registered
>> enlistments, 2021-12-03), but only recently started segfaulting while
>> trying to parse the HEAD reference. This also only happens on the
>> _second_ repository in the list, so does not reproduce if there is only
>> one registered repo.
> Interesting. This also has some overlap with my patch series that aims
> to drop the default-SHA1 fallback that we have in place for
> `the_repository` [1].

Thanks for this pointer. It indeed will help.

>> Add a test to t9210-scalar.sh to test 'scalar reconfigure --all' with
>> multiple registered repos, as a precaution. Unfortunately, I was unable
>> to reproduce the segfault using this test, so there is some coverage
>> left to be desired. What exactly causes my setup to hit this bug but not
>> this test structure? Unclear.
> One way to trigger _a_ BUG is to use an "includeIf.onbranch:foobar.path"
> config. This will cause Git to try and look up the "HEAD" reference, but
> because we have a partially-configured repository, only, that will crash
> with:
>
>      BUG: refs.c:2123: reference backend is unknown

This is a good extra find. After your explanation for the second

test, I'm confident that I was hitting the detached HEAD case on

my machine.


I will shamelessly steal your tests in my v2.

> This issue should be fixed by my patch series in [1] because we start to
> use `get_oid_hex_any()` to parse detached HEADs.
>
> Anyway, your fix is indeed effective because with `repo_init()` we now
> properly configure the repository.

I appreciate that your series will fix this in a ref-focused way. I think

this change could prevent other bad interactions with the_repository in

the future.


Thanks,

-Stolee
diff mbox series

Patch

diff --git a/scalar.c b/scalar.c
index fb2940c2a00..7234049a1b8 100644
--- a/scalar.c
+++ b/scalar.c
@@ -645,7 +645,6 @@  static int cmd_reconfigure(int argc, const char **argv)
 	};
 	struct string_list scalar_repos = STRING_LIST_INIT_DUP;
 	int i, res = 0;
-	struct repository r = { NULL };
 	struct strbuf commondir = STRBUF_INIT, gitdir = STRBUF_INIT;
 
 	argc = parse_options(argc, argv, NULL, options,
@@ -665,6 +664,7 @@  static int cmd_reconfigure(int argc, const char **argv)
 
 	for (i = 0; i < scalar_repos.nr; i++) {
 		int succeeded = 0;
+		struct repository *old_repo, r = { NULL };
 		const char *dir = scalar_repos.items[i].string;
 
 		strbuf_reset(&commondir);
@@ -712,13 +712,17 @@  static int cmd_reconfigure(int argc, const char **argv)
 
 		git_config_clear();
 
+		if (repo_init(&r, gitdir.buf, commondir.buf))
+			goto loop_end;
+
+		old_repo = the_repository;
 		the_repository = &r;
-		r.commondir = commondir.buf;
-		r.gitdir = gitdir.buf;
 
 		if (set_recommended_config(1) >= 0)
 			succeeded = 1;
 
+		the_repository = old_repo;
+
 loop_end:
 		if (!succeeded) {
 			res = -1;
diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
index 428339e3427..a696337b055 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -180,6 +180,23 @@  test_expect_success 'scalar reconfigure' '
 	test true = "$(git -C one/src config core.preloadIndex)"
 '
 
+test_expect_success 'scalar reconfigure --all' '
+	repos="two three four" &&
+	for num in $repos
+	do
+		git init $num/src &&
+		scalar register $num/src &&
+		git -C $num/src config core.preloadIndex false || return 1
+	done &&
+
+	scalar reconfigure --all &&
+
+	for num in $repos
+	do
+		test true = "$(git -C $num/src config core.preloadIndex)" || return 1
+	done
+'
+
 test_expect_success '`reconfigure -a` removes stale config entries' '
 	git init stale/src &&
 	scalar register stale &&