threads / patch / 4121

patchFix git-pack-objects for 64-bit platforms

Subject: [PATCH] Fix git-pack-objects for 64-bit platforms

## tl;dr

10 messages between May 11, 2006 and May 20, 2006. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Dennis Stosberg· May 11, 2006, 17:36 UTC · lore

The offset of an object in the pack is recorded as a 4-byte integer in the index file. When reading the offset from the mmap'ed index in prepare_pack_revindex(), the address is dereferenced as a long*. This works fine as long as the long type is four bytes wide. On NetBSD/sparc64, however, a long is 8 bytes wide and so dereferencing the offset produces garbage.

Signed-off-by: Dennis Stosberg <dennis@stosberg.net>
---

I am not sure whether an int cast or an int32_t cast is more appropriate here. An int is not guaranteed to be four bytes wide, but I don't know of any modern platform where that's not the case. On the other hand int32_t is not necessarily available before C99.

Any opinions?  I wonder why no one has hit this on x86_64...
 pack-objects.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
026b1b2cdd5332f59e15cd8611a49ead3094d08c
Show changes to pack-objects.c +1 −1
diff --git a/pack-objects.c b/pack-objects.c
index 523a1c7..29bda43 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -156,7 +156,7 @@ static void prepare_pack_revindex(struct
 
 	rix->revindex = xmalloc(sizeof(unsigned long) * (num_ent + 1));
 	for (i = 0; i < num_ent; i++) {
-		long hl = *((long *)(index + 24 * i));
+		long hl = *((int *)(index + 24 * i));
 		rix->revindex[i] = ntohl(hl);
 	}
 	/* This knows the pack format -- the 20-byte trailer

-- 
1.3.2.gbe65
Linus Torvalds· May 11, 2006, 17:58 UTC · re: Dennis Stosberg · lore

Re: [PATCH] Fix git-pack-objects for 64-bit platforms

On Thu, 11 May 2006, Dennis Stosberg wrote:
Show 7 quoted lines
> 
> I am not sure whether an int cast or an int32_t cast is more
> appropriate here.  An int is not guaranteed to be four bytes wide,
> but I don't know of any modern platform where that's not the case.
> On the other hand int32_t is not necessarily available before C99.
> 
> Any opinions?  I wonder why no one has hit this on x86_64...

I think the "ntohl()" hides it. It loads a 64-bit value, but since x86-64 is little-endian, the low 32 bits are correct. The htonl() will then strip the high bits and make it all be big-endian.

And while I actually run a 64-bit big-endian machine myself (G5 ppc64), my user space is all 32-bit by default, so it never showed up on linux-ppc64 either.

Anyway, the correct type to use is "uint32_t" in this case. That's what htonl() takes.

		Linus
Junio C Hamano· May 11, 2006, 18:52 UTC · re: Linus Torvalds · lore

Re: [PATCH] Fix git-pack-objects for 64-bit platforms

Linus Torvalds <torvalds@osdl.org> writes:
Show 12 quoted lines
> On Thu, 11 May 2006, Dennis Stosberg wrote:
>> 
>> I am not sure whether an int cast or an int32_t cast is more
>> appropriate here.  An int is not guaranteed to be four bytes wide,
>> but I don't know of any modern platform where that's not the case.
>> On the other hand int32_t is not necessarily available before C99.
>> 
>> Any opinions?  I wonder why no one has hit this on x86_64...
>
> I think the "ntohl()" hides it. It loads a 64-bit value, but since x86-64 
> is little-endian, the low 32 bits are correct. The htonl() will then strip 
> the high bits and make it all be big-endian.
That sounds sensible.

Since I saw a patch that touches only one place, I thought I'd better point this out...

There are a few more places that knows about this ((char*)base_pointer + (entry_count * 24)) magic in our code.

$ git grep -n -e '24  *\*' -e '\*  *24' master -- '*.c'
master:pack-objects.c:159:		long hl = *((long *)(index + 24 * i));
	This is yours.

master:sha1_file.c:447: if (idx_size != 4*256 + nr * 24 + 20 + 20) master:sha1_file.c:1148: memcpy(sha1, (index + 24 * n + 4), 20); master:sha1_file.c:1162: int cmp = memcmp(index + 24 * mi + 4, sha1, 20);

	These three should be OK, I think.
master:sha1_file.c:1164:			e->offset = ntohl(*((int*)(index + 24 * mi)));
	This you might want to look at; I suspect it is what
	Linus suggests.

Also we _might_ have uglier magic that assumes the base_pointer to be a pointer to a 4-byte integer and uses offset of multiple of 6 instead of 24, although I do not think it is likely.

I have to leave the keyboard in a few minutes so I cannot verify nor fix them myself for the next 8 hours or so. Sorry.

Show 6 quoted lines
> And while I actually run a 64-bit big-endian machine myself (G5 ppc64), my 
> user space is all 32-bit by default, so it never showed up on linux-ppc64 
> either.
>
> Anyway, the correct type to use is "uint32_t" in this case. That's what 
> htonl() takes.

Is uint32_t guaranteed to be exactly 32-bit, or merely enough to hold 32-bit?

Linus Torvalds· May 11, 2006, 19:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix git-pack-objects for 64-bit platforms

On Thu, 11 May 2006, Junio C Hamano wrote:
> 
> Is uint32_t guaranteed to be exactly 32-bit, or merely enough to
> hold 32-bit?

I think it's guaranteed to be 32-bit, but regardless, the current git headers already assume that "unsigned int" is 32-bit.

Which is a pretty safe assumption for at least the next ten years or so, possibly much longer. So I don't think we need to worry _too_ much about this. I think it's more important to try to get git working on Windows, than on 16-bit DOS or on a PDP-9, or one of the odd cray machines.

		Linus
Linus Torvalds· May 11, 2006, 19:27 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix git-pack-objects for 64-bit platforms

On Thu, 11 May 2006, Junio C Hamano wrote:
Show 6 quoted lines
> 
> Since I saw a patch that touches only one place, I thought I'd
> better point this out...
> 
> There are a few more places that knows about this
> ((char*)base_pointer + (entry_count * 24)) magic in our code.

Since I _do_ have a 64-bit big-endian architecture, I decided to install some of the 64-bit development libraries that I didn't already have, and I added "-m64" to the compiler flags.

All the tests seem to pass with the simple fix (and without it, we do indeed fail at least t5700-clone-reference.sh).

Of course, there might well be something else that doesn't get coverage, but passing all tests is at least a good sign. And since x86-64 has been getting pretty extensive coverage, I don't think we have a lot of 64-bit bugs per se, this one just happened to hide.

		Linus
---
Show changes to pack-objects.c +1 −1
diff --git a/pack-objects.c b/pack-objects.c
index 523a1c7..1b9e7a1 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -156,7 +156,7 @@ static void prepare_pack_revindex(struct
 
 	rix->revindex = xmalloc(sizeof(unsigned long) * (num_ent + 1));
 	for (i = 0; i < num_ent; i++) {
-		long hl = *((long *)(index + 24 * i));
+		uint32_t hl = *((uint32_t *)(index + 24 * i));
 		rix->revindex[i] = ntohl(hl);
 	}
 	/* This knows the pack format -- the 20-byte trailer
Junio C Hamano· May 13, 2006, 05:58 UTC · re: Linus Torvalds · lore

Re: [PATCH] Fix git-pack-objects for 64-bit platforms

Linus Torvalds <torvalds@osdl.org> writes:
Show 13 quoted lines
> Since I _do_ have a 64-bit big-endian architecture, I decided to install 
> some of the 64-bit development libraries that I didn't already have, and I 
> added "-m64" to the compiler flags.
>
> All the tests seem to pass with the simple fix (and without it, we do 
> indeed fail at least t5700-clone-reference.sh).
>
> Of course, there might well be something else that doesn't get coverage, 
> but passing all tests is at least a good sign. And since x86-64 has been 
> getting pretty extensive coverage, I don't think we have a lot of 64-bit 
> bugs per se, this one just happened to hide.
>
> 		Linus

Thanks, both. This is what I am thinking of applying (but I am sort of burned-out right now after a two-day meeting with my sponsors, so the real work will be tomorrow).

It takes the uint32_t version but matches another place to use that type instead of "int" (which would not matter in next 10 years perhaps).

-- >8 --
Show changes to 2 files +2 −2

pack-objects.c, sha1_file.c

diff --git a/pack-objects.c b/pack-objects.c
index c0acc46..a81d609 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -156,7 +156,7 @@ static void prepare_pack_revindex(struct
 
 	rix->revindex = xmalloc(sizeof(unsigned long) * (num_ent + 1));
 	for (i = 0; i < num_ent; i++) {
-		long hl = *((long *)(index + 24 * i));
+		uint32_t hl = *((uint32_t *)(index + 24 * i));
 		rix->revindex[i] = ntohl(hl);
 	}
 	/* This knows the pack format -- the 20-byte trailer
diff --git a/sha1_file.c b/sha1_file.c
index f2d33af..642c45a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1126,7 +1126,7 @@ int find_pack_entry_one(const unsigned c
 		int mi = (lo + hi) / 2;
 		int cmp = memcmp(index + 24 * mi + 4, sha1, 20);
 		if (!cmp) {
-			e->offset = ntohl(*((int*)(index + 24 * mi)));
+			e->offset = ntohl(*((uint32_t *)(index + 24 * mi)));
 			memcpy(e->sha1, sha1, 20);
 			e->p = p;
 			return 1;
Ben Clifford· May 14, 2006, 20:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix git-pack-objects for 64-bit platforms

> It takes the uint32_t version but matches another place to use
> that type instead of "int" (which would not matter in next 10
> years perhaps).

This use of uint32_t breaks the build for me on Mac OS X. Including <stdint.h> seems to make it work again. See following patch.

Ben Clifford· May 14, 2006, 20:58 UTC · re: Ben Clifford · lore

[PATCH] include header to define uint32_t, necessary on Mac OS X

From: Ben Clifford <benc@hawaga.org.uk>
Date: Sun, 14 May 2006 21:34:56 +0100
Subject: [PATCH] include header to define uint32_t, necessary on Mac OS X
---
  pack-objects.c |    1 +
  sha1_file.c    |    1 +
  2 files changed, 2 insertions(+), 0 deletions(-)
2ee926ab9da67ef2a6ca28bb70954a33d65ba466
Show changes to 2 files +2 −0

pack-objects.c, sha1_file.c

diff --git a/pack-objects.c b/pack-objects.c
index 1b9e7a1..5466b15 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -10,6 +10,7 @@ #include "csum-file.h"
  #include "tree-walk.h"
  #include <sys/time.h>
  #include <signal.h>
+#include <stdint.h>

  static const char pack_usage[] = "git-pack-objects [-q] [--no-reuse-delta] [--non-empty] [--local] [--incremental] [--window=N] [--depth=N] {--stdout | base-name} < object-list";

diff --git a/sha1_file.c b/sha1_file.c
index 631a605..3372ebc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -13,6 +13,7 @@ #include "blob.h"
  #include "commit.h"
  #include "tag.h"
  #include "tree.h"
+#include <stdint.h>

  #ifndef O_NOATIME
  #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))

-- 
1.3.2.g5f7f2-dirty
Alex Riesen· May 20, 2006, 14:41 UTC · re: Ben Clifford · lore

Re: [PATCH] include header to define uint32_t, necessary on Mac OS X

Ben Clifford, Sun, May 14, 2006 22:58:33 +0200:
Show 21 quoted lines
> From: Ben Clifford <benc@hawaga.org.uk>
> Date: Sun, 14 May 2006 21:34:56 +0100
> Subject: [PATCH] include header to define uint32_t, necessary on Mac OS X
> 
> ---
> 
>   pack-objects.c |    1 +
>   sha1_file.c    |    1 +
>   2 files changed, 2 insertions(+), 0 deletions(-)
> 
> 2ee926ab9da67ef2a6ca28bb70954a33d65ba466
> diff --git a/pack-objects.c b/pack-objects.c
> index 1b9e7a1..5466b15 100644
> --- a/pack-objects.c
> +++ b/pack-objects.c
> @@ -10,6 +10,7 @@ #include "csum-file.h"
>   #include "tree-walk.h"
>   #include <sys/time.h>
>   #include <signal.h>
> +#include <stdint.h>
>

BTW, Ben, how did you produce this patch? It has some unusual indentations...

Linus Torvalds· May 20, 2006, 16:50 UTC · re: Alex Riesen · lore

Re: [PATCH] include header to define uint32_t, necessary on Mac OS X

On Sat, 20 May 2006, Alex Riesen wrote:
Show 8 quoted lines
> Ben Clifford, Sun, May 14, 2006 22:58:33 +0200:
> >   #include <sys/time.h>
> >   #include <signal.h>
> > +#include <stdint.h>
> > 
> 
> BTW, Ben, how did you produce this patch? It has some unusual
> indentations...

I don't think it's the patch producer. It's some mail-mangler. Some broken mailers seem to add an extra space at the beginning of a line that already begins with a space - I've seen this before.

The headers would seem to say "Pine":
	Message-ID: <Pine.LNX.4.64.0605142057220.10680@mundungus.clifford.ac>

but that makes no sense, because mine certainly doesn't. But pine does have a few config options to mangle whitespace, so who knows..

It could also be the actual send path, of course.
		Linus

← back to recent threads