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

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

[Dennis Stosberg]

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
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

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

[Linus Torvalds]

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.

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

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

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> 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.

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

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

[Linus Torvalds]

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

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

[Linus Torvalds]

On Thu, 11 May 2006, Junio C Hamano wrote:
> 
> 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

---
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

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

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> 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 --

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;

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

[Ben Clifford]

> 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.

-- 

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

[Ben Clifford]

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>

  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

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

[Alex Riesen]

Ben Clifford, Sun, May 14, 2006 22:58:33 +0200:
> 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...

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

[Linus Torvalds]

On Sat, 20 May 2006, Alex Riesen wrote:

> 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