| commit b90ddd08f6dd688e651df9ee89ca3a69ff88cd0c |
| Author: Istvan Kurucsai <pistukem@gmail.com> |
| Date: Tue Jan 16 14:54:32 2018 +0100 |
| |
| malloc: Additional checks for unsorted bin integrity I. |
| |
| On Thu, Jan 11, 2018 at 3:50 PM, Florian Weimer <fweimer@redhat.com> wrote: |
| > On 11/07/2017 04:27 PM, Istvan Kurucsai wrote: |
| >> |
| >> + next = chunk_at_offset (victim, size); |
| > |
| > |
| > For new code, we prefer declarations with initializers. |
| |
| Noted. |
| |
| >> + 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. |
| |
| I'm fairly certain that it compares mchunk_size of victim against |
| mchunk_prev_size of the next chunk, i.e. the size of victim in its |
| header and footer. |
| |
| >> + 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. |
| |
| Noted. |
| |
| > Why do you keep using chunksize_nomask? We never investigated why the |
| > original code uses it. It may have been an accident. |
| |
| You are right, I don't think it makes a difference in these checks. So |
| the size local can be reused for the checks against victim. For next, |
| leaving it as such avoids the masking operation. |
| |
| > 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? |
| |
| We could also do a chunk boundary check similar to what I proposed in |
| the thread for the first patch in the series to be even more strict. |
| I'll gladly try to implement either but believe that refining these |
| checks would bring less benefits than in the case of the top chunk. |
| Intra-arena or intra-heap overlaps would still be doable here with |
| unsorted chunks and I don't see any way to counter that besides more |
| generic measures like randomizing allocations and your metadata |
| encoding patches. |
| |
| I've attached a revised version with the above comments incorporated |
| but without the refined checks. |
| |
| Thanks, |
| Istvan |
| |
| From a12d5d40fd7aed5fa10fc444dcb819947b72b315 Mon Sep 17 00:00:00 2001 |
| From: Istvan Kurucsai <pistukem@gmail.com> |
| Date: Tue, 16 Jan 2018 14:48:16 +0100 |
| Subject: [PATCH v2 1/1] malloc: Additional checks for unsorted bin integrity |
| I. |
| |
| 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. |
| |
| diff --git a/malloc/malloc.c b/malloc/malloc.c |
| index d8d4581a9dcea80a..dad0e73735789530 100644 |
| |
| |
| @@ -3724,11 +3724,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); |
| + mchunkptr next = chunk_at_offset (victim, size); |
| + |
| + if (__glibc_unlikely (size <= 2 * SIZE_SZ) |
| + || __glibc_unlikely (size > 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 |