{"thread":{"id":"61934","subject":"SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository.","startedAt":"2024-08-12T10:57:31Z","lastAt":"2024-08-13T15:11:05Z","messageCount":10,"participants":["ArcticLampyrid","Jeff King","Junio C Hamano","brian m. carlson","Patrick Steinhardt","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"500627","messageId":"TYWP301MB0563973ECA440E7DAF0F7E89C4852@TYWP301MB0563.JPNP301.PROD.OUTLOOK.COM","threadId":"61934","inReplyTo":null,"subject":"SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository.","fromName":"ArcticLampyrid","fromEmail":"arcticlampyrid@outlook.com","sentAt":"2024-08-12T10:57:25Z","receivedAt":"2024-08-12T10:57:31Z","isPatch":false,"sender":{"key":"arcticlampyrid@outlook.com","avatar":null},"body":"#### Steps to Reproduce\nI attempted to unbundle a pack but forgot to execute `git init` beforehand.\n\n#### Expected Behavior\nAn error message should have been displayed, reminding me to run `git init`.\n\n#### Actual Behavior\nThe process unexpectedly terminated with a SIGSEGV (Address boundary error).\n\n#### Difference Between Expected and Actual Behavior\nInstead of gracefully exiting, the process terminated abruptly.\n\n#### System Info\n- Git Version: 2.46.0\n- CPU: x86_64\n- No commit associated with this build\n- Size of `long`: 8 bytes\n- Size of `size_t`: 8 bytes\n- Shell Path: /bin/sh\n- libcurl Version: 8.9.0\n- OpenSSL Version: 3.3.1 (4 Jun 2024)\n- zlib Version: 1.3.1\n- uname: Linux 6.10.3-arch1-2 #1 SMP PREEMPT_DYNAMIC Tue, 06 Aug 2024 07:21:19 \n+0000 x86_64\n- Compiler Info: GNU Compiler Collection (GCC) 14.1\n- libc Version: glibc 2.40\n- Interactive Shell: /usr/bin/fish\n"},{"id":"500628","messageId":"20240812114733.GA3732689@coredump.intra.peff.net","threadId":"61934","inReplyTo":"TYWP301MB0563973ECA440E7DAF0F7E89C4852@TYWP301MB0563.JPNP301.PROD.OUTLOOK.COM","subject":"Re: SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-12T11:47:33Z","receivedAt":"2024-08-12T11:47:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 12, 2024 at 06:57:25PM +0800, ArcticLampyrid wrote:\n\n> #### Steps to Reproduce\n> I attempted to unbundle a pack but forgot to execute `git init` beforehand.\n> \n> #### Expected Behavior\n> An error message should have been displayed, reminding me to run `git init`.\n> \n> #### Actual Behavior\n> The process unexpectedly terminated with a SIGSEGV (Address boundary error).\n> \n> #### Difference Between Expected and Actual Behavior\n> Instead of gracefully exiting, the process terminated abruptly.\n\nThanks for the report, it's pretty easy to reproduce. Looks like another\ncasualty of c8aed5e8da (repository: stop setting SHA1 as the default\nobject hash, 2024-05-07). Author cc'd.\n\nA sample stack trace is:\n\n  #0  0x000055555573a93c in get_hash_hex_algop (\n      hex=0x555555a11180 \"b2f0a7f47f5f2aebe1e7fceff19a57de20a78c06 refs/heads/master\", hash=0x7fffffffdc00 \"$h\\240UUU\",\n      algop=0x0) at hex.c:11\n  #1  0x000055555573a9aa in get_oid_hex_algop (\n      hex=0x555555a11180 \"b2f0a7f47f5f2aebe1e7fceff19a57de20a78c06 refs/heads/master\", oid=0x7fffffffdc00, algop=0x0)\n      at hex.c:29\n  #2  0x000055555573aad1 in parse_oid_hex_algop (\n      hex=0x555555a11180 \"b2f0a7f47f5f2aebe1e7fceff19a57de20a78c06 refs/heads/master\", oid=0x7fffffffdc00,\n      end=0x7fffffffdc28, algop=0x0) at hex.c:62\n  #3  0x00005555556acaf3 in read_bundle_header_fd (fd=3, header=0x7fffffffddf0, report_path=0x555555a110a0 \"foo\")\n      at bundle.c:121\n  #4  0x00005555556accdf in read_bundle_header (path=0x555555a110a0 \"foo\", header=0x7fffffffddf0) at bundle.c:153\n  #5  0x000055555558c8d6 in open_bundle (path=0x555555a110a0 \"foo\", header=0x7fffffffddf0, name=0x0)\n      at builtin/bundle.c:123\n  #6  0x000055555558cd8b in cmd_bundle_unbundle (argc=1, argv=0x7fffffffe4b0, prefix=0x0) at builtin/bundle.c:210\n  #7  0x000055555558cff1 in cmd_bundle (argc=2, argv=0x7fffffffe4b0, prefix=0x0) at builtin/bundle.c:244\n\nCuriously, the next line after the open_bundle() call is:\n\n          if (!startup_info->have_repository)\n                  die(_(\"Need a repository to unbundle.\"));\n\nSo one option is to just do that check earlier. But that leaves other\nsub-commands of \"git bundle\":\n\n  - \"create\" likewise requires a repo, and seems OK. That makes sense\n    since we're not reading anything.\n\n  - \"verify\" requires a repo, which I wouldn't have expected, but I\n    guess it's because we probably unbundle under the hood to walk.\n    Anyway, it gets the ordering right here and checks the repo before\n    opening the bundle.\n\n  - list-heads doesn't require a repo, and segfaults. So it really does\n    need some kind of detection or default to know which hash to use.\n\n-Peff\n"},{"id":"500645","messageId":"xmqq1q2t7n33.fsf@gitster.g","threadId":"61934","inReplyTo":"20240812114733.GA3732689@coredump.intra.peff.net","subject":"Re: SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-12T15:55:12Z","receivedAt":"2024-08-12T15:55:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   - \"verify\" requires a repo, which I wouldn't have expected, but I\n>     guess it's because we probably unbundle under the hood to walk.\n>     Anyway, it gets the ordering right here and checks the repo before\n>     opening the bundle.\n\nIn hindsight \"verify\" is misnamed and overrated.  Its purpose is to\ncheck if the bundle can be unbundled into your _current_ repository\nby checking if you have all the commits _required_ to unbundle the\nbundle.\n\nIn fact, I doubt that \"verify\" looks at the pack stream part of the\nfile at all.\n\n>   - list-heads doesn't require a repo, and segfaults. So it really does\n>     need some kind of detection or default to know which hash to use.\n\nYes.\n"},{"id":"500662","messageId":"Zrp5JnsVHbqs4TyN@tapette.crustytoothpaste.net","threadId":"61934","inReplyTo":"20240812114733.GA3732689@coredump.intra.peff.net","subject":"Re: SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository.","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-08-12T21:05:42Z","receivedAt":"2024-08-12T21:05:50Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-08-12 at 11:47:33, Jeff King wrote:\n> So one option is to just do that check earlier. But that leaves other\n> sub-commands of \"git bundle\":\n> \n>   - \"create\" likewise requires a repo, and seems OK. That makes sense\n>     since we're not reading anything.\n> \n>   - \"verify\" requires a repo, which I wouldn't have expected, but I\n>     guess it's because we probably unbundle under the hood to walk.\n>     Anyway, it gets the ordering right here and checks the repo before\n>     opening the bundle.\n> \n>   - list-heads doesn't require a repo, and segfaults. So it really does\n>     need some kind of detection or default to know which hash to use.\n\nIf the bundle is v2, it's SHA-1.  If the bundle is v3, then it should\nhave a an @object-format= header, which is sufficient to set the hash\nalgorithm.  The default for v3 is still SHA-1, but we have never emitted\na v3 bundle without an object-format header (except for maybe a commit\nor two somewhere in the history).\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"500726","messageId":"cover.1723540604.git.ps@pks.im","threadId":"61934","inReplyTo":"TYWP301MB0563973ECA440E7DAF0F7E89C4852@TYWP301MB0563.JPNP301.PROD.OUTLOOK.COM","subject":"[PATCH 0/2] bundle: fix handling of object format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T09:18:05Z","receivedAt":"2024-08-13T09:18:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series addresses the segfaults reported by\nArcticLampyrid and fixes parsing of bundles that have an object hash\ndifferent than the object hash of the current repository.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (2):\n  builtin/bundle: have unbundle check for repo before opening its bundle\n  bundle: default to SHA1 when reading bundle headers\n\n builtin/bundle.c       |  5 +++--\n bundle.c               |  7 ++++++-\n t/t6020-bundle-misc.sh | 32 ++++++++++++++++++++++++++++++++\n 3 files changed, 41 insertions(+), 3 deletions(-)\n\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500727","messageId":"c57e1cca4c7d2a0f52ae8d4e0870e4e0667184fe.1723540604.git.ps@pks.im","threadId":"61934","inReplyTo":"cover.1723540604.git.ps@pks.im","subject":"[PATCH 1/2] builtin/bundle: have unbundle check for repo before opening its bundle","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T09:18:08Z","receivedAt":"2024-08-13T09:18:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `git bundle unbundle` subcommand requires a repository to unbundle\nthe contents into. As thus, the subcommand checks whether we have a\nstartup repository in the first place, and if not it dies.\n\nThis check happens after we have already opened the bundle though. This\ncauses a segfault when running outside of a repository starting with\nc8aed5e8da (repository: stop setting SHA1 as the default object hash,\n2024-05-07) because we have no hash function set up, but we do try to\nparse refs advertised by the bundle's header.\n\nThe next commit will fix that underlying issue by defaulting to the SHA1\nobject format for bundles, which will also the described segfault here.\nBut as we know that we will die anyway, we can do better than that and\navoid some vain work by moving the check for a repository before we try\nto open the bundle.\n\nReported-by: ArcticLampyrid <ArcticLampyrid@outlook.com>\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/bundle.c       | 5 +++--\n t/t6020-bundle-misc.sh | 7 +++++++\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex d5d41a8f67..86d0ed7049 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -207,12 +207,13 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n+\tif (!startup_info->have_repository)\n+\t\tdie(_(\"Need a repository to unbundle.\"));\n+\n \tif ((bundle_fd = open_bundle(bundle_file, &header, NULL)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n \t}\n-\tif (!startup_info->have_repository)\n-\t\tdie(_(\"Need a repository to unbundle.\"));\n \tif (progress)\n \t\tstrvec_pushl(&extra_index_pack_args, \"-v\", \"--progress-title\",\n \t\t\t     _(\"Unbundling objects\"), NULL);\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex fe75a06572..703434b472 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -652,4 +652,11 @@ test_expect_success 'send a bundle to standard output' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'unbundle outside of a repository' '\n+\tgit bundle create some.bundle HEAD &&\n+\techo \"fatal: Need a repository to unbundle.\" >expect &&\n+\tnongit test_must_fail git bundle unbundle \"$(pwd)/some.bundle\" 2>err &&\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500728","messageId":"7e15c3daf95283f62030fe7b5dd0bbf8e467881a.1723540604.git.ps@pks.im","threadId":"61934","inReplyTo":"cover.1723540604.git.ps@pks.im","subject":"[PATCH 2/2] bundle: default to SHA1 when reading bundle headers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-13T09:18:15Z","receivedAt":"2024-08-13T09:18:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We hit a segfault when trying to open a bundle via `git bundle\nlist-heads` when running outside of a repository. This is caused by\nc8aed5e8da (repository: stop setting SHA1 as the default object hash,\n2024-05-07), which stopped setting the default object hash so that\n`the_hash_algo` is a `NULL` pointer when running outside of any repo.\n\nThis is only a symptom of a deeper issue though. Bundles default to the\nSHA1 object format unless they advertise an \"@object-format=\" header.\nConsequently, it has been wrong in the first place to use the object\nformat used by the current repository when parsing bundles. The\nconsequence is that trying to open a bundle that uses a different object\nhash than the current repository will fail:\n\n    $ git bundle list-heads sha1.bundle\n    error: unrecognized header: ee4b540943284700a32591ad09f7e15bdeb2a10c HEAD (45)\n\nFix the bug by defaulting to the SHA1 object hash. We already handle the\n\"@object-format=\" header as expected, so we don't need to adapt this\npart.\n\nHelped-by: brian m. carlson <sandals@crustytoothpaste.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n bundle.c               |  7 ++++++-\n t/t6020-bundle-misc.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex ce164c37bc..b0a8a925cb 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -89,7 +89,12 @@ int read_bundle_header_fd(int fd, struct bundle_header *header,\n \t\tgoto abort;\n \t}\n \n-\theader->hash_algo = the_hash_algo;\n+\t/*\n+\t * The default hash format for bundles is SHA1, unless told otherwise\n+\t * by an \"object-format=\" capability, which is being handled in\n+\t * `parse_capability()`.\n+\t */\n+\theader->hash_algo = &hash_algos[GIT_HASH_SHA1];\n \n \t/* The bundle header ends with an empty line */\n \twhile (!strbuf_getwholeline_fd(&buf, fd, '\\n') &&\ndiff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh\nindex 703434b472..34b5cd62c2 100755\n--- a/t/t6020-bundle-misc.sh\n+++ b/t/t6020-bundle-misc.sh\n@@ -659,4 +659,29 @@ test_expect_success 'unbundle outside of a repository' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'list-heads outside of a repository' '\n+\tgit bundle create some.bundle HEAD &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD) HEAD\n+\tEOF\n+\tnongit git bundle list-heads \"$(pwd)/some.bundle\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+for hash in sha1 sha256\n+do\n+\ttest_expect_success \"list-heads with bundle using $hash\" '\n+\t\ttest_when_finished \"rm -rf hash\" &&\n+\t\tgit init --object-format=$hash hash &&\n+\t\ttest_commit -C hash initial &&\n+\t\tgit -C hash bundle create hash.bundle HEAD &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\t$(git -C hash rev-parse HEAD) HEAD\n+\t\tEOF\n+\t\tgit bundle list-heads hash/hash.bundle >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n test_done\n-- \n2.46.0.46.g406f326d27.dirty\n\n"},{"id":"500729","messageId":"CAPig+cQiNfD-8ByukhWgSQRBuDWgyQC_zxBF3Dhr0--m1hUFZg@mail.gmail.com","threadId":"61934","inReplyTo":"c57e1cca4c7d2a0f52ae8d4e0870e4e0667184fe.1723540604.git.ps@pks.im","subject":"Re: [PATCH 1/2] builtin/bundle: have unbundle check for repo before opening its bundle","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-08-13T09:21:32Z","receivedAt":"2024-08-13T09:21:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 13, 2024 at 5:18 AM Patrick Steinhardt <ps@pks.im> wrote:\n> The `git bundle unbundle` subcommand requires a repository to unbundle\n> the contents into. As thus, the subcommand checks whether we have a\n> startup repository in the first place, and if not it dies.\n\nPerhaps you meant: s/As thus/As such/\n\n> This check happens after we have already opened the bundle though. This\n> causes a segfault when running outside of a repository starting with\n> c8aed5e8da (repository: stop setting SHA1 as the default object hash,\n> 2024-05-07) because we have no hash function set up, but we do try to\n> parse refs advertised by the bundle's header.\n>\n> The next commit will fix that underlying issue by defaulting to the SHA1\n> object format for bundles, which will also the described segfault here.\n\nPresumably: s/also the/also fix the/\n\n> But as we know that we will die anyway, we can do better than that and\n> avoid some vain work by moving the check for a repository before we try\n> to open the bundle.\n>\n> Reported-by: ArcticLampyrid <ArcticLampyrid@outlook.com>\n> Suggested-by: Jeff King <peff@peff.net>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n"},{"id":"500767","messageId":"20240813112434.GA968816@coredump.intra.peff.net","threadId":"61934","inReplyTo":"7e15c3daf95283f62030fe7b5dd0bbf8e467881a.1723540604.git.ps@pks.im","subject":"Re: [PATCH 2/2] bundle: default to SHA1 when reading bundle headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-08-13T11:24:34Z","receivedAt":"2024-08-13T11:24:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 13, 2024 at 11:18:15AM +0200, Patrick Steinhardt wrote:\n\n> This is only a symptom of a deeper issue though. Bundles default to the\n> SHA1 object format unless they advertise an \"@object-format=\" header.\n> Consequently, it has been wrong in the first place to use the object\n> format used by the current repository when parsing bundles. The\n> consequence is that trying to open a bundle that uses a different object\n> hash than the current repository will fail:\n> \n>     $ git bundle list-heads sha1.bundle\n>     error: unrecognized header: ee4b540943284700a32591ad09f7e15bdeb2a10c HEAD (45)\n\nThat makes sense. And your test below, which covers a mismatch in the\nhash of the bundle vs the containing repo, would have failed even before\nthe segfault issue. Nice.\n\n> Fix the bug by defaulting to the SHA1 object hash. We already handle the\n> \"@object-format=\" header as expected, so we don't need to adapt this\n> part.\n\nYeah, this fix turned out to be delightfully simple as a result.\n\nBoth this patch and the first one look good to me. Thanks for jumping on\nthis.\n\n-Peff\n"},{"id":"500792","messageId":"xmqq8qx0a263.fsf@gitster.g","threadId":"61934","inReplyTo":"20240813112434.GA968816@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] bundle: default to SHA1 when reading bundle headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-13T15:11:00Z","receivedAt":"2024-08-13T15:11:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, this fix turned out to be delightfully simple as a result.\n>\n> Both this patch and the first one look good to me. Thanks for jumping on\n> this.\n\nThey look good to me too.  Thanks, all.\n"}]}