From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Patrick Steinhardt <ps@pks.im>, Derrick Stolee <stolee@gmail.com>,
Derrick Stolee <stolee@gmail.com>
Subject: [PATCH v2] scalar: avoid segfault in reconfigure --all
Date: Sun, 05 May 2024 01:58:18 +0000 [thread overview]
Message-ID: <pull.1724.v2.git.1714874298859.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1724.git.1714496333004.gitgitgadget@gmail.com>
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 tests to t9210-scalar.sh to test 'scalar reconfigure --all' with
multiple registered repos. There are two different ways that the old
use of the_repository could trigger bugs. These issues are being solved
independently to be more careful about the_repository being
uninitialized, but the change in this patch around the use of
the_repository is still a good safety precaution.
Co-authored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
scalar: avoid segfault in reconfigure --all
Update 'scalar reconfigure --all' to be more careful with the_repository
pointer, avoiding sudden halts in some scenarios.
------------------------------------------------------------------------
I noticed this while validating the v2.45.0 release (specifically the
microsoft/git fork, but this applies to the core project).
Thanks, Patrick, for digging in and finding the critical reasons why
this issue can happen. I've included Patrick's test cases and given him
co-authorship. I forged his sign-off, so could you please ACK that
sign-off, Patrick?
-Stolee
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1724%2Fderrickstolee%2Fscalar-reconfigure-repo-handle-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1724/derrickstolee/scalar-reconfigure-repo-handle-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1724
Range-diff vs v1:
1: 6f0f5fa8f00 ! 1: 5be703b6c70 scalar: avoid segfault in reconfigure --all
@@ Commit message
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.
+ Add tests to t9210-scalar.sh to test 'scalar reconfigure --all' with
+ multiple registered repos. There are two different ways that the old
+ use of the_repository could trigger bugs. These issues are being solved
+ independently to be more careful about the_repository being
+ uninitialized, but the change in this patch around the use of
+ the_repository is still a good safety precaution.
+ Co-authored-by: Patrick Steinhardt <ps@pks.im>
+ Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
## scalar.c ##
@@ t/t9210-scalar.sh: test_expect_success 'scalar reconfigure' '
test true = "$(git -C one/src config core.preloadIndex)"
'
-+test_expect_success 'scalar reconfigure --all' '
++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 &&
+
@@ t/t9210-scalar.sh: test_expect_success 'scalar reconfigure' '
+ test true = "$(git -C $num/src config core.preloadIndex)" || return 1
+ done
+'
++
++ 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
++'
+
test_expect_success '`reconfigure -a` removes stale config entries' '
git init stale/src &&
scalar.c | 10 +++++++---
t/t9210-scalar.sh | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+), 3 deletions(-)
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..a41b4fcc085 100755
--- a/t/t9210-scalar.sh
+++ b/t/t9210-scalar.sh
@@ -180,6 +180,44 @@ test_expect_success 'scalar reconfigure' '
test true = "$(git -C one/src config core.preloadIndex)"
'
+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
+'
+
+ 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
+'
+
test_expect_success '`reconfigure -a` removes stale config entries' '
git init stale/src &&
scalar register stale &&
base-commit: e326e520101dcf43a0499c3adc2df7eca30add2d
--
gitgitgadget
next prev parent reply other threads:[~2024-05-05 1:58 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 16:58 [PATCH] scalar: avoid segfault in reconfigure --all Derrick Stolee via GitGitGadget
2024-05-02 6:46 ` Patrick Steinhardt
2024-05-04 13:57 ` Derrick Stolee
2024-05-05 1:58 ` Derrick Stolee via GitGitGadget [this message]
2024-05-06 5:45 ` [PATCH v2] " Patrick Steinhardt
2024-05-08 0:05 ` [PATCH v3] " Derrick Stolee via GitGitGadget
2024-05-08 3:42 ` Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.1724.v2.git.1714874298859.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=ps@pks.im \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).