{"thread":{"id":"4121","subject":"[PATCH] Fix git-pack-objects for 64-bit platforms","startedAt":"2006-05-11T17:36:32Z","lastAt":"2006-05-20T16:50:24Z","messageCount":10,"participants":["Dennis Stosberg","Linus Torvalds","Junio C Hamano","Ben Clifford","Alex Riesen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"19854","messageId":"20060511173632.G60c08b4@leonov.stosberg.net","threadId":"4121","inReplyTo":null,"subject":"[PATCH] Fix git-pack-objects for 64-bit platforms","fromName":"Dennis Stosberg","fromEmail":"dennis@stosberg.net","sentAt":"2006-05-11T17:36:32Z","receivedAt":"2006-05-11T17:36:32Z","isPatch":true,"sender":{"key":"dennis@stosberg.net","avatar":null},"body":"The offset of an object in the pack is recorded as a 4-byte integer\nin the index file. When reading the offset from the mmap'ed index\nin prepare_pack_revindex(), the address is dereferenced as a long*.\nThis works fine as long as the long type is four bytes wide. On\nNetBSD/sparc64, however, a long is 8 bytes wide and so dereferencing\nthe offset produces garbage.\n\nSigned-off-by: Dennis Stosberg \n\n---\n\nI am not sure whether an int cast or an int32_t cast is more\nappropriate here. An int is not guaranteed to be four bytes wide,\nbut I don't know of any modern platform where that's not the case.\nOn the other hand int32_t is not necessarily available before C99.\n\nAny opinions? I wonder why no one has hit this on x86_64...\n\n\n pack-objects.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\n026b1b2cdd5332f59e15cd8611a49ead3094d08c\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 523a1c7..29bda43 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -156,7 +156,7 @@ static void prepare_pack_revindex(struct\n \n \trix->revindex = xmalloc(sizeof(unsigned long) * (num_ent + 1));\n \tfor (i = 0; i < num_ent; i++) {\n-\t\tlong hl = *((long *)(index + 24 * i));\n+\t\tlong hl = *((int *)(index + 24 * i));\n \t\trix->revindex[i] = ntohl(hl);\n \t}\n \t/* This knows the pack format -- the 20-byte trailer\n-- \n1.3.2.gbe65\n"},{"id":"19855","messageId":"Pine.LNX.4.64.0605111054290.3866@g5.osdl.org","threadId":"4121","inReplyTo":"20060511173632.G60c08b4@leonov.stosberg.net","subject":"Re: [PATCH] Fix git-pack-objects for 64-bit platforms","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-11T17:58:32Z","receivedAt":"2006-05-11T17:58:32Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 May 2006, Dennis Stosberg wrote:\n> \n> I am not sure whether an int cast or an int32_t cast is more\n> appropriate here. An int is not guaranteed to be four bytes wide,\n> but I don't know of any modern platform where that's not the case.\n> On the other hand int32_t is not necessarily available before C99.\n> \n> Any opinions? I wonder why no one has hit this on x86_64...\n\nI think the \"ntohl()\" hides it. It loads a 64-bit value, but since x86-64 \nis little-endian, the low 32 bits are correct. The htonl() will then strip \nthe high bits and make it all be big-endian.\n\nAnd while I actually run a 64-bit big-endian machine myself (G5 ppc64), my \nuser space is all 32-bit by default, so it never showed up on linux-ppc64 \neither.\n\nAnyway, the correct type to use is \"uint32_t\" in this case. That's what \nhtonl() takes.\n\n\t\tLinus\n"},{"id":"19856","messageId":"7v7j4swg0r.fsf@assigned-by-dhcp.cox.net","threadId":"4121","inReplyTo":"Pine.LNX.4.64.0605111054290.3866@g5.osdl.org","subject":"Re: [PATCH] Fix git-pack-objects for 64-bit platforms","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-11T18:52:20Z","receivedAt":"2006-05-11T18:52:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, 11 May 2006, Dennis Stosberg wrote:\n>> \n>> I am not sure whether an int cast or an int32_t cast is more\n>> appropriate here. An int is not guaranteed to be four bytes wide,\n>> but I don't know of any modern platform where that's not the case.\n>> On the other hand int32_t is not necessarily available before C99.\n>> \n>> Any opinions? I wonder why no one has hit this on x86_64...\n>\n> I think the \"ntohl()\" hides it. It loads a 64-bit value, but since x86-64 \n> is little-endian, the low 32 bits are correct. The htonl() will then strip \n> the high bits and make it all be big-endian.\n\nThat sounds sensible.\n\nSince I saw a patch that touches only one place, I thought I'd\nbetter point this out...\n\nThere are a few more places that knows about this\n((char*)base_pointer + (entry_count * 24)) magic in our code.\n\n$ git grep -n -e '24 *\\*' -e '\\* *24' master -- '*.c'\n\nmaster:pack-objects.c:159:\t\tlong hl = *((long *)(index + 24 * i));\n\n\tThis is yours.\n\nmaster:sha1_file.c:447:\tif (idx_size != 4*256 + nr * 24 + 20 + 20)\nmaster:sha1_file.c:1148:\tmemcpy(sha1, (index + 24 * n + 4), 20);\nmaster:sha1_file.c:1162:\t\tint cmp = memcmp(index + 24 * mi + 4, sha1, 20);\n\n\tThese three should be OK, I think.\n\nmaster:sha1_file.c:1164:\t\t\te->offset = ntohl(*((int*)(index + 24 * mi)));\n\n\tThis you might want to look at; I suspect it is what\n\tLinus suggests.\n\nAlso we _might_ have uglier magic that assumes the base_pointer to\nbe a pointer to a 4-byte integer and uses offset of multiple of\n6 instead of 24, although I do not think it is likely.\n\nI have to leave the keyboard in a few minutes so I cannot verify\nnor fix them myself for the next 8 hours or so. Sorry.\n\n> And while I actually run a 64-bit big-endian machine myself (G5 ppc64), my \n> user space is all 32-bit by default, so it never showed up on linux-ppc64 \n> either.\n>\n> Anyway, the correct type to use is \"uint32_t\" in this case. That's what \n> htonl() takes.\n\nIs uint32_t guaranteed to be exactly 32-bit, or merely enough to\nhold 32-bit?\n"},{"id":"19857","messageId":"Pine.LNX.4.64.0605111207200.3866@g5.osdl.org","threadId":"4121","inReplyTo":"7v7j4swg0r.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix git-pack-objects for 64-bit platforms","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-11T19:10:59Z","receivedAt":"2006-05-11T19:10:59Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 May 2006, Junio C Hamano wrote:\n> \n> Is uint32_t guaranteed to be exactly 32-bit, or merely enough to\n> hold 32-bit?\n\nI think it's guaranteed to be 32-bit, but regardless, the current git \nheaders already assume that \"unsigned int\" is 32-bit.\n\nWhich is a pretty safe assumption for at least the next ten years or so, \npossibly much longer. So I don't think we need to worry _too_ much about \nthis. I think it's more important to try to get git working on Windows, \nthan on 16-bit DOS or on a PDP-9, or one of the odd cray machines.\n\n\t\tLinus\n"},{"id":"19858","messageId":"Pine.LNX.4.64.0605111218380.3866@g5.osdl.org","threadId":"4121","inReplyTo":"7v7j4swg0r.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix git-pack-objects for 64-bit platforms","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-11T19:27:48Z","receivedAt":"2006-05-11T19:27:48Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 11 May 2006, Junio C Hamano wrote:\n> \n> Since I saw a patch that touches only one place, I thought I'd\n> better point this out...\n> \n> There are a few more places that knows about this\n> ((char*)base_pointer + (entry_count * 24)) magic in our code.\n\nSince I _do_ have a 64-bit big-endian architecture, I decided to install \nsome of the 64-bit development libraries that I didn't already have, and I \nadded \"-m64\" to the compiler flags.\n\nAll the tests seem to pass with the simple fix (and without it, we do \nindeed fail at least t5700-clone-reference.sh).\n\nOf course, there might well be something else that doesn't get coverage, \nbut passing all tests is at least a good sign. And since x86-64 has been \ngetting pretty extensive coverage, I don't think we have a lot of 64-bit \nbugs per se, this one just happened to hide.\n\n\t\tLinus\n\n---\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 523a1c7..1b9e7a1 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -156,7 +156,7 @@ static void prepare_pack_revindex(struct\n \n \trix->revindex = xmalloc(sizeof(unsigned long) * (num_ent + 1));\n \tfor (i = 0; i < num_ent; i++) {\n-\t\tlong hl = *((long *)(index + 24 * i));\n+\t\tuint32_t hl = *((uint32_t *)(index + 24 * i));\n \t\trix->revindex[i] = ntohl(hl);\n \t}\n \t/* This knows the pack format -- the 20-byte trailer\n"},{"id":"19873","messageId":"7viroav534.fsf@assigned-by-dhcp.cox.net","threadId":"4121","inReplyTo":"Pine.LNX.4.64.0605111218380.3866@g5.osdl.org","subject":"Re: [PATCH] Fix git-pack-objects for 64-bit platforms","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-05-13T05:58:23Z","receivedAt":"2006-05-13T05:58:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Since I _do_ have a 64-bit big-endian architecture, I decided to install \n> some of the 64-bit development libraries that I didn't already have, and I \n> added \"-m64\" to the compiler flags.\n>\n> All the tests seem to pass with the simple fix (and without it, we do \n> indeed fail at least t5700-clone-reference.sh).\n>\n> Of course, there might well be something else that doesn't get coverage, \n> but passing all tests is at least a good sign. And since x86-64 has been \n> getting pretty extensive coverage, I don't think we have a lot of 64-bit \n> bugs per se, this one just happened to hide.\n>\n> \t\tLinus\n\nThanks, both. This is what I am thinking of applying (but I am\nsort of burned-out right now after a two-day meeting with my\nsponsors, so the real work will be tomorrow).\n\nIt takes the uint32_t version but matches another place to use\nthat type instead of \"int\" (which would not matter in next 10\nyears perhaps).\n\n-- >8 --\n\ndiff --git a/pack-objects.c b/pack-objects.c\nindex c0acc46..a81d609 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -156,7 +156,7 @@ static void prepare_pack_revindex(struct\n \n \trix->revindex = xmalloc(sizeof(unsigned long) * (num_ent + 1));\n \tfor (i = 0; i < num_ent; i++) {\n-\t\tlong hl = *((long *)(index + 24 * i));\n+\t\tuint32_t hl = *((uint32_t *)(index + 24 * i));\n \t\trix->revindex[i] = ntohl(hl);\n \t}\n \t/* This knows the pack format -- the 20-byte trailer\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f2d33af..642c45a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1126,7 +1126,7 @@ int find_pack_entry_one(const unsigned c\n \t\tint mi = (lo + hi) / 2;\n \t\tint cmp = memcmp(index + 24 * mi + 4, sha1, 20);\n \t\tif (!cmp) {\n-\t\t\te->offset = ntohl(*((int*)(index + 24 * mi)));\n+\t\t\te->offset = ntohl(*((uint32_t *)(index + 24 * mi)));\n \t\t\tmemcpy(e->sha1, sha1, 20);\n \t\t\te->p = p;\n \t\t\treturn 1;\n"},{"id":"19927","messageId":"Pine.LNX.4.64.0605142056320.3038@mundungus.clifford.ac","threadId":"4121","inReplyTo":"7viroav534.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix git-pack-objects for 64-bit platforms","fromName":"Ben Clifford","fromEmail":"benc@hawaga.org.uk","sentAt":"2006-05-14T20:56:49Z","receivedAt":"2006-05-14T20:56:49Z","isPatch":true,"sender":{"key":"benc@hawaga.org.uk","avatar":"https://gravatar.com/avatar/c7ce083471287f8e77b69dd147f757799d1efdd740727fd7e3003f33e88be898?d=mp&s=160"},"body":"\n> It takes the uint32_t version but matches another place to use\n> that type instead of \"int\" (which would not matter in next 10\n> years perhaps).\n\nThis use of uint32_t breaks the build for me on Mac OS X. Including\n seems to make it work again. See following patch.\n\n-- \n"},{"id":"19928","messageId":"Pine.LNX.4.64.0605142057220.10680@mundungus.clifford.ac","threadId":"4121","inReplyTo":"Pine.LNX.4.64.0605142056320.3038@mundungus.clifford.ac","subject":"[PATCH] include header to define uint32_t, necessary on Mac OS X","fromName":"Ben Clifford","fromEmail":"benc@hawaga.org.uk","sentAt":"2006-05-14T20:58:33Z","receivedAt":"2006-05-14T20:58:33Z","isPatch":true,"sender":{"key":"benc@hawaga.org.uk","avatar":"https://gravatar.com/avatar/c7ce083471287f8e77b69dd147f757799d1efdd740727fd7e3003f33e88be898?d=mp&s=160"},"body":"\nFrom: Ben Clifford \nDate: Sun, 14 May 2006 21:34:56 +0100\nSubject: [PATCH] include header to define uint32_t, necessary on Mac OS X\n\n---\n\n pack-objects.c | 1 +\n sha1_file.c | 1 +\n 2 files changed, 2 insertions(+), 0 deletions(-)\n\n2ee926ab9da67ef2a6ca28bb70954a33d65ba466\ndiff --git a/pack-objects.c b/pack-objects.c\nindex 1b9e7a1..5466b15 100644\n--- a/pack-objects.c\n+++ b/pack-objects.c\n@@ -10,6 +10,7 @@ #include \"csum-file.h\"\n #include \"tree-walk.h\"\n #include \n #include \n+#include \n\n 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\";\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 631a605..3372ebc 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -13,6 +13,7 @@ #include \"blob.h\"\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"tree.h\"\n+#include \n\n #ifndef O_NOATIME\n #if defined(__linux__) && (defined(__i386__) || defined(__PPC__))\n-- \n1.3.2.g5f7f2-dirty\n"},{"id":"20325","messageId":"20060520144111.GA5798@steel.home","threadId":"4121","inReplyTo":"Pine.LNX.4.64.0605142057220.10680@mundungus.clifford.ac","subject":"Re: [PATCH] include header to define uint32_t, necessary on Mac OS X","fromName":"Alex Riesen","fromEmail":"fork0@t-online.de","sentAt":"2006-05-20T14:41:11Z","receivedAt":"2006-05-20T14:41:11Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Ben Clifford, Sun, May 14, 2006 22:58:33 +0200:\n> From: Ben Clifford \n> Date: Sun, 14 May 2006 21:34:56 +0100\n> Subject: [PATCH] include header to define uint32_t, necessary on Mac OS X\n> \n> ---\n> \n> pack-objects.c | 1 +\n> sha1_file.c | 1 +\n> 2 files changed, 2 insertions(+), 0 deletions(-)\n> \n> 2ee926ab9da67ef2a6ca28bb70954a33d65ba466\n> diff --git a/pack-objects.c b/pack-objects.c\n> index 1b9e7a1..5466b15 100644\n> --- a/pack-objects.c\n> +++ b/pack-objects.c\n> @@ -10,6 +10,7 @@ #include \"csum-file.h\"\n> #include \"tree-walk.h\"\n> #include \n> #include \n> +#include \n> \n\nBTW, Ben, how did you produce this patch? It has some unusual\nindentations...\n"},{"id":"20327","messageId":"Pine.LNX.4.64.0605200943270.10823@g5.osdl.org","threadId":"4121","inReplyTo":"20060520144111.GA5798@steel.home","subject":"Re: [PATCH] include header to define uint32_t, necessary on Mac OS X","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-05-20T16:50:24Z","receivedAt":"2006-05-20T16:50:24Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 20 May 2006, Alex Riesen wrote:\n\n> Ben Clifford, Sun, May 14, 2006 22:58:33 +0200:\n> > #include \n> > #include \n> > +#include \n> > \n> \n> BTW, Ben, how did you produce this patch? It has some unusual\n> indentations...\n\nI don't think it's the patch producer. It's some mail-mangler. Some broken \nmailers seem to add an extra space at the beginning of a line that already \nbegins with a space - I've seen this before.\n\nThe headers would seem to say \"Pine\":\n\n\tMessage-ID: \n\nbut that makes no sense, because mine certainly doesn't. But pine does \nhave a few config options to mangle whitespace, so who knows..\n\nIt could also be the actual send path, of course.\n\n\t\tLinus\n"}]}