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

On Sat, Jun 16, 2018 at 04:35:13PM +0200, SZEDER Gábor wrote: > > + head -c 512 <$bitmap >$bitmap.tmp && > > + mv $bitmap.tmp $bitmap && > > This line turns out to be problematic on OSX and ultimately causes the > test to fail. > > When OSX's 'mv's destination is read-only, it asks whether

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

> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh > index 423c0a475f..237ee6e5fc 100755 > --- a/t/t5310-pack-bitmaps.sh > +++ b/t/t5310-pack-bitmaps.sh > @@ -331,4 +331,17 @@ test_expect_success 'pack reuse respects --incremental' ' > git show-index actual && > test_cmp

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

Jeff King writes: > 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

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

Jeff King writes: > On Fri, Jun 15, 2018 at 07:10:28PM +0200, SZEDER Gábor wrote: > >> > I said "yuck" because the original does not work if there happen to >> > be more than (or for that matter, less than) one '.bitmap' file >> > there. But at least as long as there is one, it should work ;-)

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

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

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

On Fri, Jun 15, 2018 at 07:10:28PM +0200, SZEDER Gábor wrote: > > I said "yuck" because the original does not work if there happen to > > be more than (or for that matter, less than) one '.bitmap' file > > there. But at least as long as there is one, it should work ;-) > > Well, the test starts

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

On Fri, Jun 15, 2018 at 6:20 PM, Junio C Hamano wrote: > SZEDER Gábor writes: > >>> +bitmap=$(ls .git/objects/pack/*.bitmap) && >> >> I think the 'ls' is unnecessary and this would do: >> >> bitmap=.git/objects/pack/*.bitmap > > Yuck. Oh, wow, now this is embarrassing... > I said "yuck"

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

Jeff King writes: > diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c > index bed1994551..33c08c40f8 100644 > --- a/ewah/ewah_io.c > +++ b/ewah/ewah_io.c > @@ -122,16 +122,23 @@ int ewah_serialize_strbuf(struct ewah_bitmap *self, > struct strbuf *sb) > return ewah_serialize_to(self,

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

SZEDER Gábor writes: >> +bitmap=$(ls .git/objects/pack/*.bitmap) && > > I think the 'ls' is unnecessary and this would do: > > bitmap=.git/objects/pack/*.bitmap Yuck. >> +test_when_finished "rm -f $bitmap" && OK, this will become "rm -f .git/objects/pack/*.bitmap" and then eval that

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

> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh > index 423c0a475f..237ee6e5fc 100755 > --- a/t/t5310-pack-bitmaps.sh > +++ b/t/t5310-pack-bitmaps.sh > @@ -331,4 +331,17 @@ test_expect_success 'pack reuse respects --incremental' ' > git show-index actual && > test_cmp

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

The on-disk ewah format tells us how big the ewah data is, and we blindly read that much from the buffer without considering whether the mmap'd data is long enough, which can lead to out-of-bound reads. Let's make sure we have data available before reading it, both for the ewah header/footer as