[PATCH] pack-bitmap: do not core dump

[PATCH] pack-bitmap: do not core dump

[Kyle J. McKay]

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

Re: [PATCH] pack-bitmap: do not core dump

[Junio C Hamano]

"Kyle J. McKay" <mackyle@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 <mackyle@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)

Re: [PATCH] pack-bitmap: do not core dump

[Jeff King]

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

Re: [PATCH] pack-bitmap: do not core dump

[Kyle J. McKay]

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