You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
123 lines
5.3 KiB
123 lines
5.3 KiB
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
|
|
--- a/malloc/malloc.c
|
|
+++ b/malloc/malloc.c
|
|
@@ -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
|