On Tue, 2016-09-13 at 13:27 +0200, Otto Moerbeek wrote:
> On Thu, Sep 08, 2016 at 06:42:33PM -0400, Daniel Micay wrote:
>
> > A bit off-topic: 'J' enables junk-on-init which is for debugging,
> > but it
> > also currently has security improvements for large allocations.
> > There's
> > only partial junk-on-free by default (half a page), and 'U' disables
> > large allocation junk-on-free without 'J'. I think it would make
> > sense
> > to remove those optimizations since it's fine if the cost scales up
> > with
> > larger allocations and losing the guarantee of not leaking data via
> > uninitialized memory with 'U' is not great. Using 'U' is quite
> > expensive
> > regardless, and adds some pathological performance cases for small
> > size
> > allocations which is more important. I ended up removing both of
> > those
> > optimizations for the CopperheadOS port.
>
> I would prefer to see a diff with this. For me, that should be easier
> to understand than you description.
This is the diff from the CopperheadOS port which won't apply directly
to malloc.c in OpenBSD, but should explain what I mean since it's just a
few lines. Just ignore the part where it removes malloc_junk=2, which is
because junk-on-init is split out (so this obsoleted the extra mode).
The current OpenBSD code only wipes up to MALLOC_MAXCHUNK with junk @ 1,
and it similarly doesn't wipe at all with 'U' (even though junk-on-free
also serves the purpose of preventing information leaks, not just
mitigating use-after-free). IMO, optimizing large allocation perf like
this isn't worthwhile.
diff --git a/libc/bionic/omalloc.c b/libc/bionic/omalloc.c
index e451d79..9277ee7 100644
--- a/libc/bionic/omalloc.c
+++ b/libc/bionic/omalloc.c
@@ -504,7 +504,7 @@ map(struct dir_info *d, void *hint, size_t sz, int
zero_fill)
madvise(p, sz, MADV_NORMAL);
if (zero_fill)
memset(p, 0, sz);
- else if (mopts.malloc_junk == 2 &&
+ else if (mopts.malloc_junk &&
mopts.malloc_freeunmap)
memset(p, SOME_FREEJUNK, sz);
return p;
@@ -524,7 +524,7 @@ map(struct dir_info *d, void *hint, size_t sz, int
zero_fill)
d->free_regions_size -= psz;
if (zero_fill)
memset(p, 0, sz);
- else if (mopts.malloc_junk == 2 && mopts.malloc_freeunmap)
+ else if (mopts.malloc_junk && mopts.malloc_freeunmap)
memset(p, SOME_FREEJUNK, sz);
return p;
}
@@ -603,7 +603,7 @@ omalloc_parseopt(char opt)
mopts.malloc_junk = 0;
break;
case 'J':
- mopts.malloc_junk = 2;
+ mopts.malloc_junk = 1;
break;
case 'i':
mopts.malloc_junk_init = 0;
@@ -1517,8 +1517,7 @@ ofree(struct dir_info *pool, void *p)
STATS_SUB(pool->malloc_guarded, mopts.malloc_guard);
}
if (mopts.malloc_junk && !mopts.malloc_freeunmap) {
- size_t amt = mopts.malloc_junk == 1 ? MALLOC_MAXCHUNK :
- PAGEROUND(sz) - mopts.malloc_guard;
+ size_t amt = PAGEROUND(sz) - mopts.malloc_guard;
memset(p, SOME_FREEJUNK, amt);
}
unmap(pool, p, PAGEROUND(sz));
