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.


> 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

> -     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.

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

Reply via email to