Skip to content

Commit 2fe489d

Browse files
peffdscho
authored andcommitted
index-pack, unpack-objects: use get_be32() for reading pack header
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 <[email protected]> Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
1 parent 9992be6 commit 2fe489d

File tree

3 files changed

+16
-12
lines changed

3 files changed

+16
-12
lines changed

builtin/index-pack.c

+7-5
Original file line numberDiff line numberDiff line change
@@ -379,16 +379,18 @@ static const char *open_pack_file(const char *pack_name)
379379

380380
static void parse_pack_header(void)
381381
{
382-
struct pack_header *hdr = fill(sizeof(struct pack_header));
382+
unsigned char *hdr = fill(sizeof(struct pack_header));
383383

384384
/* Header consistency check */
385-
if (hdr->hdr_signature != htonl(PACK_SIGNATURE))
385+
if (get_be32(hdr) != PACK_SIGNATURE)
386386
die(_("pack signature mismatch"));
387-
if (!pack_version_ok(hdr->hdr_version))
387+
hdr += 4;
388+
if (!pack_version_ok_native(get_be32(hdr)))
388389
die(_("pack version %"PRIu32" unsupported"),
389-
ntohl(hdr->hdr_version));
390+
get_be32(hdr));
391+
hdr += 4;
390392

391-
nr_objects = ntohl(hdr->hdr_entries);
393+
nr_objects = get_be32(hdr);
392394
use(sizeof(struct pack_header));
393395
}
394396

builtin/unpack-objects.c

+7-6
Original file line numberDiff line numberDiff line change
@@ -579,15 +579,16 @@ static void unpack_one(unsigned nr)
579579
static void unpack_all(void)
580580
{
581581
int i;
582-
struct pack_header *hdr = fill(sizeof(struct pack_header));
582+
unsigned char *hdr = fill(sizeof(struct pack_header));
583583

584-
nr_objects = ntohl(hdr->hdr_entries);
585-
586-
if (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)
584+
if (get_be32(hdr) != PACK_SIGNATURE)
587585
die("bad pack file");
588-
if (!pack_version_ok(hdr->hdr_version))
586+
hdr += 4;
587+
if (!pack_version_ok_native(get_be32(hdr)))
589588
die("unknown pack file version %"PRIu32,
590-
ntohl(hdr->hdr_version));
589+
get_be32(hdr));
590+
hdr += 4;
591+
nr_objects = get_be32(hdr);
591592
use(sizeof(struct pack_header));
592593

593594
if (!quiet)

pack.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ struct repository;
1313
*/
1414
#define PACK_SIGNATURE 0x5041434b /* "PACK" */
1515
#define PACK_VERSION 2
16-
#define pack_version_ok(v) ((v) == htonl(2) || (v) == htonl(3))
16+
#define pack_version_ok(v) pack_version_ok_native(ntohl(v))
17+
#define pack_version_ok_native(v) ((v) == 2 || (v) == 3)
1718
struct pack_header {
1819
uint32_t hdr_signature;
1920
uint32_t hdr_version;

0 commit comments

Comments
 (0)