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?
> 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)) {
