Both of these commands read the incoming pack into a static unsigned
char buffer in BSS, and then parse it by casting the start of the buffer
to a struct pack_header. This can result in SIGBUS on some platforms if
the compiler doesn't place the buffer in a position that is properly
aligned for 4-byte integers.
This reportedly happens with unpack-objects (but not index-pack) on
sparc64 when compiled with clang (but not gcc). But we are definitely in
the wrong in both spots; since the buffer's type is unsigned char, we
can't depend on larger alignment. When it works it is only because we
are lucky.
We'll fix this by switching to get_be32() to read the headers (just like
the last few commits similarly switched us to put_be32() for writing
into the same buffer).
It would be nice to factor this out into a common helper function, but
the interface ends up quite awkward. Either the caller needs to hardcode
how many bytes we'll need, or it needs to pass us its fill()/use()
functions as pointers. So I've just fixed both spots in the same way;
this is not code that is likely to be repeated a third time (most of the
pack reading code uses an mmap'd buffer, which should be properly
aligned).
I did make one tweak to the shared code: our pack_version_ok() macro
expects us to pass the big-endian value we'd get by casting. We can
introduce a "native" variant which uses the host integer ordering.
Reported-by: Koakuma <koachan@xxxxxxxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
New in v2.
I did write the function-pointer version, and it's not even _too_ bad to
read. But while trying to write a comment documenting the helper, it
was just too gross to justify.
builtin/index-pack.c | 12 +++++++-----
builtin/unpack-objects.c | 13 +++++++------
pack.h | 3 ++-
3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 75b84f78f4..d6fd4bbde6 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -379,16 +379,18 @@ static const char *open_pack_file(const char *pack_name)
static void parse_pack_header(void)
{
- struct pack_header *hdr = fill(sizeof(struct pack_header));
+ unsigned char *hdr = fill(sizeof(struct pack_header));
/* Header consistency check */
- if (hdr->hdr_signature != htonl(PACK_SIGNATURE))
+ if (get_be32(hdr) != PACK_SIGNATURE)
die(_("pack signature mismatch"));
- if (!pack_version_ok(hdr->hdr_version))
+ hdr += 4;
+ if (!pack_version_ok_native(get_be32(hdr)))
die(_("pack version %"PRIu32" unsupported"),
- ntohl(hdr->hdr_version));
+ get_be32(hdr));
+ hdr += 4;
- nr_objects = ntohl(hdr->hdr_entries);
+ nr_objects = get_be32(hdr);
use(sizeof(struct pack_header));
}
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index cf2bc5c531..76c6a9031b 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -579,15 +579,16 @@ static void unpack_one(unsigned nr)
static void unpack_all(void)
{
int i;
- struct pack_header *hdr = fill(sizeof(struct pack_header));
+ unsigned char *hdr = fill(sizeof(struct pack_header));
- nr_objects = ntohl(hdr->hdr_entries);
-
- if (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)
+ if (get_be32(hdr) != PACK_SIGNATURE)
die("bad pack file");
- if (!pack_version_ok(hdr->hdr_version))
+ hdr += 4;
+ if (!pack_version_ok_native(get_be32(hdr)))
die("unknown pack file version %"PRIu32,
- ntohl(hdr->hdr_version));
+ get_be32(hdr));
+ hdr += 4;
+ nr_objects = get_be32(hdr);
use(sizeof(struct pack_header));
if (!quiet)
diff --git a/pack.h b/pack.h
index a8da040629..78f8d8b213 100644
--- a/pack.h
+++ b/pack.h
@@ -13,7 +13,8 @@ struct repository;
*/
#define PACK_SIGNATURE 0x5041434b /* "PACK" */
#define PACK_VERSION 2
-#define pack_version_ok(v) ((v) == htonl(2) || (v) == htonl(3))
+#define pack_version_ok(v) pack_version_ok_native(ntohl(v))
+#define pack_version_ok_native(v) ((v) == 2 || (v) == 3)
struct pack_header {
uint32_t hdr_signature;
uint32_t hdr_version;
--
2.48.1.466.g22b8cfd1fa
