{"thread":{"id":"61399","subject":"[PATCH] scalar: avoid segfault in reconfigure --all","startedAt":"2024-04-30T16:58:56Z","lastAt":"2024-05-08T03:42:29Z","messageCount":7,"participants":["Derrick Stolee via GitGitGadget","Patrick Steinhardt","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"493797","messageId":"pull.1724.git.1714496333004.gitgitgadget@gmail.com","threadId":"61399","inReplyTo":null,"subject":"[PATCH] scalar: avoid segfault in reconfigure --all","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-04-30T16:58:52Z","receivedAt":"2024-04-30T16:58:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nDuring the latest v2.45.0 update, 'scalar reconfigure --all' started to\nsegfault on my machine. Breaking it down via the debugger, it was\nfaulting on a NULL reference to the_hash_algo, which is a macro pointing\nto the_repository->hash_algo.\n\nThis NULL reference appears to be due to the way the loop is abusing the\nthe_repository pointer, pointing it to a local repository struct after\ndiscovering that the current directory is a valid Git repository. This\nrepo-swapping bit was in the original implementation from 4582676075\n(scalar: teach 'reconfigure' to optionally handle all registered\nenlistments, 2021-12-03), but only recently started segfaulting while\ntrying to parse the HEAD reference. This also only happens on the\n_second_ repository in the list, so does not reproduce if there is only\none registered repo.\n\nMy first inclination was to try to refactor cmd_reconfigure() to execute\n'git for-each-repo' instead of this loop. In addition to the difficulty\nof executing 'scalar reconfigure' within 'git for-each-repo', it would\nbe difficult to perform the clean-up logic for non-existent repos if we\nrelied on that child process.\n\nInstead, I chose to move the temporary repo to be within the loop and\nreinstate the_repository to its old value after we are done performing\nlogic on the current array item.\n\nAdd a test to t9210-scalar.sh to test 'scalar reconfigure --all' with\nmultiple registered repos, as a precaution. Unfortunately, I was unable\nto reproduce the segfault using this test, so there is some coverage\nleft to be desired. What exactly causes my setup to hit this bug but not\nthis test structure? Unclear.\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n    scalar: avoid segfault in reconfigure --all\n    \n    I noticed this while validating the v2.45.0 release (specifically the\n    microsoft/git fork, but this applies to the core project).\n    \n    I couldn't figure out if or why this changed during this cycle. There\n    were some changes in setup.c (30b7c4bdca (setup: notice more types of\n    implicit bare repositories, 2024-03-09) and 45bb916248 (setup: allow\n    cwd=.git w/ bareRepository=explicit, 2024-01-20)) but they seemed\n    innocuous and were not in the stack trace of the segfault.\n    \n    After asking around, it seems like this kind of issue has been happening\n    in older versions, but users were ignoring the error during the\n    installer or was hidden by a silent installer.\n    \n    -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1724%2Fderrickstolee%2Fscalar-reconfigure-repo-handle-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1724/derrickstolee/scalar-reconfigure-repo-handle-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1724\n\n scalar.c          | 10 +++++++---\n t/t9210-scalar.sh | 17 +++++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex fb2940c2a00..7234049a1b8 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -645,7 +645,6 @@ static int cmd_reconfigure(int argc, const char **argv)\n \t};\n \tstruct string_list scalar_repos = STRING_LIST_INIT_DUP;\n \tint i, res = 0;\n-\tstruct repository r = { NULL };\n \tstruct strbuf commondir = STRBUF_INIT, gitdir = STRBUF_INIT;\n \n \targc = parse_options(argc, argv, NULL, options,\n@@ -665,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \tfor (i = 0; i < scalar_repos.nr; i++) {\n \t\tint succeeded = 0;\n+\t\tstruct repository *old_repo, r = { NULL };\n \t\tconst char *dir = scalar_repos.items[i].string;\n \n \t\tstrbuf_reset(&commondir);\n@@ -712,13 +712,17 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \t\tgit_config_clear();\n \n+\t\tif (repo_init(&r, gitdir.buf, commondir.buf))\n+\t\t\tgoto loop_end;\n+\n+\t\told_repo = the_repository;\n \t\tthe_repository = &r;\n-\t\tr.commondir = commondir.buf;\n-\t\tr.gitdir = gitdir.buf;\n \n \t\tif (set_recommended_config(1) >= 0)\n \t\t\tsucceeded = 1;\n \n+\t\tthe_repository = old_repo;\n+\n loop_end:\n \t\tif (!succeeded) {\n \t\t\tres = -1;\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex 428339e3427..a696337b055 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -180,6 +180,23 @@ test_expect_success 'scalar reconfigure' '\n \ttest true = \"$(git -C one/src config core.preloadIndex)\"\n '\n \n+test_expect_success 'scalar reconfigure --all' '\n+\trepos=\"two three four\" &&\n+\tfor num in $repos\n+\tdo\n+\t\tgit init $num/src &&\n+\t\tscalar register $num/src &&\n+\t\tgit -C $num/src config core.preloadIndex false || return 1\n+\tdone &&\n+\n+\tscalar reconfigure --all &&\n+\n+\tfor num in $repos\n+\tdo\n+\t\ttest true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n+\tdone\n+'\n+\n test_expect_success '`reconfigure -a` removes stale config entries' '\n \tgit init stale/src &&\n \tscalar register stale &&\n\nbase-commit: e326e520101dcf43a0499c3adc2df7eca30add2d\n-- \ngitgitgadget\n"},{"id":"493876","messageId":"ZjM23X_Tf3pcWsIq@tanuki","threadId":"61399","inReplyTo":"pull.1724.git.1714496333004.gitgitgadget@gmail.com","subject":"Re: [PATCH] scalar: avoid segfault in reconfigure --all","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-02T06:46:53Z","receivedAt":"2024-05-02T06:46:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Apr 30, 2024 at 04:58:52PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <stolee@gmail.com>\n> \n> During the latest v2.45.0 update, 'scalar reconfigure --all' started to\n> segfault on my machine. Breaking it down via the debugger, it was\n> faulting on a NULL reference to the_hash_algo, which is a macro pointing\n> to the_repository->hash_algo.\n> \n> This NULL reference appears to be due to the way the loop is abusing the\n> the_repository pointer, pointing it to a local repository struct after\n> discovering that the current directory is a valid Git repository. This\n> repo-swapping bit was in the original implementation from 4582676075\n> (scalar: teach 'reconfigure' to optionally handle all registered\n> enlistments, 2021-12-03), but only recently started segfaulting while\n> trying to parse the HEAD reference. This also only happens on the\n> _second_ repository in the list, so does not reproduce if there is only\n> one registered repo.\n\nInteresting. This also has some overlap with my patch series that aims\nto drop the default-SHA1 fallback that we have in place for\n`the_repository` [1].\n\n> Add a test to t9210-scalar.sh to test 'scalar reconfigure --all' with\n> multiple registered repos, as a precaution. Unfortunately, I was unable\n> to reproduce the segfault using this test, so there is some coverage\n> left to be desired. What exactly causes my setup to hit this bug but not\n> this test structure? Unclear.\n\nOne way to trigger _a_ BUG is to use an \"includeIf.onbranch:foobar.path\"\nconfig. This will cause Git to try and look up the \"HEAD\" reference, but\nbecause we have a partially-configured repository, only, that will crash\nwith:\n\n    BUG: refs.c:2123: reference backend is unknown\n\nWhether that bug is the one that you have seen I cannot tell. It at\nleast does not sound like it.\n\n    test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '\n        repos=\"two three four\" &&\n        for num in $repos\n        do\n            git init $num/src &&\n            scalar register $num/src &&\n            git -C $num/src config includeif.\"onbranch:foo\".path something &&\n            git -C $num/src config core.preloadIndex false || return 1\n        done &&\n\n        scalar reconfigure --all &&\n\n        for num in $repos\n        do\n            test true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n        done\n    '\n\nAnother issue, which is likely the one you report here, is if you have a\nrepository with detached \"HEAD\". In that case we use `get_oid_hex()` to\nparse \"HEAD\", which will implicitly use `the_hash_algo`. But because you\nunset it in the second round this will fail with a segfault when you try\nto access it:\n\n    ./test-lib.sh: line 1069: 583995 Segmentation fault      (core dumped) scalar reconfigure --all\n\nThis is the following testcase:\n\n    test_expect_success 'scalar reconfigure --all with detached HEADs' '\n        repos=\"two three four\" &&\n        for num in $repos\n        do\n            rm -rf $num/src &&\n            git init $num/src &&\n            scalar register $num/src &&\n            git -C $num/src config core.preloadIndex false &&\n            test_commit -C $num/src initial &&\n            git -C $num/src switch --detach HEAD || return 1\n        done &&\n\n        scalar reconfigure --all &&\n\n        for num in $repos\n        do\n            test true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n        done\n    '\n\nThis issue should be fixed by my patch series in [1] because we start to\nuse `get_oid_hex_any()` to parse detached HEADs.\n\nAnyway, your fix is indeed effective because with `repo_init()` we now\nproperly configure the repository.\n\n[1]: https://lore.kernel.org/git/cover.1714371422.git.ps@pks.im/\n\nPatrick\n"},{"id":"494078","messageId":"0e043864-a5ce-4e21-97eb-15a0bf614fc8@gmail.com","threadId":"61399","inReplyTo":"ZjM23X_Tf3pcWsIq@tanuki","subject":"Re: [PATCH] scalar: avoid segfault in reconfigure --all","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-05-04T13:57:44Z","receivedAt":"2024-05-04T13:57:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/2/24 2:46 AM, Patrick Steinhardt wrote:\n> On Tue, Apr 30, 2024 at 04:58:52PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee <stolee@gmail.com>\n>>\n>> During the latest v2.45.0 update, 'scalar reconfigure --all' started to\n>> segfault on my machine. Breaking it down via the debugger, it was\n>> faulting on a NULL reference to the_hash_algo, which is a macro pointing\n>> to the_repository->hash_algo.\n>>\n>> This NULL reference appears to be due to the way the loop is abusing the\n>> the_repository pointer, pointing it to a local repository struct after\n>> discovering that the current directory is a valid Git repository. This\n>> repo-swapping bit was in the original implementation from 4582676075\n>> (scalar: teach 'reconfigure' to optionally handle all registered\n>> enlistments, 2021-12-03), but only recently started segfaulting while\n>> trying to parse the HEAD reference. This also only happens on the\n>> _second_ repository in the list, so does not reproduce if there is only\n>> one registered repo.\n> Interesting. This also has some overlap with my patch series that aims\n> to drop the default-SHA1 fallback that we have in place for\n> `the_repository` [1].\n\nThanks for this pointer. It indeed will help.\n\n>> Add a test to t9210-scalar.sh to test 'scalar reconfigure --all' with\n>> multiple registered repos, as a precaution. Unfortunately, I was unable\n>> to reproduce the segfault using this test, so there is some coverage\n>> left to be desired. What exactly causes my setup to hit this bug but not\n>> this test structure? Unclear.\n> One way to trigger _a_ BUG is to use an \"includeIf.onbranch:foobar.path\"\n> config. This will cause Git to try and look up the \"HEAD\" reference, but\n> because we have a partially-configured repository, only, that will crash\n> with:\n>\n>      BUG: refs.c:2123: reference backend is unknown\n\nThis is a good extra find. After your explanation for the second\n\ntest, I'm confident that I was hitting the detached HEAD case on\n\nmy machine.\n\n\nI will shamelessly steal your tests in my v2.\n\n> This issue should be fixed by my patch series in [1] because we start to\n> use `get_oid_hex_any()` to parse detached HEADs.\n>\n> Anyway, your fix is indeed effective because with `repo_init()` we now\n> properly configure the repository.\n\nI appreciate that your series will fix this in a ref-focused way. I think\n\nthis change could prevent other bad interactions with the_repository in\n\nthe future.\n\n\nThanks,\n\n-Stolee\n\n\n"},{"id":"494084","messageId":"pull.1724.v2.git.1714874298859.gitgitgadget@gmail.com","threadId":"61399","inReplyTo":"pull.1724.git.1714496333004.gitgitgadget@gmail.com","subject":"[PATCH v2] scalar: avoid segfault in reconfigure --all","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-05T01:58:18Z","receivedAt":"2024-05-05T01:58:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nDuring the latest v2.45.0 update, 'scalar reconfigure --all' started to\nsegfault on my machine. Breaking it down via the debugger, it was\nfaulting on a NULL reference to the_hash_algo, which is a macro pointing\nto the_repository->hash_algo.\n\nThis NULL reference appears to be due to the way the loop is abusing the\nthe_repository pointer, pointing it to a local repository struct after\ndiscovering that the current directory is a valid Git repository. This\nrepo-swapping bit was in the original implementation from 4582676075\n(scalar: teach 'reconfigure' to optionally handle all registered\nenlistments, 2021-12-03), but only recently started segfaulting while\ntrying to parse the HEAD reference. This also only happens on the\n_second_ repository in the list, so does not reproduce if there is only\none registered repo.\n\nMy first inclination was to try to refactor cmd_reconfigure() to execute\n'git for-each-repo' instead of this loop. In addition to the difficulty\nof executing 'scalar reconfigure' within 'git for-each-repo', it would\nbe difficult to perform the clean-up logic for non-existent repos if we\nrelied on that child process.\n\nInstead, I chose to move the temporary repo to be within the loop and\nreinstate the_repository to its old value after we are done performing\nlogic on the current array item.\n\nAdd tests to t9210-scalar.sh to test 'scalar reconfigure --all' with\nmultiple registered repos. There are two different ways that the old\nuse of the_repository could trigger bugs. These issues are being solved\nindependently to be more careful about the_repository being\nuninitialized, but the change in this patch around the use of\nthe_repository is still a good safety precaution.\n\nCo-authored-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n    scalar: avoid segfault in reconfigure --all\n    \n    Update 'scalar reconfigure --all' to be more careful with the_repository\n    pointer, avoiding sudden halts in some scenarios.\n    \n    ------------------------------------------------------------------------\n    \n    I noticed this while validating the v2.45.0 release (specifically the\n    microsoft/git fork, but this applies to the core project).\n    \n    Thanks, Patrick, for digging in and finding the critical reasons why\n    this issue can happen. I've included Patrick's test cases and given him\n    co-authorship. I forged his sign-off, so could you please ACK that\n    sign-off, Patrick?\n    \n    -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1724%2Fderrickstolee%2Fscalar-reconfigure-repo-handle-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1724/derrickstolee/scalar-reconfigure-repo-handle-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1724\n\nRange-diff vs v1:\n\n 1:  6f0f5fa8f00 ! 1:  5be703b6c70 scalar: avoid segfault in reconfigure --all\n     @@ Commit message\n          reinstate the_repository to its old value after we are done performing\n          logic on the current array item.\n      \n     -    Add a test to t9210-scalar.sh to test 'scalar reconfigure --all' with\n     -    multiple registered repos, as a precaution. Unfortunately, I was unable\n     -    to reproduce the segfault using this test, so there is some coverage\n     -    left to be desired. What exactly causes my setup to hit this bug but not\n     -    this test structure? Unclear.\n     +    Add tests to t9210-scalar.sh to test 'scalar reconfigure --all' with\n     +    multiple registered repos. There are two different ways that the old\n     +    use of the_repository could trigger bugs. These issues are being solved\n     +    independently to be more careful about the_repository being\n     +    uninitialized, but the change in this patch around the use of\n     +    the_repository is still a good safety precaution.\n      \n     +    Co-authored-by: Patrick Steinhardt <ps@pks.im>\n     +    Signed-off-by: Patrick Steinhardt <ps@pks.im>\n          Signed-off-by: Derrick Stolee <stolee@gmail.com>\n      \n       ## scalar.c ##\n     @@ t/t9210-scalar.sh: test_expect_success 'scalar reconfigure' '\n       \ttest true = \"$(git -C one/src config core.preloadIndex)\"\n       '\n       \n     -+test_expect_success 'scalar reconfigure --all' '\n     ++test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '\n      +\trepos=\"two three four\" &&\n      +\tfor num in $repos\n      +\tdo\n      +\t\tgit init $num/src &&\n      +\t\tscalar register $num/src &&\n     ++\t\tgit -C $num/src config includeif.\"onbranch:foo\".path something &&\n      +\t\tgit -C $num/src config core.preloadIndex false || return 1\n      +\tdone &&\n      +\n     @@ t/t9210-scalar.sh: test_expect_success 'scalar reconfigure' '\n      +\t\ttest true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n      +\tdone\n      +'\n     ++\n     ++ test_expect_success 'scalar reconfigure --all with detached HEADs' '\n     ++\trepos=\"two three four\" &&\n     ++\tfor num in $repos\n     ++\tdo\n     ++\t\trm -rf $num/src &&\n     ++\t\tgit init $num/src &&\n     ++\t\tscalar register $num/src &&\n     ++\t\tgit -C $num/src config core.preloadIndex false &&\n     ++\t\ttest_commit -C $num/src initial &&\n     ++\t\tgit -C $num/src switch --detach HEAD || return 1\n     ++\tdone &&\n     ++\n     ++\tscalar reconfigure --all &&\n     ++\n     ++\tfor num in $repos\n     ++\tdo\n     ++\t\ttest true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n     ++\tdone\n     ++'\n      +\n       test_expect_success '`reconfigure -a` removes stale config entries' '\n       \tgit init stale/src &&\n\n\n scalar.c          | 10 +++++++---\n t/t9210-scalar.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+), 3 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex fb2940c2a00..7234049a1b8 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -645,7 +645,6 @@ static int cmd_reconfigure(int argc, const char **argv)\n \t};\n \tstruct string_list scalar_repos = STRING_LIST_INIT_DUP;\n \tint i, res = 0;\n-\tstruct repository r = { NULL };\n \tstruct strbuf commondir = STRBUF_INIT, gitdir = STRBUF_INIT;\n \n \targc = parse_options(argc, argv, NULL, options,\n@@ -665,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \tfor (i = 0; i < scalar_repos.nr; i++) {\n \t\tint succeeded = 0;\n+\t\tstruct repository *old_repo, r = { NULL };\n \t\tconst char *dir = scalar_repos.items[i].string;\n \n \t\tstrbuf_reset(&commondir);\n@@ -712,13 +712,17 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \t\tgit_config_clear();\n \n+\t\tif (repo_init(&r, gitdir.buf, commondir.buf))\n+\t\t\tgoto loop_end;\n+\n+\t\told_repo = the_repository;\n \t\tthe_repository = &r;\n-\t\tr.commondir = commondir.buf;\n-\t\tr.gitdir = gitdir.buf;\n \n \t\tif (set_recommended_config(1) >= 0)\n \t\t\tsucceeded = 1;\n \n+\t\tthe_repository = old_repo;\n+\n loop_end:\n \t\tif (!succeeded) {\n \t\t\tres = -1;\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex 428339e3427..a41b4fcc085 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -180,6 +180,44 @@ test_expect_success 'scalar reconfigure' '\n \ttest true = \"$(git -C one/src config core.preloadIndex)\"\n '\n \n+test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '\n+\trepos=\"two three four\" &&\n+\tfor num in $repos\n+\tdo\n+\t\tgit init $num/src &&\n+\t\tscalar register $num/src &&\n+\t\tgit -C $num/src config includeif.\"onbranch:foo\".path something &&\n+\t\tgit -C $num/src config core.preloadIndex false || return 1\n+\tdone &&\n+\n+\tscalar reconfigure --all &&\n+\n+\tfor num in $repos\n+\tdo\n+\t\ttest true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n+\tdone\n+'\n+\n+ test_expect_success 'scalar reconfigure --all with detached HEADs' '\n+\trepos=\"two three four\" &&\n+\tfor num in $repos\n+\tdo\n+\t\trm -rf $num/src &&\n+\t\tgit init $num/src &&\n+\t\tscalar register $num/src &&\n+\t\tgit -C $num/src config core.preloadIndex false &&\n+\t\ttest_commit -C $num/src initial &&\n+\t\tgit -C $num/src switch --detach HEAD || return 1\n+\tdone &&\n+\n+\tscalar reconfigure --all &&\n+\n+\tfor num in $repos\n+\tdo\n+\t\ttest true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n+\tdone\n+'\n+\n test_expect_success '`reconfigure -a` removes stale config entries' '\n \tgit init stale/src &&\n \tscalar register stale &&\n\nbase-commit: e326e520101dcf43a0499c3adc2df7eca30add2d\n-- \ngitgitgadget\n"},{"id":"494110","messageId":"Zjhufiq2DmBlVpIx@tanuki","threadId":"61399","inReplyTo":"pull.1724.v2.git.1714874298859.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] scalar: avoid segfault in reconfigure --all","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-06T05:45:34Z","receivedAt":"2024-05-06T05:45:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, May 05, 2024 at 01:58:18AM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <stolee@gmail.com>\n> \n> During the latest v2.45.0 update, 'scalar reconfigure --all' started to\n> segfault on my machine. Breaking it down via the debugger, it was\n> faulting on a NULL reference to the_hash_algo, which is a macro pointing\n> to the_repository->hash_algo.\n> \n> This NULL reference appears to be due to the way the loop is abusing the\n> the_repository pointer, pointing it to a local repository struct after\n> discovering that the current directory is a valid Git repository. This\n> repo-swapping bit was in the original implementation from 4582676075\n> (scalar: teach 'reconfigure' to optionally handle all registered\n> enlistments, 2021-12-03), but only recently started segfaulting while\n> trying to parse the HEAD reference. This also only happens on the\n> _second_ repository in the list, so does not reproduce if there is only\n> one registered repo.\n\nI think this explanation could use an update now that we have figured\nout the root cause likely being a detached HEAD, where `get_oid_hex()`\nwill then access a NULL pointer.\n\n> My first inclination was to try to refactor cmd_reconfigure() to execute\n> 'git for-each-repo' instead of this loop. In addition to the difficulty\n> of executing 'scalar reconfigure' within 'git for-each-repo', it would\n> be difficult to perform the clean-up logic for non-existent repos if we\n> relied on that child process.\n> \n> Instead, I chose to move the temporary repo to be within the loop and\n> reinstate the_repository to its old value after we are done performing\n> logic on the current array item.\n> \n> Add tests to t9210-scalar.sh to test 'scalar reconfigure --all' with\n> multiple registered repos. There are two different ways that the old\n> use of the_repository could trigger bugs. These issues are being solved\n> independently to be more careful about the_repository being\n> uninitialized, but the change in this patch around the use of\n> the_repository is still a good safety precaution.\n> \n> Co-authored-by: Patrick Steinhardt <ps@pks.im>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> Signed-off-by: Derrick Stolee <stolee@gmail.com>\n> ---\n>     scalar: avoid segfault in reconfigure --all\n>     \n>     Update 'scalar reconfigure --all' to be more careful with the_repository\n>     pointer, avoiding sudden halts in some scenarios.\n>     \n>     ------------------------------------------------------------------------\n>     \n>     I noticed this while validating the v2.45.0 release (specifically the\n>     microsoft/git fork, but this applies to the core project).\n>     \n>     Thanks, Patrick, for digging in and finding the critical reasons why\n>     this issue can happen. I've included Patrick's test cases and given him\n>     co-authorship. I forged his sign-off, so could you please ACK that\n>     sign-off, Patrick?\n>     \n>     -Stolee\n\nOther than the above nit regarding the root cause analysis this patch\nlooks good to me, and I'm fine with the forged SOB. Thanks!\n\nPatrick\n"},{"id":"494291","messageId":"pull.1724.v3.git.1715126749391.gitgitgadget@gmail.com","threadId":"61399","inReplyTo":"pull.1724.v2.git.1714874298859.gitgitgadget@gmail.com","subject":"[PATCH v3] scalar: avoid segfault in reconfigure --all","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-05-08T00:05:49Z","receivedAt":"2024-05-08T00:05:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nDuring the latest v2.45.0 update, 'scalar reconfigure --all' started to\nsegfault on my machine. Breaking it down via the debugger, it was\nfaulting on a NULL reference to the_hash_algo, which is a macro pointing\nto the_repository->hash_algo.\n\nIn my case, this is due to one of my repositories having a detached HEAD,\nwhich requires get_oid_hex() to parse that the HEAD reference is valid.\nAnother way to cause a failure is to use the \"includeIf.onbranch\" config\nkey, which will lead to a BUG() statement.\n\nMy first inclination was to try to refactor cmd_reconfigure() to execute\n'git for-each-repo' instead of this loop. In addition to the difficulty\nof executing 'scalar reconfigure' within 'git for-each-repo', it would\nbe difficult to perform the clean-up logic for non-existent repos if we\nrelied on that child process.\n\nInstead, I chose to move the temporary repo to be within the loop and\nreinstate the_repository to its old value after we are done performing\nlogic on the current array item.\n\nAdd tests to t9210-scalar.sh to test 'scalar reconfigure --all' with\nmultiple registered repos. There are two different ways that the old\nuse of the_repository could trigger bugs. These issues are being solved\nindependently to be more careful about the_repository being\nuninitialized, but the change in this patch around the use of\nthe_repository is still a good safety precaution.\n\nCo-authored-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n    scalar: avoid segfault in reconfigure --all\n    \n    Update 'scalar reconfigure --all' to be more careful with the_repository\n    pointer, avoiding sudden halts in some scenarios.\n    \n    ------------------------------------------------------------------------\n    \n    I noticed this while validating the v2.45.0 release (specifically the\n    microsoft/git fork, but this applies to the core project).\n    \n    Thanks, Patrick, for digging in and finding the critical reasons why\n    this issue can happen. Patrick ACKed the sign-off in v2.\n    \n    v3 includes an update to the commit message. Sorry that I missed this\n    paragraph when updating for v2.\n    \n    -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1724%2Fderrickstolee%2Fscalar-reconfigure-repo-handle-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1724/derrickstolee/scalar-reconfigure-repo-handle-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1724\n\nRange-diff vs v2:\n\n 1:  5be703b6c70 ! 1:  00d101986f8 scalar: avoid segfault in reconfigure --all\n     @@ Commit message\n          faulting on a NULL reference to the_hash_algo, which is a macro pointing\n          to the_repository->hash_algo.\n      \n     -    This NULL reference appears to be due to the way the loop is abusing the\n     -    the_repository pointer, pointing it to a local repository struct after\n     -    discovering that the current directory is a valid Git repository. This\n     -    repo-swapping bit was in the original implementation from 4582676075\n     -    (scalar: teach 'reconfigure' to optionally handle all registered\n     -    enlistments, 2021-12-03), but only recently started segfaulting while\n     -    trying to parse the HEAD reference. This also only happens on the\n     -    _second_ repository in the list, so does not reproduce if there is only\n     -    one registered repo.\n     +    In my case, this is due to one of my repositories having a detached HEAD,\n     +    which requires get_oid_hex() to parse that the HEAD reference is valid.\n     +    Another way to cause a failure is to use the \"includeIf.onbranch\" config\n     +    key, which will lead to a BUG() statement.\n      \n          My first inclination was to try to refactor cmd_reconfigure() to execute\n          'git for-each-repo' instead of this loop. In addition to the difficulty\n\n\n scalar.c          | 10 +++++++---\n t/t9210-scalar.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+), 3 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex fb2940c2a00..7234049a1b8 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -645,7 +645,6 @@ static int cmd_reconfigure(int argc, const char **argv)\n \t};\n \tstruct string_list scalar_repos = STRING_LIST_INIT_DUP;\n \tint i, res = 0;\n-\tstruct repository r = { NULL };\n \tstruct strbuf commondir = STRBUF_INIT, gitdir = STRBUF_INIT;\n \n \targc = parse_options(argc, argv, NULL, options,\n@@ -665,6 +664,7 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \tfor (i = 0; i < scalar_repos.nr; i++) {\n \t\tint succeeded = 0;\n+\t\tstruct repository *old_repo, r = { NULL };\n \t\tconst char *dir = scalar_repos.items[i].string;\n \n \t\tstrbuf_reset(&commondir);\n@@ -712,13 +712,17 @@ static int cmd_reconfigure(int argc, const char **argv)\n \n \t\tgit_config_clear();\n \n+\t\tif (repo_init(&r, gitdir.buf, commondir.buf))\n+\t\t\tgoto loop_end;\n+\n+\t\told_repo = the_repository;\n \t\tthe_repository = &r;\n-\t\tr.commondir = commondir.buf;\n-\t\tr.gitdir = gitdir.buf;\n \n \t\tif (set_recommended_config(1) >= 0)\n \t\t\tsucceeded = 1;\n \n+\t\tthe_repository = old_repo;\n+\n loop_end:\n \t\tif (!succeeded) {\n \t\t\tres = -1;\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex 428339e3427..a41b4fcc085 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -180,6 +180,44 @@ test_expect_success 'scalar reconfigure' '\n \ttest true = \"$(git -C one/src config core.preloadIndex)\"\n '\n \n+test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '\n+\trepos=\"two three four\" &&\n+\tfor num in $repos\n+\tdo\n+\t\tgit init $num/src &&\n+\t\tscalar register $num/src &&\n+\t\tgit -C $num/src config includeif.\"onbranch:foo\".path something &&\n+\t\tgit -C $num/src config core.preloadIndex false || return 1\n+\tdone &&\n+\n+\tscalar reconfigure --all &&\n+\n+\tfor num in $repos\n+\tdo\n+\t\ttest true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n+\tdone\n+'\n+\n+ test_expect_success 'scalar reconfigure --all with detached HEADs' '\n+\trepos=\"two three four\" &&\n+\tfor num in $repos\n+\tdo\n+\t\trm -rf $num/src &&\n+\t\tgit init $num/src &&\n+\t\tscalar register $num/src &&\n+\t\tgit -C $num/src config core.preloadIndex false &&\n+\t\ttest_commit -C $num/src initial &&\n+\t\tgit -C $num/src switch --detach HEAD || return 1\n+\tdone &&\n+\n+\tscalar reconfigure --all &&\n+\n+\tfor num in $repos\n+\tdo\n+\t\ttest true = \"$(git -C $num/src config core.preloadIndex)\" || return 1\n+\tdone\n+'\n+\n test_expect_success '`reconfigure -a` removes stale config entries' '\n \tgit init stale/src &&\n \tscalar register stale &&\n\nbase-commit: e326e520101dcf43a0499c3adc2df7eca30add2d\n-- \ngitgitgadget\n"},{"id":"494295","messageId":"Zjr0nmJib_1EtdJU@tanuki","threadId":"61399","inReplyTo":"pull.1724.v3.git.1715126749391.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] scalar: avoid segfault in reconfigure --all","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-08T03:42:22Z","receivedAt":"2024-05-08T03:42:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 08, 2024 at 12:05:49AM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <stolee@gmail.com>\n> \n> During the latest v2.45.0 update, 'scalar reconfigure --all' started to\n> segfault on my machine. Breaking it down via the debugger, it was\n> faulting on a NULL reference to the_hash_algo, which is a macro pointing\n> to the_repository->hash_algo.\n> \n> In my case, this is due to one of my repositories having a detached HEAD,\n> which requires get_oid_hex() to parse that the HEAD reference is valid.\n> Another way to cause a failure is to use the \"includeIf.onbranch\" config\n> key, which will lead to a BUG() statement.\n> \n> My first inclination was to try to refactor cmd_reconfigure() to execute\n> 'git for-each-repo' instead of this loop. In addition to the difficulty\n> of executing 'scalar reconfigure' within 'git for-each-repo', it would\n> be difficult to perform the clean-up logic for non-existent repos if we\n> relied on that child process.\n> \n> Instead, I chose to move the temporary repo to be within the loop and\n> reinstate the_repository to its old value after we are done performing\n> logic on the current array item.\n> \n> Add tests to t9210-scalar.sh to test 'scalar reconfigure --all' with\n> multiple registered repos. There are two different ways that the old\n> use of the_repository could trigger bugs. These issues are being solved\n> independently to be more careful about the_repository being\n> uninitialized, but the change in this patch around the use of\n> the_repository is still a good safety precaution.\n> \n> Co-authored-by: Patrick Steinhardt <ps@pks.im>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> Signed-off-by: Derrick Stolee <stolee@gmail.com>\n\nThanks, this version looks good to me!\n\nPatrick\n"}]}