diff mbox

[v2,2/7] malloc: Additional checks for unsorted bin integrity I.

Message ID 1510068430-27816-3-git-send-email-pistukem@gmail.com
State Superseded, archived
Headers

Commit Message

Istvan Kurucsai Nov. 7, 2017, 3:27 p.m. UTC 
  Ensure the following properties of chunks encountered during binning:
- victim chunk has reasonable size
- next chunk has reasonable size
- next->prev_size == victim->size
- valid double linked list
- PREV_INUSE of next chunk is unset

    * malloc/malloc.c (_int_malloc): Additional binning code checks.
---
 malloc/malloc.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Florian Weimer Jan. 11, 2018, 2:50 p.m. UTC | #1 
On 11/07/2017 04:27 PM, Istvan Kurucsai wrote:
> +          next = chunk_at_offset (victim, size);

For new code, we prefer declarations with initializers.

> +          if (__glibc_unlikely (chunksize_nomask (victim) <= 2 * SIZE_SZ)
> +              || __glibc_unlikely (chunksize_nomask (victim) > av->system_mem))
> +            malloc_printerr("malloc(): invalid size (unsorted)");
> +          if (__glibc_unlikely (chunksize_nomask (next) < 2 * SIZE_SZ)
> +              || __glibc_unlikely (chunksize_nomask (next) > av->system_mem))
> +            malloc_printerr("malloc(): invalid next size (unsorted)");
> +          if (__glibc_unlikely ((prev_size (next) & ~(SIZE_BITS)) != size))
> +            malloc_printerr("malloc(): mismatching next->prev_size (unsorted)");

I think this check is redundant because prev_size (next) and chunksize 
(victim) are loaded from the same memory location.

> +          if (__glibc_unlikely (bck->fd != victim)
> +              || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
> +            malloc_printerr("malloc(): unsorted double linked list corrupted");
> +          if (__glibc_unlikely (prev_inuse(next)))
> +            malloc_printerr("malloc(): invalid next->prev_inuse (unsorted)");

There's a missing space after malloc_printerr.

Why do you keep using chunksize_nomask?  We never investigated why the 
original code uses it.  It may have been an accident.

Again, for non-main arenas, the checks against av->system_mem could be 
made tighter (against the heap size).  Maybe you could put the condition 
into a separate inline function?

Thanks,
Florian
diff mbox

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 4a30c42..d3fac7e 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3513,6 +3513,7 @@  _int_malloc (mstate av, size_t bytes)
   INTERNAL_SIZE_T size;             /* its size */
   int victim_index;                 /* its bin index */
 
+  mchunkptr next;                   /* next contiguous chunk */
   mchunkptr remainder;              /* remainder from a split */
   unsigned long remainder_size;     /* its size */
 
@@ -3718,11 +3719,22 @@  _int_malloc (mstate av, size_t bytes)
       while ((victim = unsorted_chunks (av)->bk) != unsorted_chunks (av))
         {
           bck = victim->bk;
-          if (__builtin_expect (chunksize_nomask (victim) <= 2 * SIZE_SZ, 0)
-              || __builtin_expect (chunksize_nomask (victim)
-				   > av->system_mem, 0))
-            malloc_printerr ("malloc(): memory corruption");
           size = chunksize (victim);
+          next = chunk_at_offset (victim, size);
+
+          if (__glibc_unlikely (chunksize_nomask (victim) <= 2 * SIZE_SZ)
+              || __glibc_unlikely (chunksize_nomask (victim) > av->system_mem))
+            malloc_printerr("malloc(): invalid size (unsorted)");
+          if (__glibc_unlikely (chunksize_nomask (next) < 2 * SIZE_SZ)
+              || __glibc_unlikely (chunksize_nomask (next) > av->system_mem))
+            malloc_printerr("malloc(): invalid next size (unsorted)");
+          if (__glibc_unlikely ((prev_size (next) & ~(SIZE_BITS)) != size))
+            malloc_printerr("malloc(): mismatching next->prev_size (unsorted)");
+          if (__glibc_unlikely (bck->fd != victim)
+              || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
+            malloc_printerr("malloc(): unsorted double linked list corrupted");
+          if (__glibc_unlikely (prev_inuse(next)))
+            malloc_printerr("malloc(): invalid next->prev_inuse (unsorted)");
 
           /*
              If a small request, try to use last remainder if it is the