Re: [PATCH 1/3] ewah_read_mmap: bounds-check mmap reads

[Jeff King <peff@peff.net>]

On Fri, Jun 15, 2018 at 10:05:42AM -0700, Junio C Hamano wrote:

> > -	memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t));
> > -	ptr += self->buffer_size * sizeof(eword_t);
> 
> 
> > +	data_len = st_mult(self->buffer_size, sizeof(eword_t));
> 
> This is a faithful conversion from the original, but I somehow would
> have appreciated if the latter were not sizeof(eword_t) but rather
> sizeof(self->buffer_size[0]), especially as I wondered ...

I actually thought about going the _other_ way. The sizeof(eword_t) is
not something we can change, but is actually decided by the on-disk
format.  So I wondered if this should be much more clearly "hey, this is
8 bytes". Possibly with an assert(sizeof(*self->buffer_size) == 8).

And yes, I think having the on-disk format specify the size in 8-byte
double words is vaguely crazy. Blame JGit. ;) Or maybe even blame the
original EWAH authors, this may have originated even earlier (I didn't
dig).

> > +	if (len < data_len)
> > +		return error("corrupt ewah bitmap: eof in data "
> > +			     "(%"PRIuMAX" bytes short)",
> > +			     (uintmax_t)(data_len - len));
> > +	memcpy(self->buffer, ptr, data_len);
> > +	ptr += data_len;
> > +	len -= data_len;
> >  
> >  	for (i = 0; i < self->buffer_size; ++i)
> >  		self->buffer[i] = ntohll(self->buffer[i]);
> 
> ... what individual datum one iteration of this loop is copying, and
> then realized "buffer_size" is a misleading field name (anything that 
> claims to be size and not measuring in bytes is misleading to me ;-).

Yeah, it confused me at first, too. I don't mind changing these kinds of
cosmetics, but I'd like to do it in a separate patch from this fix.

> > -	return (3 * 4) + (self->buffer_size * 8);
> > +	return ptr - (const uint8_t *)map;
> 
> Much nicer; I needed to wonder what these 12 and 8 in the original are.

Me too. ;)

> >  int ewah_deserialize(struct ewah_bitmap *self, int fd);
> > -int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len);
> > +ssize_t ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len);
> 
> I double checked all the callers and made sure that they are already
> prepared to react sensibly to error returns, which is good.

Yep, modulo the int/ssize_t thing from the fourth patch.

-Peff