From: Jeff King <peff@peff.net>
Date: Tue, 22 Apr 2014 23:17:48 GMT
Subject: Re: [PATCH] pack-bitmap: do not core dump
Message-ID: <20140422231748.GA20353@sigill.intra.peff.net>
In-Reply-To: <781073ea08e86301f62e708bc0b80fd@74d39fa044aa309eaea14b9f57fe79c>

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