# [PATCH] Fix git-pack-objects for 64-bit platforms 10 messages from 2006-05-11 to 2006-05-20. Participants: Dennis Stosberg, Linus Torvalds, Junio C Hamano, Ben Clifford, Alex Riesen. Thread: https://gitlist.dev/t/4121 ## Dennis Stosberg, 2006-05-11 17:36 Subject: [PATCH] Fix git-pack-objects for 64-bit platforms Message-ID: <20060511173632.G60c08b4@leonov.stosberg.net> URL: https://gitlist.dev/e/20060511173632.G60c08b4%40leonov.stosberg.net ``` 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 --- 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 ``` ## Linus Torvalds, 2006-05-11 17:58 Subject: Re: [PATCH] Fix git-pack-objects for 64-bit platforms Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0605111054290.3866%40g5.osdl.org In-Reply-To: <20060511173632.G60c08b4@leonov.stosberg.net> ``` 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 ``` ## Junio C Hamano, 2006-05-11 18:52 Subject: Re: [PATCH] Fix git-pack-objects for 64-bit platforms Message-ID: <7v7j4swg0r.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v7j4swg0r.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds 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? ``` ## Linus Torvalds, 2006-05-11 19:10 Subject: Re: [PATCH] Fix git-pack-objects for 64-bit platforms Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0605111207200.3866%40g5.osdl.org In-Reply-To: <7v7j4swg0r.fsf@assigned-by-dhcp.cox.net> ``` 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, 2006-05-11 19:27 Subject: Re: [PATCH] Fix git-pack-objects for 64-bit platforms Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0605111218380.3866%40g5.osdl.org In-Reply-To: <7v7j4swg0r.fsf@assigned-by-dhcp.cox.net> ``` 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 ``` ## Junio C Hamano, 2006-05-13 05:58 Subject: Re: [PATCH] Fix git-pack-objects for 64-bit platforms Message-ID: <7viroav534.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7viroav534.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds 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; ``` ## Ben Clifford, 2006-05-14 20:56 Subject: Re: [PATCH] Fix git-pack-objects for 64-bit platforms Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0605142056320.3038%40mundungus.clifford.ac In-Reply-To: <7viroav534.fsf@assigned-by-dhcp.cox.net> ``` > 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 seems to make it work again. See following patch. -- ``` ## Ben Clifford, 2006-05-14 20:58 Subject: [PATCH] include header to define uint32_t, necessary on Mac OS X Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0605142057220.10680%40mundungus.clifford.ac In-Reply-To: ``` From: Ben Clifford 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 #include +#include 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 #ifndef O_NOATIME #if defined(__linux__) && (defined(__i386__) || defined(__PPC__)) -- 1.3.2.g5f7f2-dirty ``` ## Alex Riesen, 2006-05-20 14:41 Subject: Re: [PATCH] include header to define uint32_t, necessary on Mac OS X Message-ID: <20060520144111.GA5798@steel.home> URL: https://gitlist.dev/e/20060520144111.GA5798%40steel.home In-Reply-To: ``` Ben Clifford, Sun, May 14, 2006 22:58:33 +0200: > From: Ben Clifford > 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 > #include > +#include > BTW, Ben, how did you produce this patch? It has some unusual indentations... ``` ## Linus Torvalds, 2006-05-20 16:50 Subject: Re: [PATCH] include header to define uint32_t, necessary on Mac OS X Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0605200943270.10823%40g5.osdl.org In-Reply-To: <20060520144111.GA5798@steel.home> ``` On Sat, 20 May 2006, Alex Riesen wrote: > Ben Clifford, Sun, May 14, 2006 22:58:33 +0200: > > #include > > #include > > +#include > > > > 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: 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 ```