On Mon, Jan 09, 2017 at 07:49:24PM +0100, Otto Moerbeek wrote:

> Hi,
> 
> this diff implements some improvements to realloc and some cleanup of
> the MALLOC_MOVE code.
> 
> 1. When shrinking a chunk allocation, compare the size of the current
> allocation to the size of the new allocation (instead of the requested size).
> Current code does a malloc-free-copy dance in too many cases.
> 
> 2. Current code takes the easy way and always reallocates if C is
> active. This diff fixes by carefully updating the recorded requested
> size in all cases, and writing the canary bytes in the proper location
> after reallocating.
> 
> 3. Introduce defines to test if MALLOC_MOVE should be done and to
> compute the new value.
> 
> Please review and test with you favorite (and other) combination of
> malloc flags.

Anybody tested?

No feedback no progress, Jimi Hendrix could have said....

        -Otto


> 
> Index: malloc.c
> ===================================================================
> RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
> retrieving revision 1.211
> diff -u -p -r1.211 malloc.c
> --- malloc.c  4 Nov 2016 09:11:20 -0000       1.211
> +++ malloc.c  9 Jan 2017 13:35:29 -0000
> @@ -73,6 +73,12 @@
>   * Set to zero to be the most strict.
>   */
>  #define MALLOC_LEEWAY                0
> +#define MALLOC_MOVE_COND(sz) ((sz) - mopts.malloc_guard <            \
> +                                 MALLOC_PAGESIZE - MALLOC_LEEWAY)
> +#define MALLOC_MOVE(p, sz)   (((char *)(p)) +                        \
> +                                 ((MALLOC_PAGESIZE - MALLOC_LEEWAY - \
> +                                 ((sz) - mopts.malloc_guard)) &      \
> +                                 ~(MALLOC_MINSIZE - 1)))
>  
>  #define PAGEROUND(x)  (((x) + (MALLOC_PAGEMASK)) & ~MALLOC_PAGEMASK)
>  
> @@ -199,6 +205,7 @@ char              *malloc_options;        /* compile-time 
> o
>  static u_char getrbyte(struct dir_info *d);
>  static __dead void wrterror(struct dir_info *d, char *msg, ...)
>      __attribute__((__format__ (printf, 2, 3)));
> +static void fill_canary(char *ptr, size_t sz, size_t allocated);
>  
>  #ifdef MALLOC_STATS
>  void malloc_dump(int, int, struct dir_info *);
> @@ -209,8 +216,8 @@ static void malloc_exit(void);
>  #define CALLER       NULL
>  #endif
>  
> -/* low bits of r->p determine size: 0 means >= page size and p->size holding
> - *  real size, otherwise r->size is a shift count, or 1 for malloc(0)
> +/* low bits of r->p determine size: 0 means >= page size and r->size holding
> + * real size, otherwise low bits are a shift count, or 1 for malloc(0)
>   */
>  #define REALSIZE(sz, r)                                              \
>       (sz) = (uintptr_t)(r)->p & MALLOC_PAGEMASK,             \
> @@ -905,23 +912,10 @@ omalloc_make_chunks(struct dir_info *d, 
>       return bp;
>  }
>  
> -
> -/*
> - * Allocate a chunk
> - */
> -static void *
> -malloc_bytes(struct dir_info *d, size_t argsize, void *f)
> +static int
> +find_chunksize(size_t size)
>  {
> -     int             i, j, listnum;
> -     size_t          k, size;
> -     u_short         u, *lp;
> -     struct chunk_info *bp;
> -
> -     if (mopts.malloc_canary != (d->canary1 ^ (u_int32_t)(uintptr_t)d) ||
> -         d->canary1 != ~d->canary2)
> -             wrterror(d, "internal struct corrupt");
> -
> -     size = argsize;
> +     int             i, j;
>  
>       /* Don't bother with anything less than this */
>       /* unless we have a malloc(0) requests */
> @@ -937,6 +931,25 @@ malloc_bytes(struct dir_info *d, size_t 
>               while (i >>= 1)
>                       j++;
>       }
> +     return j;
> +}
> +
> +/*
> + * Allocate a chunk
> + */
> +static void *
> +malloc_bytes(struct dir_info *d, size_t size, void *f)
> +{
> +     int             i, j, listnum;
> +     size_t          k;
> +     u_short         u, *lp;
> +     struct chunk_info *bp;
> +
> +     if (mopts.malloc_canary != (d->canary1 ^ (u_int32_t)(uintptr_t)d) ||
> +         d->canary1 != ~d->canary2)
> +             wrterror(d, "internal struct corrupt");
> +
> +     j = find_chunksize(size);
>  
>       listnum = getrbyte(d) % MALLOC_CHUNK_LISTS;
>       /* If it's empty, make a page more of that size chunks */
> @@ -990,25 +1003,30 @@ malloc_bytes(struct dir_info *d, size_t 
>       k += (lp - bp->bits) * MALLOC_BITS;
>       
>       if (mopts.chunk_canaries)
> -             bp->bits[bp->offset + k] = argsize;
> +             bp->bits[bp->offset + k] = size;
>  
>       k <<= bp->shift;
>  
>       if (bp->size > 0) {
>               if (mopts.malloc_junk == 2)
>                       memset((char *)bp->page + k, SOME_JUNK, bp->size);
> -             else if (mopts.chunk_canaries) {
> -                     size_t sz = bp->size - argsize;
> -
> -                     if (sz > CHUNK_CHECK_LENGTH)
> -                             sz = CHUNK_CHECK_LENGTH;
> -                     memset((char *)bp->page + k + argsize, SOME_JUNK, sz);
> -             }
> +             else if (mopts.chunk_canaries)
> +                     fill_canary((char *)bp->page + k, size, bp->size);
>       }
>       return ((char *)bp->page + k);
>  }
>  
>  static void
> +fill_canary(char *ptr, size_t sz, size_t allocated)
> +{
> +     size_t check_sz = allocated - sz;
> +
> +     if (check_sz > CHUNK_CHECK_LENGTH)
> +             check_sz = CHUNK_CHECK_LENGTH;
> +     memset(ptr + sz, SOME_JUNK, check_sz);
> +}
> +
> +static void
>  validate_canary(struct dir_info *d, u_char *ptr, size_t sz, size_t allocated)
>  {
>       size_t check_sz = allocated - sz;
> @@ -1130,13 +1148,12 @@ omalloc(struct dir_info *pool, size_t sz
>                       STATS_ADD(pool->malloc_guarded, mopts.malloc_guard);
>               }
>  
> -             if (sz - mopts.malloc_guard < MALLOC_PAGESIZE - MALLOC_LEEWAY) {
> +             if (MALLOC_MOVE_COND(sz)) {
>                       /* fill whole allocation */
>                       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));
> +                     p = MALLOC_MOVE(p, sz);
>                       /* fill zeros if needed and overwritten above */
>                       if (zero_fill && mopts.malloc_junk == 2)
>                               memset(p, 0, sz - mopts.malloc_guard);
> @@ -1149,14 +1166,9 @@ omalloc(struct dir_info *pool, size_t sz
>                                       memset(p, SOME_JUNK,
>                                           psz - mopts.malloc_guard);
>                       }
> -                     else if (mopts.chunk_canaries) {
> -                             size_t csz = psz - sz;
> -
> -                             if (csz > CHUNK_CHECK_LENGTH)
> -                                     csz = CHUNK_CHECK_LENGTH;
> -                             memset((char *)p + sz - mopts.malloc_guard,
> -                                 SOME_JUNK, csz);
> -                     }
> +                     else if (mopts.chunk_canaries)
> +                             fill_canary(p, sz - mopts.malloc_guard,
> +                                 psz - mopts.malloc_guard);
>               }
>  
>       } else {
> @@ -1308,8 +1320,7 @@ ofree(struct dir_info *argpool, void *p)
>  
>       REALSIZE(sz, r);
>       if (sz > MALLOC_MAXCHUNK) {
> -             if (sz - mopts.malloc_guard >= MALLOC_PAGESIZE -
> -                 MALLOC_LEEWAY) {
> +             if (!MALLOC_MOVE_COND(sz)) {
>                       if (r->p != p)
>                               wrterror(pool, "bogus pointer %p", p);
>                       if (mopts.chunk_canaries)
> @@ -1410,9 +1421,11 @@ orealloc(struct dir_info *argpool, void 
>  {
>       struct dir_info *pool;
>       struct region_info *r;
> +     struct chunk_info *info;
>       size_t oldsz, goldsz, gnewsz;
>       void *q, *ret;
>       int i;
> +     uint32_t chunknum;
>       
>       pool = argpool;
>  
> @@ -1445,6 +1458,11 @@ orealloc(struct dir_info *argpool, void 
>       }
>  
>       REALSIZE(oldsz, r);
> +     if (mopts.chunk_canaries && oldsz <= MALLOC_MAXCHUNK) {
> +             chunknum = find_chunknum(pool, r, p, 0);
> +             info = (struct chunk_info *)r->size;
> +     }
> +
>       goldsz = oldsz;
>       if (oldsz > MALLOC_MAXCHUNK) {
>               if (oldsz < mopts.malloc_guard)
> @@ -1457,11 +1475,14 @@ orealloc(struct dir_info *argpool, void 
>               gnewsz += mopts.malloc_guard;
>  
>       if (newsz > MALLOC_MAXCHUNK && oldsz > MALLOC_MAXCHUNK && p == r->p &&
> -         !mopts.chunk_canaries && !mopts.malloc_realloc) {
> +         !mopts.malloc_realloc) {
> +             /* First case: from n pages sized allocation to m pages sized
> +                allocation, no malloc_move in effect */
>               size_t roldsz = PAGEROUND(goldsz);
>               size_t rnewsz = PAGEROUND(gnewsz);
>  
>               if (rnewsz > roldsz) {
> +                     /* try to extend existing region */
>                       if (!mopts.malloc_guard) {
>                               void *hint = (char *)p + roldsz;
>                               size_t needed = rnewsz - roldsz;
> @@ -1482,6 +1503,8 @@ gotit:
>                                       if (mopts.malloc_junk == 2)
>                                               memset(q, SOME_JUNK, needed);
>                                       r->size = newsz;
> +                                     if (mopts.chunk_canaries)
> +                                             fill_canary(p, newsz, 
> PAGEROUND(newsz));
>                                       STATS_SETF(r, f);
>                                       STATS_INC(pool->cheap_reallocs);
>                                       ret = p;
> @@ -1492,6 +1515,7 @@ gotit:
>                               }
>                       }
>               } else if (rnewsz < roldsz) {
> +                     /* shrink number of pages */
>                       if (mopts.malloc_guard) {
>                               if (mprotect((char *)p + roldsz -
>                                   mopts.malloc_guard, mopts.malloc_guard,
> @@ -1504,27 +1528,38 @@ gotit:
>                       }
>                       unmap(pool, (char *)p + rnewsz, roldsz - rnewsz);
>                       r->size = gnewsz;
> +                     if (mopts.chunk_canaries)
> +                             fill_canary(p, newsz, PAGEROUND(newsz));
>                       STATS_SETF(r, f);
>                       ret = p;
>                       goto done;
>               } else {
> +                     /* number of pages remains the same */
>                       if (newsz > oldsz && mopts.malloc_junk == 2)
>                               memset((char *)p + newsz, SOME_JUNK,
>                                   rnewsz - mopts.malloc_guard - newsz);
>                       r->size = gnewsz;
> +                     if (mopts.chunk_canaries)
> +                             fill_canary(p, newsz, PAGEROUND(newsz));
>                       STATS_SETF(r, f);
>                       ret = p;
>                       goto done;
>               }
>       }
> -     if (newsz <= oldsz && newsz > oldsz / 2 && !mopts.chunk_canaries &&
> -         !mopts.malloc_realloc) {
> -             if (mopts.malloc_junk == 2 && newsz > 0)
> +     if (oldsz <= MALLOC_MAXCHUNK && oldsz > 0 &&
> +         newsz <= MALLOC_MAXCHUNK && newsz > 0 &&
> +         1 << find_chunksize(newsz) == oldsz && !mopts.malloc_realloc) {
> +             /* do not reallocate if new size fits good in existing chunk */
> +             if (mopts.malloc_junk == 2)
>                       memset((char *)p + newsz, SOME_JUNK, oldsz - newsz);
> +             if (mopts.chunk_canaries) {
> +                     info->bits[info->offset + chunknum] = newsz;
> +                     fill_canary(p, newsz, info->size);
> +             }
>               STATS_SETF(r, f);
>               ret = p;
> -     } else if (newsz != oldsz || mopts.chunk_canaries ||
> -         mopts.malloc_realloc) {
> +     } else if (newsz != oldsz || mopts.malloc_realloc) {
> +             /* create new allocation */
>               q = omalloc(pool, newsz, 0, f);
>               if (q == NULL) {
>                       ret = NULL;
> @@ -1535,6 +1570,12 @@ gotit:
>               ofree(pool, p);
>               ret = q;
>       } else {
> +             /* > page size allocation didnt change */
> +             if (mopts.chunk_canaries && oldsz <= MALLOC_MAXCHUNK) {
> +                     info->bits[info->offset + chunknum] = newsz;
> +                     if (info->size > 0)
> +                             fill_canary(p, newsz, info->size);
> +             }       
>               STATS_SETF(r, f);
>               ret = p;
>       }

Reply via email to