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

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

[ArcticLampyrid]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

[PATCH 0/2] bundle: fix handling of object format

[Patrick Steinhardt]

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

[PATCH 1/2] builtin/bundle: have unbundle check for repo before opening its bundle

[Patrick Steinhardt]

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

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

Re: [PATCH 1/2] builtin/bundle: have unbundle check for repo before opening its bundle

[Eric Sunshine]

On Tue, Aug 13, 2024 at 5:18 AM Patrick Steinhardt <ps@pks.im> 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 <ArcticLampyrid@outlook.com>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

[PATCH 2/2] bundle: default to SHA1 when reading bundle headers

[Patrick Steinhardt]

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

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

Re: [PATCH 2/2] bundle: default to SHA1 when reading bundle headers

[Jeff King]

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

Re: [PATCH 2/2] bundle: default to SHA1 when reading bundle headers

[Junio C Hamano]

Jeff King <peff@peff.net> 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.