On Fri, Feb 12, 2021 at 02:48:34PM +0100, Otto Moerbeek wrote:
> On Fri, Feb 12, 2021 at 02:18:08PM +0100, Otto Moerbeek wrote:
>
> > Hi,
> >
> > Curently, junking is done like this:
> >
> > - for small chunk, the whole chunk junked on free
> >
> > - for pages sized and larger, the first half a page is filled
> >
> > - after a delayed free, the first 32 bytes of small chunks are
> > validated to not be overwritten
> >
> > - page sized and larger allocations are not validated at all, even if
> > they end up in the cache.
> >
> > This diff changes the following:
> >
> > - I make use of the fact that I know how the chunks are aligned, and
> > write 8 bytes at the time by using a uint64_t pointer. For an
> > allocation a max of 4 such uint64_t's are written spread over the
> > allocation. For pages sized and larger, the first page is junked in
> > such a way.
> >
> > - Delayed free of a small chunk checks the corresponiding way.
> >
> > - Pages ending up in the cache are validated upon unmapping or re-use.
> >
> > The last point is the real gain: we also check for write-after-free
> > for large allocations, which we did not do before.
> >
> > So we are catching more writes-after-frees. A price to pay is that
> > larger chunks are not completely junked, but only a total of 32 bytes
> > are. I chose this number after comparing performance with the current
> > code: we still gain a bit in speed.
> >
> > Junk mode 0 (j) and junk mode 2 (J) are not changed.
> >
> > Please test and review,
> >
> > -Otto
> >
>
> And now with correct version of diff
Any feedback?
-Otto
>
>
> Index: stdlib/malloc.3
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/malloc.3,v
> retrieving revision 1.126
> diff -u -p -r1.126 malloc.3
> --- stdlib/malloc.3 14 Sep 2019 13:16:50 -0000 1.126
> +++ stdlib/malloc.3 12 Feb 2021 08:14:54 -0000
> @@ -619,7 +619,7 @@ or
> reallocate an unallocated pointer was made.
> .It Dq chunk is already free
> There was an attempt to free a chunk that had already been freed.
> -.It Dq use after free
> +.It Dq write after free
> A chunk has been modified after it was freed.
> .It Dq modified chunk-pointer
> The pointer passed to
> Index: stdlib/malloc.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.267
> diff -u -p -r1.267 malloc.c
> --- stdlib/malloc.c 23 Nov 2020 15:42:11 -0000 1.267
> +++ stdlib/malloc.c 12 Feb 2021 08:14:54 -0000
> @@ -89,6 +89,7 @@
> */
> #define SOME_JUNK 0xdb /* deadbeef */
> #define SOME_FREEJUNK 0xdf /* dead, free */
> +#define SOME_FREEJUNK_ULL 0xdfdfdfdfdfdfdfdfULL
>
> #define MMAP(sz,f) mmap(NULL, (sz), PROT_READ | PROT_WRITE, \
> MAP_ANON | MAP_PRIVATE | (f), -1, 0)
> @@ -655,6 +656,49 @@ delete(struct dir_info *d, struct region
> }
> }
>
> +static inline void
> +junk_free(int junk, void *p, size_t sz)
> +{
> + size_t i, step = 1;
> + uint64_t *lp = p;
> +
> + if (junk == 0 || sz == 0)
> + return;
> + sz /= sizeof(uint64_t);
> + if (junk == 1) {
> + if (sz > MALLOC_PAGESIZE / sizeof(uint64_t))
> + sz = MALLOC_PAGESIZE / sizeof(uint64_t);
> + step = sz / 4;
> + if (step == 0)
> + step = 1;
> + }
> + for (i = 0; i < sz; i += step)
> + lp[i] = SOME_FREEJUNK_ULL;
> +}
> +
> +static inline void
> +validate_junk(struct dir_info *pool, void *p, size_t sz)
> +{
> + size_t i, step = 1;
> + uint64_t *lp = p;
> +
> + if (pool->malloc_junk == 0 || sz == 0)
> + return;
> + sz /= sizeof(uint64_t);
> + if (pool->malloc_junk == 1) {
> + if (sz > MALLOC_PAGESIZE / sizeof(uint64_t))
> + sz = MALLOC_PAGESIZE / sizeof(uint64_t);
> + step = sz / 4;
> + if (step == 0)
> + step = 1;
> + }
> + for (i = 0; i < sz; i += step) {
> + if (lp[i] != SOME_FREEJUNK_ULL)
> + wrterror(pool, "write after free %p", p);
> + }
> +}
> +
> +
> /*
> * Cache maintenance. We keep at most malloc_cache pages cached.
> * If the cache is becoming full, unmap pages in the cache for real,
> @@ -663,7 +707,7 @@ delete(struct dir_info *d, struct region
> * cache are in MALLOC_PAGESIZE units.
> */
> static void
> -unmap(struct dir_info *d, void *p, size_t sz, size_t clear, int junk)
> +unmap(struct dir_info *d, void *p, size_t sz, size_t clear)
> {
> size_t psz = sz >> MALLOC_PAGESHIFT;
> size_t rsz;
> @@ -695,6 +739,8 @@ unmap(struct dir_info *d, void *p, size_
> r = &d->free_regions[(i + offset) & mask];
> if (r->p != NULL) {
> rsz = r->size << MALLOC_PAGESHIFT;
> + if (!mopts.malloc_freeunmap)
> + validate_junk(d, r->p, rsz);
> if (munmap(r->p, rsz))
> wrterror(d, "munmap %p", r->p);
> r->p = NULL;
> @@ -716,12 +762,11 @@ unmap(struct dir_info *d, void *p, size_
> if (r->p == NULL) {
> if (clear > 0)
> memset(p, 0, clear);
> - if (junk && !mopts.malloc_freeunmap) {
> - size_t amt = junk == 1 ? MALLOC_MAXCHUNK : sz;
> - memset(p, SOME_FREEJUNK, amt);
> - }
> if (mopts.malloc_freeunmap)
> mprotect(p, sz, PROT_NONE);
> + else
> + junk_free(d->malloc_junk, p,
> + psz << MALLOC_PAGESHIFT);
> r->p = p;
> r->size = psz;
> d->free_regions_size += psz;
> @@ -760,15 +805,17 @@ map(struct dir_info *d, size_t sz, int z
> if (r->p != NULL) {
> if (r->size == psz) {
> p = r->p;
> + if (!mopts.malloc_freeunmap)
> + validate_junk(d, p,
> + psz << MALLOC_PAGESHIFT);
> r->p = NULL;
> d->free_regions_size -= psz;
> if (mopts.malloc_freeunmap)
> mprotect(p, sz, PROT_READ | PROT_WRITE);
> if (zero_fill)
> memset(p, 0, sz);
> - else if (d->malloc_junk == 2 &&
> - mopts.malloc_freeunmap)
> - memset(p, SOME_FREEJUNK, sz);
> + else if (mopts.malloc_freeunmap)
> + junk_free(d->malloc_junk, p, sz);
> d->rotor += i + 1;
> return p;
> } else if (r->size > psz)
> @@ -778,15 +825,20 @@ map(struct dir_info *d, size_t sz, int z
> if (big != NULL) {
> r = big;
> p = r->p;
> + if (!mopts.malloc_freeunmap)
> + validate_junk(d, p, r->size << MALLOC_PAGESHIFT);
> r->p = (char *)r->p + (psz << MALLOC_PAGESHIFT);
> - if (mopts.malloc_freeunmap)
> - mprotect(p, sz, PROT_READ | PROT_WRITE);
> r->size -= psz;
> d->free_regions_size -= psz;
> + if (mopts.malloc_freeunmap)
> + mprotect(p, sz, PROT_READ | PROT_WRITE);
> + else
> + junk_free(d->malloc_junk, r->p,
> + r->size << MALLOC_PAGESHIFT);
> if (zero_fill)
> memset(p, 0, sz);
> - else if (d->malloc_junk == 2 && mopts.malloc_freeunmap)
> - memset(p, SOME_FREEJUNK, sz);
> + else if (mopts.malloc_freeunmap)
> + junk_free(d->malloc_junk, p, sz);
> return p;
> }
> if (d->free_regions_size > d->malloc_cache)
> @@ -892,7 +944,7 @@ omalloc_make_chunks(struct dir_info *d,
> return bp;
>
> err:
> - unmap(d, pp, MALLOC_PAGESIZE, 0, d->malloc_junk);
> + unmap(d, pp, MALLOC_PAGESIZE, 0);
> return NULL;
> }
>
> @@ -1091,7 +1143,7 @@ free_bytes(struct dir_info *d, struct re
>
> if (info->size == 0 && !mopts.malloc_freeunmap)
> mprotect(info->page, MALLOC_PAGESIZE, PROT_READ | PROT_WRITE);
> - unmap(d, info->page, MALLOC_PAGESIZE, 0, 0);
> + unmap(d, info->page, MALLOC_PAGESIZE, 0);
>
> delete(d, r);
> if (info->size != 0)
> @@ -1122,7 +1174,7 @@ omalloc(struct dir_info *pool, size_t sz
> return NULL;
> }
> if (insert(pool, p, sz, f)) {
> - unmap(pool, p, psz, 0, 0);
> + unmap(pool, p, psz, 0);
> errno = ENOMEM;
> return NULL;
> }
> @@ -1282,27 +1334,6 @@ malloc_conceal(size_t size)
> }
> DEF_WEAK(malloc_conceal);
>
> -static void
> -validate_junk(struct dir_info *pool, void *p)
> -{
> - struct region_info *r;
> - size_t byte, sz;
> -
> - if (p == NULL)
> - return;
> - r = find(pool, p);
> - if (r == NULL)
> - wrterror(pool, "bogus pointer in validate_junk %p", p);
> - REALSIZE(sz, r);
> - if (sz > CHUNK_CHECK_LENGTH)
> - sz = CHUNK_CHECK_LENGTH;
> - for (byte = 0; byte < sz; byte++) {
> - if (((unsigned char *)p)[byte] != SOME_FREEJUNK)
> - wrterror(pool, "use after free %p", p);
> - }
> -}
> -
> -
> static struct region_info *
> findpool(void *p, struct dir_info *argpool, struct dir_info **foundpool,
> char **saved_function)
> @@ -1402,8 +1433,7 @@ ofree(struct dir_info **argpool, void *p
> }
> STATS_SUB(pool->malloc_guarded, mopts.malloc_guard);
> }
> - unmap(pool, p, PAGEROUND(sz), clear ? argsz : 0,
> - pool->malloc_junk);
> + unmap(pool, p, PAGEROUND(sz), clear ? argsz : 0);
> delete(pool, r);
> } else {
> /* Validate and optionally canary check */
> @@ -1419,20 +1449,24 @@ ofree(struct dir_info **argpool, void *p
> wrterror(pool,
> "double free %p", p);
> }
> - if (pool->malloc_junk && sz > 0)
> - memset(p, SOME_FREEJUNK, sz);
> + junk_free(pool->malloc_junk, p, sz);
> i = getrbyte(pool) & MALLOC_DELAYED_CHUNK_MASK;
> tmp = p;
> p = pool->delayed_chunks[i];
> if (tmp == p)
> wrterror(pool, "double free %p", tmp);
> pool->delayed_chunks[i] = tmp;
> - if (pool->malloc_junk)
> - validate_junk(pool, p);
> - } else if (argsz > 0)
> + if (p != NULL) {
> + r = find(pool, p);
> + REALSIZE(sz, r);
> + if (r != NULL)
> + validate_junk(pool, p, sz);
> + }
> + } else if (argsz > 0) {
> + r = find(pool, p);
> memset(p, 0, argsz);
> + }
> if (p != NULL) {
> - r = find(pool, p);
> if (r == NULL)
> wrterror(pool,
> "bogus pointer (double free?) %p", p);
> @@ -1969,7 +2003,7 @@ omemalign(struct dir_info *pool, size_t
> }
>
> if (insert(pool, p, sz, f)) {
> - unmap(pool, p, psz, 0, 0);
> + unmap(pool, p, psz, 0);
> errno = ENOMEM;
> return NULL;
> }
>
