[Bug malloc/15089] New: malloc_trim always trims for large padding

["izetip at yahoo dot com" <sourceware-bugzilla at sourceware dot org>]

http://sourceware.org/bugzilla/show_bug.cgi?id=15089

             Bug #: 15089
           Summary: malloc_trim always trims for large padding
           Product: glibc
           Version: 2.18
            Status: NEW
          Severity: normal
          Priority: P2
         Component: malloc
        AssignedTo: unassigned@sourceware.org
        ReportedBy: izetip@yahoo.com
    Classification: Unclassified


If you call malloc_trim() with a large enough padding, then an error with how
signed/unsigned arithmetic is handled will cause that large padding to actually
have the same effect as specifying no padding.  This is a problem since
malloc_trim() then becomes overly aggressive and hurts performance.

The bug is in systrim().  Here's the current code:
1236 #define MINSIZE  \
1237   (unsigned long)(((MIN_CHUNK_SIZE+MALLOC_ALIGN_MASK) &
~MALLOC_ALIGN_MASK))
....
2707 static int systrim(size_t pad, mstate av)
2708 {
2709   long  top_size;        /* Amount of top-most memory */
2710   long  extra;           /* Amount to release */
...
2714   size_t pagesz;
2715 
2716   pagesz = GLRO(dl_pagesize);
2717   top_size = chunksize(av->top);
2718 
2719   /* Release in pagesize units, keeping at least one page */
2720   extra = (top_size - pad - MINSIZE - 1) & ~(pagesz - 1);
2721 
2722   if (extra > 0) {
...       MORECORE(-extra); ends up being called

Notice that pad and pagesz are of type size_t and MINSIZE is unsigned long, all
of these are unsigned while the other variables are signed.  When computing
extra, the first operation is:
   top_size - pad
this is  (long) - (size_t) and the result is automatically promoted to size_t
-- which is unsigned.  So if pad > top_size, then while the expected result is
that subtracting the two would be negative, the actual result is that it
becomes an extremely large positive value.  Every single following operation
will continue to be promoted to the unsigned size_t.  This means extra will
always be greater than or equal to 0.

Here's an example that shows how the wrong result is produced. If top_size=128
and pad=512, we would expect extra to be negative and no memory to be trimmed
(also, with 128 bytes, we have less than a page left, so that's another reason
we are not supposed to trim regardless of what pad is set to).  Instead we get:
(gdb) p (((long) 128) - (unsigned long)(512) - ((unsigned long)8) - 1) &
~(((unsigned long)4096)-1)
$1 = 4294963200

extra is positive and so we enter the if statement and will likely erroneously
call MORECORE(-4294963200).

What we wanted is something more like:
(gdb) p (((long) 128) - (long)(512) - ((long)8) - 1) & ~(((long)4096)-1)
$2 = -4096

To be truly robust, the fix should handle the situation where pad > 1LL<<62 in
case someone decides they want to disable trimming by setting pad to something
like (size_t)-1.  Something like the following would probably work:
  long x = top_size - ((long)MINSIZE) - 1;
  extra = x <= pad? -1 : (x - ((long)pad)) & ~(pagesz - 1);

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.