# SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository. 10 messages from 2024-08-12 to 2024-08-13. Participants: ArcticLampyrid, Jeff King, Junio C Hamano, brian m. carlson, Patrick Steinhardt, Eric Sunshine. Thread: https://gitlist.dev/t/61934 ## ArcticLampyrid, 2024-08-12 10:57 Subject: SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository. Message-ID: URL: https://gitlist.dev/e/TYWP301MB0563973ECA440E7DAF0F7E89C4852%40TYWP301MB0563.JPNP301.PROD.OUTLOOK.COM ``` #### Steps to Reproduce I attempted to unbundle a pack but forgot to execute `git init` beforehand. #### Expected Behavior An error message should have been displayed, reminding me to run `git init`. #### Actual Behavior The process unexpectedly terminated with a SIGSEGV (Address boundary error). #### Difference Between Expected and Actual Behavior Instead of gracefully exiting, the process terminated abruptly. #### System Info - Git Version: 2.46.0 - CPU: x86_64 - No commit associated with this build - Size of `long`: 8 bytes - Size of `size_t`: 8 bytes - Shell Path: /bin/sh - libcurl Version: 8.9.0 - OpenSSL Version: 3.3.1 (4 Jun 2024) - zlib Version: 1.3.1 - uname: Linux 6.10.3-arch1-2 #1 SMP PREEMPT_DYNAMIC Tue, 06 Aug 2024 07:21:19 +0000 x86_64 - Compiler Info: GNU Compiler Collection (GCC) 14.1 - libc Version: glibc 2.40 - Interactive Shell: /usr/bin/fish ``` ## Jeff King, 2024-08-12 11:47 Subject: Re: SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository. Message-ID: <20240812114733.GA3732689@coredump.intra.peff.net> URL: https://gitlist.dev/e/20240812114733.GA3732689%40coredump.intra.peff.net In-Reply-To: ``` On Mon, Aug 12, 2024 at 06:57:25PM +0800, ArcticLampyrid wrote: > #### Steps to Reproduce > I attempted to unbundle a pack but forgot to execute `git init` beforehand. > > #### Expected Behavior > An error message should have been displayed, reminding me to run `git init`. > > #### Actual Behavior > The process unexpectedly terminated with a SIGSEGV (Address boundary error). > > #### Difference Between Expected and Actual Behavior > Instead of gracefully exiting, the process terminated abruptly. Thanks for the report, it's pretty easy to reproduce. Looks like another casualty of c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07). Author cc'd. A sample stack trace is: #0 0x000055555573a93c in get_hash_hex_algop ( hex=0x555555a11180 "b2f0a7f47f5f2aebe1e7fceff19a57de20a78c06 refs/heads/master", hash=0x7fffffffdc00 "$h\240UUU", algop=0x0) at hex.c:11 #1 0x000055555573a9aa in get_oid_hex_algop ( hex=0x555555a11180 "b2f0a7f47f5f2aebe1e7fceff19a57de20a78c06 refs/heads/master", oid=0x7fffffffdc00, algop=0x0) at hex.c:29 #2 0x000055555573aad1 in parse_oid_hex_algop ( hex=0x555555a11180 "b2f0a7f47f5f2aebe1e7fceff19a57de20a78c06 refs/heads/master", oid=0x7fffffffdc00, end=0x7fffffffdc28, algop=0x0) at hex.c:62 #3 0x00005555556acaf3 in read_bundle_header_fd (fd=3, header=0x7fffffffddf0, report_path=0x555555a110a0 "foo") at bundle.c:121 #4 0x00005555556accdf in read_bundle_header (path=0x555555a110a0 "foo", header=0x7fffffffddf0) at bundle.c:153 #5 0x000055555558c8d6 in open_bundle (path=0x555555a110a0 "foo", header=0x7fffffffddf0, name=0x0) at builtin/bundle.c:123 #6 0x000055555558cd8b in cmd_bundle_unbundle (argc=1, argv=0x7fffffffe4b0, prefix=0x0) at builtin/bundle.c:210 #7 0x000055555558cff1 in cmd_bundle (argc=2, argv=0x7fffffffe4b0, prefix=0x0) at builtin/bundle.c:244 Curiously, the next line after the open_bundle() call is: if (!startup_info->have_repository) die(_("Need a repository to unbundle.")); So one option is to just do that check earlier. But that leaves other sub-commands of "git bundle": - "create" likewise requires a repo, and seems OK. That makes sense since we're not reading anything. - "verify" requires a repo, which I wouldn't have expected, but I guess it's because we probably unbundle under the hood to walk. Anyway, it gets the ordering right here and checks the repo before opening the bundle. - list-heads doesn't require a repo, and segfaults. So it really does need some kind of detection or default to know which hash to use. -Peff ``` ## Junio C Hamano, 2024-08-12 15:55 Subject: Re: SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository. Message-ID: URL: https://gitlist.dev/e/xmqq1q2t7n33.fsf%40gitster.g In-Reply-To: <20240812114733.GA3732689@coredump.intra.peff.net> ``` Jeff King writes: > - "verify" requires a repo, which I wouldn't have expected, but I > guess it's because we probably unbundle under the hood to walk. > Anyway, it gets the ordering right here and checks the repo before > opening the bundle. In hindsight "verify" is misnamed and overrated. Its purpose is to check if the bundle can be unbundled into your _current_ repository by checking if you have all the commits _required_ to unbundle the bundle. In fact, I doubt that "verify" looks at the pack stream part of the file at all. > - list-heads doesn't require a repo, and segfaults. So it really does > need some kind of detection or default to know which hash to use. Yes. ``` ## brian m. carlson, 2024-08-12 21:05 Subject: Re: SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository. Message-ID: URL: https://gitlist.dev/e/Zrp5JnsVHbqs4TyN%40tapette.crustytoothpaste.net In-Reply-To: <20240812114733.GA3732689@coredump.intra.peff.net> ``` On 2024-08-12 at 11:47:33, Jeff King wrote: > So one option is to just do that check earlier. But that leaves other > sub-commands of "git bundle": > > - "create" likewise requires a repo, and seems OK. That makes sense > since we're not reading anything. > > - "verify" requires a repo, which I wouldn't have expected, but I > guess it's because we probably unbundle under the hood to walk. > Anyway, it gets the ordering right here and checks the repo before > opening the bundle. > > - list-heads doesn't require a repo, and segfaults. So it really does > need some kind of detection or default to know which hash to use. If the bundle is v2, it's SHA-1. If the bundle is v3, then it should have a an @object-format= header, which is sufficient to set the hash algorithm. The default for v3 is still SHA-1, but we have never emitted a v3 bundle without an object-format header (except for maybe a commit or two somewhere in the history). -- brian m. carlson (they/them or he/him) Toronto, Ontario, CA ``` ## Patrick Steinhardt, 2024-08-13 09:18 Subject: [PATCH 0/2] bundle: fix handling of object format Message-ID: URL: https://gitlist.dev/e/cover.1723540604.git.ps%40pks.im In-Reply-To: ``` Hi, this small patch series addresses the segfaults reported by ArcticLampyrid and fixes parsing of bundles that have an object hash different than the object hash of the current repository. Thanks! Patrick Patrick Steinhardt (2): builtin/bundle: have unbundle check for repo before opening its bundle bundle: default to SHA1 when reading bundle headers builtin/bundle.c | 5 +++-- bundle.c | 7 ++++++- t/t6020-bundle-misc.sh | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) -- 2.46.0.46.g406f326d27.dirty ``` ## Patrick Steinhardt, 2024-08-13 09:18 Subject: [PATCH 1/2] builtin/bundle: have unbundle check for repo before opening its bundle Message-ID: URL: https://gitlist.dev/e/c57e1cca4c7d2a0f52ae8d4e0870e4e0667184fe.1723540604.git.ps%40pks.im In-Reply-To: ``` The `git bundle unbundle` subcommand requires a repository to unbundle the contents into. As thus, the subcommand checks whether we have a startup repository in the first place, and if not it dies. This check happens after we have already opened the bundle though. This causes a segfault when running outside of a repository starting with c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07) because we have no hash function set up, but we do try to parse refs advertised by the bundle's header. The next commit will fix that underlying issue by defaulting to the SHA1 object format for bundles, which will also the described segfault here. But as we know that we will die anyway, we can do better than that and avoid some vain work by moving the check for a repository before we try to open the bundle. Reported-by: ArcticLampyrid Suggested-by: Jeff King Signed-off-by: Patrick Steinhardt --- builtin/bundle.c | 5 +++-- t/t6020-bundle-misc.sh | 7 +++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/builtin/bundle.c b/builtin/bundle.c index d5d41a8f67..86d0ed7049 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -207,12 +207,13 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) builtin_bundle_unbundle_usage, options, &bundle_file); /* bundle internals use argv[1] as further parameters */ + if (!startup_info->have_repository) + die(_("Need a repository to unbundle.")); + if ((bundle_fd = open_bundle(bundle_file, &header, NULL)) < 0) { ret = 1; goto cleanup; } - if (!startup_info->have_repository) - die(_("Need a repository to unbundle.")); if (progress) strvec_pushl(&extra_index_pack_args, "-v", "--progress-title", _("Unbundling objects"), NULL); diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh index fe75a06572..703434b472 100755 --- a/t/t6020-bundle-misc.sh +++ b/t/t6020-bundle-misc.sh @@ -652,4 +652,11 @@ test_expect_success 'send a bundle to standard output' ' test_cmp expect actual ' +test_expect_success 'unbundle outside of a repository' ' + git bundle create some.bundle HEAD && + echo "fatal: Need a repository to unbundle." >expect && + nongit test_must_fail git bundle unbundle "$(pwd)/some.bundle" 2>err && + test_cmp expect err +' + test_done -- 2.46.0.46.g406f326d27.dirty ``` ## Patrick Steinhardt, 2024-08-13 09:18 Subject: [PATCH 2/2] bundle: default to SHA1 when reading bundle headers Message-ID: <7e15c3daf95283f62030fe7b5dd0bbf8e467881a.1723540604.git.ps@pks.im> URL: https://gitlist.dev/e/7e15c3daf95283f62030fe7b5dd0bbf8e467881a.1723540604.git.ps%40pks.im In-Reply-To: ``` We hit a segfault when trying to open a bundle via `git bundle list-heads` when running outside of a repository. This is caused by c8aed5e8da (repository: stop setting SHA1 as the default object hash, 2024-05-07), which stopped setting the default object hash so that `the_hash_algo` is a `NULL` pointer when running outside of any repo. This is only a symptom of a deeper issue though. Bundles default to the SHA1 object format unless they advertise an "@object-format=" header. Consequently, it has been wrong in the first place to use the object format used by the current repository when parsing bundles. The consequence is that trying to open a bundle that uses a different object hash than the current repository will fail: $ git bundle list-heads sha1.bundle error: unrecognized header: ee4b540943284700a32591ad09f7e15bdeb2a10c HEAD (45) Fix the bug by defaulting to the SHA1 object hash. We already handle the "@object-format=" header as expected, so we don't need to adapt this part. Helped-by: brian m. carlson Signed-off-by: Patrick Steinhardt --- bundle.c | 7 ++++++- t/t6020-bundle-misc.sh | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/bundle.c b/bundle.c index ce164c37bc..b0a8a925cb 100644 --- a/bundle.c +++ b/bundle.c @@ -89,7 +89,12 @@ int read_bundle_header_fd(int fd, struct bundle_header *header, goto abort; } - header->hash_algo = the_hash_algo; + /* + * The default hash format for bundles is SHA1, unless told otherwise + * by an "object-format=" capability, which is being handled in + * `parse_capability()`. + */ + header->hash_algo = &hash_algos[GIT_HASH_SHA1]; /* The bundle header ends with an empty line */ while (!strbuf_getwholeline_fd(&buf, fd, '\n') && diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh index 703434b472..34b5cd62c2 100755 --- a/t/t6020-bundle-misc.sh +++ b/t/t6020-bundle-misc.sh @@ -659,4 +659,29 @@ test_expect_success 'unbundle outside of a repository' ' test_cmp expect err ' +test_expect_success 'list-heads outside of a repository' ' + git bundle create some.bundle HEAD && + cat >expect <<-EOF && + $(git rev-parse HEAD) HEAD + EOF + nongit git bundle list-heads "$(pwd)/some.bundle" >actual && + test_cmp expect actual +' + +for hash in sha1 sha256 +do + test_expect_success "list-heads with bundle using $hash" ' + test_when_finished "rm -rf hash" && + git init --object-format=$hash hash && + test_commit -C hash initial && + git -C hash bundle create hash.bundle HEAD && + + cat >expect <<-EOF && + $(git -C hash rev-parse HEAD) HEAD + EOF + git bundle list-heads hash/hash.bundle >actual && + test_cmp expect actual + ' +done + test_done -- 2.46.0.46.g406f326d27.dirty ``` ## Eric Sunshine, 2024-08-13 09:21 Subject: Re: [PATCH 1/2] builtin/bundle: have unbundle check for repo before opening its bundle Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQiNfD-8ByukhWgSQRBuDWgyQC_zxBF3Dhr0--m1hUFZg%40mail.gmail.com In-Reply-To: ``` On Tue, Aug 13, 2024 at 5:18 AM Patrick Steinhardt wrote: > The `git bundle unbundle` subcommand requires a repository to unbundle > the contents into. As thus, the subcommand checks whether we have a > startup repository in the first place, and if not it dies. Perhaps you meant: s/As thus/As such/ > This check happens after we have already opened the bundle though. This > causes a segfault when running outside of a repository starting with > c8aed5e8da (repository: stop setting SHA1 as the default object hash, > 2024-05-07) because we have no hash function set up, but we do try to > parse refs advertised by the bundle's header. > > The next commit will fix that underlying issue by defaulting to the SHA1 > object format for bundles, which will also the described segfault here. Presumably: s/also the/also fix the/ > But as we know that we will die anyway, we can do better than that and > avoid some vain work by moving the check for a repository before we try > to open the bundle. > > Reported-by: ArcticLampyrid > Suggested-by: Jeff King > Signed-off-by: Patrick Steinhardt ``` ## Jeff King, 2024-08-13 11:24 Subject: Re: [PATCH 2/2] bundle: default to SHA1 when reading bundle headers Message-ID: <20240813112434.GA968816@coredump.intra.peff.net> URL: https://gitlist.dev/e/20240813112434.GA968816%40coredump.intra.peff.net In-Reply-To: <7e15c3daf95283f62030fe7b5dd0bbf8e467881a.1723540604.git.ps@pks.im> ``` On Tue, Aug 13, 2024 at 11:18:15AM +0200, Patrick Steinhardt wrote: > This is only a symptom of a deeper issue though. Bundles default to the > SHA1 object format unless they advertise an "@object-format=" header. > Consequently, it has been wrong in the first place to use the object > format used by the current repository when parsing bundles. The > consequence is that trying to open a bundle that uses a different object > hash than the current repository will fail: > > $ git bundle list-heads sha1.bundle > error: unrecognized header: ee4b540943284700a32591ad09f7e15bdeb2a10c HEAD (45) That makes sense. And your test below, which covers a mismatch in the hash of the bundle vs the containing repo, would have failed even before the segfault issue. Nice. > Fix the bug by defaulting to the SHA1 object hash. We already handle the > "@object-format=" header as expected, so we don't need to adapt this > part. Yeah, this fix turned out to be delightfully simple as a result. Both this patch and the first one look good to me. Thanks for jumping on this. -Peff ``` ## Junio C Hamano, 2024-08-13 15:11 Subject: Re: [PATCH 2/2] bundle: default to SHA1 when reading bundle headers Message-ID: URL: https://gitlist.dev/e/xmqq8qx0a263.fsf%40gitster.g In-Reply-To: <20240813112434.GA968816@coredump.intra.peff.net> ``` Jeff King writes: > Yeah, this fix turned out to be delightfully simple as a result. > > Both this patch and the first one look good to me. Thanks for jumping on > this. They look good to me too. Thanks, all. ```