@matheusbsbs / matheus-bagliano / commits / b64b0df9da

scalar: avoid segfault in reconfigure --all

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. In my case, this is due to one of my repositories having a detached HEAD, which requires get_oid_hex() to parse that the HEAD reference is valid. Another way to cause a failure is to use the "includeIf.onbranch" config key, which will lead to a BUG() statement. 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Derrick Stolee committed May 8, 2024 at 00:05 UTC b64b0df9dac14d8a9b498a6ecf899046eddf53ab

2 files changed +45 -3

scalar.c

+7 -3

@@ -645,7 +645,6 @@ static int cmd_reconfigure(int argc, const char **argv)

645 };

646 struct string_list scalar_repos = STRING_LIST_INIT_DUP;

647 int i, res = 0;

648 - struct repository r = { NULL };

648 struct strbuf commondir = STRBUF_INIT, gitdir = STRBUF_INIT;

649

650 argc = parse_options(argc, argv, NULL, options,

@@ -665,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)

664

665 for (i = 0; i < scalar_repos.nr; i++) {

666 int succeeded = 0;

667 + struct repository *old_repo, r = { NULL };

668 const char *dir = scalar_repos.items[i].string;

669

670 strbuf_reset(&commondir);

@@ -712,13 +712,17 @@ static int cmd_reconfigure(int argc, const char **argv)

712

713 git_config_clear();

714

715 + if (repo_init(&r, gitdir.buf, commondir.buf))

716 + goto loop_end;

717 +

718 + old_repo = the_repository;

719 the_repository = &r;

716 - r.commondir = commondir.buf;

717 - r.gitdir = gitdir.buf;

720

721 if (set_recommended_config(1) >= 0)

722 succeeded = 1;

723

724 + the_repository = old_repo;

725 +

726 loop_end:

727 if (!succeeded) {

728 res = -1;

t/t9210-scalar.sh

+38

@@ -180,6 +180,44 @@ test_expect_success 'scalar reconfigure' '

180 test true = "$(git -C one/src config core.preloadIndex)"

181 '

182

183 +test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '

184 + repos="two three four" &&

185 + for num in $repos

186 + do

187 + git init $num/src &&

188 + scalar register $num/src &&

189 + git -C $num/src config includeif."onbranch:foo".path something &&

190 + git -C $num/src config core.preloadIndex false || return 1

191 + done &&

192 +

193 + scalar reconfigure --all &&

194 +

195 + for num in $repos

196 + do

197 + test true = "$(git -C $num/src config core.preloadIndex)" || return 1

198 + done

199 +'

200 +

201 + test_expect_success 'scalar reconfigure --all with detached HEADs' '

202 + repos="two three four" &&

203 + for num in $repos

204 + do

205 + rm -rf $num/src &&

206 + git init $num/src &&

207 + scalar register $num/src &&

208 + git -C $num/src config core.preloadIndex false &&

209 + test_commit -C $num/src initial &&

210 + git -C $num/src switch --detach HEAD || return 1

211 + done &&

212 +

213 + scalar reconfigure --all &&

214 +

215 + for num in $repos

216 + do

217 + test true = "$(git -C $num/src config core.preloadIndex)" || return 1

218 + done

219 +'

220 +

221 test_expect_success '`reconfigure -a` removes stale config entries' '

222 git init stale/src &&

223 scalar register stale &&