Re: [BUG] git crashes with a SIGBUS on sparc64 during pull

From: Jeff King <peff@peff.net>
Date: Jan 17, 2025, 12:11 UTC
Message-ID: <20250117121121.GA2356599@coredump.intra.peff.net>
In-Reply-To: <xd6Mn7uXpDxaT8zF28BwOIdV98lnvm3S0rBgxmm97nuRW3z0GQSUi_KeYe0fUuRfz8rCgo6Uialw5rnraqp5tjzMFQkFH0MZV_n4fEQg8go=@protonmail.com>

On Fri, Jan 17, 2025 at 03:30:09AM +0000, Koakuma wrote:

> re: unpack-objects, I don't know how to specifically trigger that code,
> but I know that once `unpack-objects` is triggered it will reliably crash
> with the error above.

Perhaps:

  git init repo
  cd repo
  git commit --allow-empty -m foo
  git repack -ad
  pack=$(ls .git/objects/pack/*.pack)
  dd if=$pack of=no-header.pack bs=1 skip=12
  # don't bother parsing the first 12 bytes; we know it is
  # a version 2 pack with 2 objects
  git unpack-objects --pack_header=2,2 <no-header.pack

would be a minimal reproduction?

Show 7 quoted lines

> According to gdb, the crash happens on this line:
> #0  0x000001000019ca18 in cmd_unpack_objects (argc=<optimized out>, argv=0x1000063a4e0, prefix=<optimized out>, repo=<optimized out>) at builtin/unpack-objects.c:653
> 653					hdr->hdr_signature = htonl(PACK_SIGNATURE);
> 
> Overaligning the `buffer` declaration in the same file to try to get around
> possible alignment issues seems to be able to prevent the crash,
> but I don't know if it would be a proper fix for it.

Interesting. We are pretty cavalier about casting mmap'd buffers to structs in order to read pack headers (since the format is designed to be 4-byte aligned).

But in this case we are _writing_ into a buffer through the cast structure (our caller has already parsed the header, so we are reconstructing it). So we are relying on the alignment of a static "unsigned char" in the data section.

Which certainly seems sketchy, though it is kind of interesting that it has never been a problem before (and the code has been this way for decades). At any rate, the fix is probably this (and we'd want the same in index-pack, too, I'd think):

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 2197d6d933..288cecf98f 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -645,18 +645,20 @@ int cmd_unpack_objects(int argc,
 				continue;
 			}
 			if (starts_with(arg, "--pack_header=")) {
-				struct pack_header *hdr;
+				unsigned char *hdr = buffer;
 				char *c;
 
-				hdr = (struct pack_header *)buffer;
-				hdr->hdr_signature = htonl(PACK_SIGNATURE);
-				hdr->hdr_version = htonl(strtoul(arg + 14, &c, 10));
+				put_be32(hdr, PACK_SIGNATURE);
+				hdr += 4;
+				put_be32(hdr, strtoul(arg + 14, &c, 10));
+				hdr += 4;
 				if (*c != ',')
 					die("bad %s", arg);
-				hdr->hdr_entries = htonl(strtoul(c + 1, &c, 10));
+				put_be32(hdr, strtoul(c + 1, &c, 10));
+				hdr += 4;
 				if (*c)
 					die("bad %s", arg);
-				len = sizeof(*hdr);
+				len = hdr - buffer;
 				continue;
 			}
 			if (skip_prefix(arg, "--max-input-size=", &arg)) {