Re: [PATCH 1/3] ewah_read_mmap: bounds-check mmap reads
- From
Jeff King <peff@peff.net>
- Date
- Jun 15, 2018, 17:26 UTC
- Message-ID
- <20180615172656.GB3067@sigill.intra.peff.net>
- In-Reply-To
- <xmqqsh5o0yl5.fsf@gitster-ct.c.googlers.com>
On Fri, Jun 15, 2018 at 10:05:42AM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> > - 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).
Show 14 quoted lines
> > + 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. ;)
Show 6 quoted lines
> > 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