SIGSEGV Error Occurs When Attempting to Unbundle Without Initializing Git Repository.

#### 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

On Mon, Aug 12, 2024 at 06:57:25PM +0800, ArcticLampyrid wrote:

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

Jeff King <peff@peff.net> writes:

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.

Yes.

On 2024-08-12 at 11:47:33, Jeff King wrote:

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).

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(-)

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 <ArcticLampyrid@outlook.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/bundle.c       | 5 +++--
 t/t6020-bundle-misc.sh | 7 +++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

On Tue, Aug 13, 2024 at 5:18 AM Patrick Steinhardt <ps@pks.im> wrote:

Perhaps you meant: s/As thus/As such/

Presumably: s/also the/also fix the/

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 <sandals@crustytoothpaste.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bundle.c               |  7 ++++++-
 t/t6020-bundle-misc.sh | 25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

On Tue, Aug 13, 2024 at 11:18:15AM +0200, Patrick Steinhardt wrote:

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.

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

Jeff King <peff@peff.net> writes:

They look good to me too.  Thanks, all.