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

From: Jeff King <peff@peff.net>
Date: Apr 22, 2014, 23:17 UTC
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