On Sun, Apr 13, 2014 at 06:34:17PM -0400, Ted Unangst wrote:
> I took another look at the way junk works in malloc, and there's a few
> improvements I'd like to make.
>
> 1. Remove the Z option. In general, I think malloc options should make
> programs crash more, not less. This option is a bandaid, but looks
> like something that makes things better. (Anecdotally, I've seen
> people turn it on because it somehow sounded even better than J.
> Incorrect.)
>
> 2. Default to something halfway to J. When we free a chunk, always
> overwrite it. When we free a larger allocation, just overwrite the
> front of it. This has the desirable property of wiping small freed
> secrets and data structures, but won't have the performance impact of
> wiping large buffers (or faulting in pages that weren't even touched).
> J turns it up to junking on malloc() as before, and j turns it off.
>
> I have't been able to build perl reliably with this diff, but haven't
> ruled out the 5.18 update either.
Idea looks good...
Some remarks:
- always junk half a page? Maybe that's a tad over done, athough it
might be relatively cheap since your writing the page anyway.
- Chunks are now always completely junked on free, maybe a bit
overdone for malloc_junk = 1
- if we are freeing a large region, it does not end up in the cache,
so junking (part of) it is a waste of effort. (That already is the
case for current, you are not introducing that).
- What about 'j'? Do we want it to cancel a J or always disable
junking?
This need some timing/profiling on various archs to get some idea on
the costs.
-Otto
>
> Index: malloc.3
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/malloc.3,v
> retrieving revision 1.73
> diff -u -p -r1.73 malloc.3
> --- malloc.3 18 Jul 2013 10:14:49 -0000 1.73
> +++ malloc.3 13 Apr 2014 21:49:47 -0000
> @@ -298,11 +298,6 @@ malloc_options = "X";
> .Pp
> Note that this will cause code that is supposed to handle
> out-of-memory conditions gracefully to abort instead.
> -.It Cm Z
> -.Dq Zero .
> -Fill some junk into the area allocated (see
> -.Cm J ) ,
> -except for the exact length the user asked for, which is zeroed.
> .It Cm <
> .Dq Half the cache size .
> Decrease the size of the free page cache by a factor of two.
> Index: malloc.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.152
> diff -u -p -r1.152 malloc.c
> --- malloc.c 3 Apr 2014 16:18:11 -0000 1.152
> +++ malloc.c 13 Apr 2014 21:05:11 -0000
> @@ -167,7 +167,6 @@ struct malloc_readonly {
> int malloc_move; /* move allocations to end of page? */
> int malloc_realloc; /* always realloc? */
> int malloc_xmalloc; /* xmalloc behaviour? */
> - int malloc_zero; /* zero fill? */
> size_t malloc_guard; /* use guard pages after allocations? */
> u_int malloc_cache; /* free pages we cache */
> #ifdef MALLOC_STATS
> @@ -412,7 +411,7 @@ map(struct dir_info *d, size_t sz, int z
> d->free_regions_size -= psz;
> if (zero_fill)
> memset(p, 0, sz);
> - else if (mopts.malloc_junk &&
> + else if (mopts.malloc_junk == 2 &&
> mopts.malloc_freeunmap)
> memset(p, SOME_FREEJUNK, sz);
> return p;
> @@ -431,7 +430,7 @@ map(struct dir_info *d, size_t sz, int z
> d->free_regions_size -= psz;
> if (zero_fill)
> memset(p, 0, sz);
> - else if (mopts.malloc_junk && mopts.malloc_freeunmap)
> + else if (mopts.malloc_junk == 2 && mopts.malloc_freeunmap)
> memset(p, SOME_FREEJUNK, sz);
> return p;
> }
> @@ -461,6 +460,7 @@ omalloc_init(struct dir_info **dp)
> * Default options
> */
> mopts.malloc_abort = 1;
> + mopts.malloc_junk = 1;
> mopts.malloc_move = 1;
> mopts.malloc_cache = MALLOC_DEFAULT_CACHE;
>
> @@ -534,7 +534,7 @@ omalloc_init(struct dir_info **dp)
> mopts.malloc_junk = 0;
> break;
> case 'J':
> - mopts.malloc_junk = 1;
> + mopts.malloc_junk = 2;
> break;
> case 'n':
> case 'N':
> @@ -557,7 +557,8 @@ omalloc_init(struct dir_info **dp)
> mopts.malloc_cache = MALLOC_DEFAULT_CACHE;
> break;
> case 'S':
> - mopts.malloc_freeunmap = mopts.malloc_junk = 1;
> + mopts.malloc_freeunmap = 1;
> + mopts.malloc_junk = 2;
> mopts.malloc_guard = MALLOC_PAGESIZE;
> mopts.malloc_cache = 0;
> break;
> @@ -573,12 +574,6 @@ omalloc_init(struct dir_info **dp)
> case 'X':
> mopts.malloc_xmalloc = 1;
> break;
> - case 'z':
> - mopts.malloc_zero = 0;
> - break;
> - case 'Z':
> - mopts.malloc_zero = 1;
> - break;
> default: {
> static const char q[] = "malloc() warning: "
> "unknown char in MALLOC_OPTIONS\n";
> @@ -589,13 +584,6 @@ omalloc_init(struct dir_info **dp)
> }
> }
>
> - /*
> - * We want junk in the entire allocation, and zero only in the part
> - * the user asked for.
> - */
> - if (mopts.malloc_zero)
> - mopts.malloc_junk = 1;
> -
> #ifdef MALLOC_STATS
> if (mopts.malloc_stats && (atexit(malloc_exit) == -1)) {
> static const char q[] = "malloc() warning: atexit(2) failed."
> @@ -969,7 +957,7 @@ malloc_bytes(struct dir_info *d, size_t
> k += (lp - bp->bits) * MALLOC_BITS;
> k <<= bp->shift;
>
> - if (mopts.malloc_junk && bp->size > 0)
> + if (mopts.malloc_junk == 2 && bp->size > 0)
> memset((char *)bp->page + k, SOME_JUNK, bp->size);
> return ((char *)bp->page + k);
> }
> @@ -1067,16 +1055,16 @@ omalloc(size_t sz, int zero_fill, void *
> sz - mopts.malloc_guard < MALLOC_PAGESIZE -
> MALLOC_LEEWAY) {
> /* fill whole allocation */
> - if (mopts.malloc_junk)
> + if (mopts.malloc_junk == 2)
> memset(p, SOME_JUNK, psz - mopts.malloc_guard);
> /* shift towards the end */
> p = ((char *)p) + ((MALLOC_PAGESIZE - MALLOC_LEEWAY -
> (sz - mopts.malloc_guard)) & ~(MALLOC_MINSIZE-1));
> /* fill zeros if needed and overwritten above */
> - if (zero_fill && mopts.malloc_junk)
> + if (zero_fill && mopts.malloc_junk == 2)
> memset(p, 0, sz - mopts.malloc_guard);
> } else {
> - if (mopts.malloc_junk) {
> + if (mopts.malloc_junk == 2) {
> if (zero_fill)
> memset((char *)p + sz -
> mopts.malloc_guard,
> SOME_JUNK, psz - sz);
> @@ -1144,7 +1132,7 @@ malloc(size_t size)
> malloc_recurse();
> return NULL;
> }
> - r = omalloc(size, mopts.malloc_zero, CALLER);
> + r = omalloc(size, 0, CALLER);
> malloc_active--;
> _MALLOC_UNLOCK();
> if (r == NULL && mopts.malloc_xmalloc) {
> @@ -1196,9 +1184,11 @@ ofree(void *p)
> }
> malloc_guarded -= mopts.malloc_guard;
> }
> - if (mopts.malloc_junk && !mopts.malloc_freeunmap)
> - memset(p, SOME_FREEJUNK,
> - PAGEROUND(sz) - mopts.malloc_guard);
> + if (mopts.malloc_junk && !mopts.malloc_freeunmap) {
> + size_t amt = mopts.malloc_junk == 1 ? MALLOC_MAXCHUNK :
> + PAGEROUND(sz) - mopts.malloc_guard;
> + memset(p, SOME_FREEJUNK, amt);
> + }
> unmap(g_pool, p, PAGEROUND(sz));
> delete(g_pool, r);
> } else {
> @@ -1302,7 +1292,7 @@ orealloc(void *p, size_t newsz, void *f)
> q = MAP_FAILED;
> if (q == hint) {
> malloc_used += needed;
> - if (mopts.malloc_junk)
> + if (mopts.malloc_junk == 2)
> memset(q, SOME_JUNK, needed);
> r->size = newsz;
> STATS_SETF(r, f);
> @@ -1329,7 +1319,7 @@ orealloc(void *p, size_t newsz, void *f)
> STATS_SETF(r, f);
> return p;
> } else {
> - if (newsz > oldsz && mopts.malloc_junk)
> + if (newsz > oldsz && mopts.malloc_junk == 2)
> memset((char *)p + newsz, SOME_JUNK,
> rnewsz - mopts.malloc_guard - newsz);
> r->size = gnewsz;
> @@ -1338,7 +1328,7 @@ orealloc(void *p, size_t newsz, void *f)
> }
> }
> if (newsz <= oldsz && newsz > oldsz / 2 && !mopts.malloc_realloc) {
> - if (mopts.malloc_junk && newsz > 0)
> + if (mopts.malloc_junk == 2 && newsz > 0)
> memset((char *)p + newsz, SOME_JUNK, oldsz - newsz);
> STATS_SETF(r, f);
> return p;
> @@ -1509,7 +1499,7 @@ omemalign(size_t alignment, size_t sz, i
> malloc_guarded += mopts.malloc_guard;
> }
>
> - if (mopts.malloc_junk) {
> + if (mopts.malloc_junk == 2) {
> if (zero_fill)
> memset((char *)p + sz - mopts.malloc_guard,
> SOME_JUNK, psz - sz);
> @@ -1540,7 +1530,7 @@ posix_memalign(void **memptr, size_t ali
> malloc_recurse();
> goto err;
> }
> - r = omemalign(alignment, size, mopts.malloc_zero, CALLER);
> + r = omemalign(alignment, size, 0, CALLER);
> malloc_active--;
> _MALLOC_UNLOCK();
> if (r == NULL) {
