[PATCH] pack-bitmap: do not core dump
So I was trying to use pack.writebitmaps=true and all I got was core dumps. The fix with a real subject line ;) is below. I think perhaps this should be picked up for the 2.0.0 release. (Patch is against master.) --Kyle 8 Subject: [PATCH] ewah_bitmap.c: do not assume size_t and eword_t are the same size When buffer_grow changes the size of the buffer using realloc, it first computes and saves the rlw pointer's offset into the buffer using (uint8_t *) math before the realloc but then restores it using (eword_t *) math. In order to do this it's necessary to convert the (uint8_t *) offset into an (eword_t *) offset. It was doing this by dividing by the sizeof(size_t). Unfortunately sizeof(size_t) is not same as sizeof(eword_t) on all platforms. This causes illegal memory accesses and other bad things to happen when attempting to use bitmaps on those platforms. Fix this by dividing by the sizeof(eword_t) instead which will always be correct for all platforms. Signed-off-by: Kyle J. McKay mack...@gmail.com --- ewah/ewah_bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c index 9ced2dad..fccb42b5 100644 --- a/ewah/ewah_bitmap.c +++ b/ewah/ewah_bitmap.c @@ -41,7 +41,7 @@ static inline void buffer_grow(struct ewah_bitmap *self, size_t new_size) self-alloc_size = new_size; self-buffer = ewah_realloc(self-buffer, self-alloc_size * sizeof(eword_t)); - self-rlw = self-buffer + (rlw_offset / sizeof(size_t)); + self-rlw = self-buffer + (rlw_offset / sizeof(eword_t)); } static inline void buffer_push(struct ewah_bitmap *self, eword_t value) -- 1.8.5 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pack-bitmap: do not core dump
Kyle J. McKay mack...@gmail.com writes: So I was trying to use pack.writebitmaps=true and all I got was core dumps. The fix with a real subject line ;) is below. I think perhaps this should be picked up for the 2.0.0 release. (Patch is against master.) Of course---a breakage in a new code may be less important than a regression fix in the sense that there is an option not to use the new feature, but an obvious fix to such a breakage is certainly very much welcomed during the -rc period. Thanks. I'll try not to forget until tomorrow's integration cycle (I just finished and pushed the results out for today), which will also give me a chance to wait for Peff's Ack ;-). --Kyle 8 Subject: [PATCH] ewah_bitmap.c: do not assume size_t and eword_t are the same size When buffer_grow changes the size of the buffer using realloc, it first computes and saves the rlw pointer's offset into the buffer using (uint8_t *) math before the realloc but then restores it using (eword_t *) math. In order to do this it's necessary to convert the (uint8_t *) offset into an (eword_t *) offset. It was doing this by dividing by the sizeof(size_t). Unfortunately sizeof(size_t) is not same as sizeof(eword_t) on all platforms. This causes illegal memory accesses and other bad things to happen when attempting to use bitmaps on those platforms. Fix this by dividing by the sizeof(eword_t) instead which will always be correct for all platforms. Signed-off-by: Kyle J. McKay mack...@gmail.com --- ewah/ewah_bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c index 9ced2dad..fccb42b5 100644 --- a/ewah/ewah_bitmap.c +++ b/ewah/ewah_bitmap.c @@ -41,7 +41,7 @@ static inline void buffer_grow(struct ewah_bitmap *self, size_t new_size) self-alloc_size = new_size; self-buffer = ewah_realloc(self-buffer, self-alloc_size * sizeof(eword_t)); - self-rlw = self-buffer + (rlw_offset / sizeof(size_t)); + self-rlw = self-buffer + (rlw_offset / sizeof(eword_t)); } static inline void buffer_push(struct ewah_bitmap *self, eword_t value) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pack-bitmap: do not core dump
On Tue, Apr 22, 2014 at 03:53:02PM -0700, Kyle J. McKay wrote: So I was trying to use pack.writebitmaps=true and all I got was core dumps. Eek. The fix with a real subject line ;) is below. I think perhaps this should be picked up for the 2.0.0 release. (Patch is against master.) Yes, this is definitely the sort of bugfix we want to see during the -rc period (well, we would prefer not to see bugs at all, but if we must have them, fixes are helpful). 8 Subject: [PATCH] ewah_bitmap.c: do not assume size_t and eword_t are the same size Thanks for a very well-written commit message. I think your fix makes sense: - self-rlw = self-buffer + (rlw_offset / sizeof(size_t)); + self-rlw = self-buffer + (rlw_offset / sizeof(eword_t)); We could also write it as: self-rlw = (uint8_t *)self-buffer + rlw_offset; but I do not think that is necessarily any more readable, especially because we probably need to cast it like: self-rlw = (eword_t *)((uint8_t *)self-buffer + rlw_offset); Given that self-rlw is a pointer to eword_t, though, we can assume rlw_offset is always going to be a multiple of sizeof(eword_t) anyway (and if it is not, the division in the original is a big problem, but I do not think that is the case). So why do any uint8_t math in the first place? I think we could write it as: eword_t *old = self-buffer; ... realloc ... self-rlw = self-buffer + (self-rlw - old); I'm fine with your patch, though. I also poked through the rest of the bitmap code looking for similar problems, but didn't find any. I do not think this was a systemic issue with bad use of types; it was just a think-o that happened to work on 64-bit machines. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pack-bitmap: do not core dump
On Apr 22, 2014, at 16:17, Jeff King wrote: but I do not think that is necessarily any more readable, especially because we probably need to cast it like: self-rlw = (eword_t *)((uint8_t *)self-buffer + rlw_offset); I suspect that will produce a warning about a cast increasing pointer alignment in some cases. So why do any uint8_t math in the first place? Just guessing it started out as uint8_t math throughout then the warning about increasing alignment showed up so that part got changed but the save pointer offset part did not. I think we could write it as: eword_t *old = self-buffer; ... realloc ... self-rlw = self-buffer + (self-rlw - old); Yeah that seems good too. I'm fine with your patch, though. thanks. I tend to gravitate towards the most minimal change that fixes the issue when touching someone else's code especially if I'm not familiar with it. There's also the minor issue of optimizing out the pointer arithmetic implicit divide by sizeof(eword_t) for the subtract and the implicit multiply by sizeof(eword_t) for the add -- I didn't check to see if one of the alternatives is best -- the one you mention above with the cast (keeping the uint8_t math) is probably guaranteed not to have any implicit multiply, but there's that pesky warning to get rid of. Of course those multiplies and divides should just end up being shifts so not a big deal if they didn't get optimized out (the realloc time should dwarf them into irrelevancy anyway). --Kyle -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
