{"thread":{"id":"36476","subject":"[PATCH] pack-bitmap: do not core dump","startedAt":"2014-04-22T22:53:02Z","lastAt":"2014-04-23T00:40:23Z","messageCount":4,"participants":["Kyle J. McKay","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"239399","messageId":"781073ea08e86301f62e708bc0b80fd@74d39fa044aa309eaea14b9f57fe79c","threadId":"36476","inReplyTo":null,"subject":"[PATCH] pack-bitmap: do not core dump","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-22T22:53:02Z","receivedAt":"2014-04-22T22:53:02Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"So I was trying to use pack.writebitmaps=true and all I got was core dumps.\n\nThe fix with a real subject line ;) is below.  I think perhaps this should be\npicked up for the 2.0.0 release.  (Patch is against master.)\n\n--Kyle\n\n---- >8 ----\nSubject: [PATCH] ewah_bitmap.c: do not assume size_t and eword_t are the same size\n\nWhen buffer_grow changes the size of the buffer using realloc,\nit first computes and saves the rlw pointer's offset into the\nbuffer using (uint8_t *) math before the realloc but then\nrestores it using (eword_t *) math.\n\nIn order to do this it's necessary to convert the (uint8_t *)\noffset into an (eword_t *) offset.  It was doing this by\ndividing by the sizeof(size_t).  Unfortunately sizeof(size_t)\nis not same as sizeof(eword_t) on all platforms.\n\nThis causes illegal memory accesses and other bad things to\nhappen when attempting to use bitmaps on those platforms.\n\nFix this by dividing by the sizeof(eword_t) instead which\nwill always be correct for all platforms.\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n---\n ewah/ewah_bitmap.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c\nindex 9ced2dad..fccb42b5 100644\n--- a/ewah/ewah_bitmap.c\n+++ b/ewah/ewah_bitmap.c\n@@ -41,7 +41,7 @@ static inline void buffer_grow(struct ewah_bitmap *self, size_t new_size)\n \tself->alloc_size = new_size;\n \tself->buffer = ewah_realloc(self->buffer,\n \t\tself->alloc_size * sizeof(eword_t));\n-\tself->rlw = self->buffer + (rlw_offset / sizeof(size_t));\n+\tself->rlw = self->buffer + (rlw_offset / sizeof(eword_t));\n }\n \n static inline void buffer_push(struct ewah_bitmap *self, eword_t value)\n-- \n1.8.5\n"},{"id":"239400","messageId":"xmqq7g6h2c4f.fsf@gitster.dls.corp.google.com","threadId":"36476","inReplyTo":"781073ea08e86301f62e708bc0b80fd@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] pack-bitmap: do not core dump","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-22T23:11:28Z","receivedAt":"2014-04-22T23:11:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> writes:\n\n> So I was trying to use pack.writebitmaps=true and all I got was core dumps.\n>\n> The fix with a real subject line ;) is below.  I think perhaps this should be\n> picked up for the 2.0.0 release.  (Patch is against master.)\n\nOf course---a breakage in a new code may be less important than a\nregression fix in the sense that there is an option not to use the\nnew feature, but an obvious fix to such a breakage is certainly very\nmuch welcomed during the -rc period.\n\nThanks.  I'll try not to forget until tomorrow's integration cycle\n(I just finished and pushed the results out for today), which will\nalso give me a chance to wait for Peff's Ack ;-).\n\n> --Kyle\n>\n> ---- >8 ----\n> Subject: [PATCH] ewah_bitmap.c: do not assume size_t and eword_t are the same size\n>\n> When buffer_grow changes the size of the buffer using realloc,\n> it first computes and saves the rlw pointer's offset into the\n> buffer using (uint8_t *) math before the realloc but then\n> restores it using (eword_t *) math.\n>\n> In order to do this it's necessary to convert the (uint8_t *)\n> offset into an (eword_t *) offset.  It was doing this by\n> dividing by the sizeof(size_t).  Unfortunately sizeof(size_t)\n> is not same as sizeof(eword_t) on all platforms.\n>\n> This causes illegal memory accesses and other bad things to\n> happen when attempting to use bitmaps on those platforms.\n>\n> Fix this by dividing by the sizeof(eword_t) instead which\n> will always be correct for all platforms.\n>\n> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>\n> ---\n>  ewah/ewah_bitmap.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c\n> index 9ced2dad..fccb42b5 100644\n> --- a/ewah/ewah_bitmap.c\n> +++ b/ewah/ewah_bitmap.c\n> @@ -41,7 +41,7 @@ static inline void buffer_grow(struct ewah_bitmap *self, size_t new_size)\n>  \tself->alloc_size = new_size;\n>  \tself->buffer = ewah_realloc(self->buffer,\n>  \t\tself->alloc_size * sizeof(eword_t));\n> -\tself->rlw = self->buffer + (rlw_offset / sizeof(size_t));\n> +\tself->rlw = self->buffer + (rlw_offset / sizeof(eword_t));\n>  }\n>  \n>  static inline void buffer_push(struct ewah_bitmap *self, eword_t value)\n"},{"id":"239402","messageId":"20140422231748.GA20353@sigill.intra.peff.net","threadId":"36476","inReplyTo":"781073ea08e86301f62e708bc0b80fd@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] pack-bitmap: do not core dump","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-22T23:17:48Z","receivedAt":"2014-04-22T23:17:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 22, 2014 at 03:53:02PM -0700, Kyle J. McKay wrote:\n\n> So I was trying to use pack.writebitmaps=true and all I got was core dumps.\n\nEek.\n\n> The fix with a real subject line ;) is below.  I think perhaps this should be\n> picked up for the 2.0.0 release.  (Patch is against master.)\n\nYes, this is definitely the sort of bugfix we want to see during the -rc\nperiod (well, we would prefer not to see bugs at all, but if we must\nhave them, fixes are helpful).\n\n> ---- >8 ----\n> Subject: [PATCH] ewah_bitmap.c: do not assume size_t and eword_t are the same size\n\nThanks for a very well-written commit message. I think your fix makes\nsense:\n\n> -\tself->rlw = self->buffer + (rlw_offset / sizeof(size_t));\n> +\tself->rlw = self->buffer + (rlw_offset / sizeof(eword_t));\n\nWe could also write it as:\n\n  self->rlw = (uint8_t *)self->buffer + rlw_offset;\n\nbut I do not think that is necessarily any more readable, especially\nbecause we probably need to cast it like:\n\n  self->rlw = (eword_t *)((uint8_t *)self->buffer + rlw_offset);\n\nGiven that self->rlw is a pointer to eword_t, though, we can assume\nrlw_offset is always going to be a multiple of sizeof(eword_t) anyway\n(and if it is not, the division in the original is a big problem, but I\ndo not think that is the case).  So why do any uint8_t math in the first\nplace? I think we could write it as:\n\n\teword_t *old = self->buffer;\n\t... realloc ...\n\tself->rlw = self->buffer + (self->rlw - old);\n\nI'm fine with your patch, though.\n\nI also poked through the rest of the bitmap code looking for similar\nproblems, but didn't find any. I do not think this was a systemic issue\nwith bad use of types; it was just a think-o that happened to work on\n64-bit machines.\n\n-Peff\n"},{"id":"239404","messageId":"CC07C833-D42A-488D-8523-46445F8C1C20@gmail.com","threadId":"36476","inReplyTo":"20140422231748.GA20353@sigill.intra.peff.net","subject":"Re: [PATCH] pack-bitmap: do not core dump","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-23T00:40:23Z","receivedAt":"2014-04-23T00:40:23Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 22, 2014, at 16:17, Jeff King wrote:\n> but I do not think that is necessarily any more readable, especially\n> because we probably need to cast it like:\n>\n>  self->rlw = (eword_t *)((uint8_t *)self->buffer + rlw_offset);\n\nI suspect that will produce a warning about a cast increasing pointer  \nalignment in some cases.\n\n> So why do any uint8_t math in the first place?\n\nJust guessing it started out as uint8_t math throughout then the  \nwarning about increasing alignment showed up so that part got changed  \nbut the save pointer offset part did not.\n\n> I think we could write it as:\n>\n> \teword_t *old = self->buffer;\n> \t... realloc ...\n> \tself->rlw = self->buffer + (self->rlw - old);\n\nYeah that seems good too.\n\n> I'm fine with your patch, though.\n\n\nthanks.\n\nI tend to gravitate towards the most minimal change that fixes the  \nissue when touching someone else's code especially if I'm not familiar  \nwith it.  There's also the minor issue of optimizing out the pointer  \narithmetic implicit divide by sizeof(eword_t) for the subtract and the  \nimplicit multiply by sizeof(eword_t) for the add -- I didn't check to  \nsee if one of the alternatives is best -- the one you mention above  \nwith the cast (keeping the uint8_t math) is probably guaranteed not to  \nhave any implicit multiply, but there's that pesky warning to get rid  \nof.  Of course those multiplies and divides should just end up being  \nshifts so not a big deal if they didn't get optimized out (the realloc  \ntime should dwarf them into irrelevancy anyway).\n\n--Kyle\n"}]}