| Message ID | 1510068430-27816-2-git-send-email-pistukem@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Additional integrity checks for the malloc | expand |
On Nov 07 2017, Istvan Kurucsai <pistukem@gmail.com> wrote: > diff --git a/malloc/malloc.c b/malloc/malloc.c > index f94d51c..4a30c42 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -4078,6 +4078,10 @@ _int_malloc (mstate av, size_t bytes) > > if ((unsigned long) (size) >= (unsigned long) (nb + MINSIZE)) > { > + if (__glibc_unlikely ((unsigned long) (size) > > + (unsigned long) (av->system_mem))) Line break before operator, not after. Redundant parens. > + malloc_printerr("malloc(): corrupted top chunk"); Space before paren. Andreas.
On 11/07/2017 04:27 PM, Istvan Kurucsai wrote: > Ensure that the size of top is below av->system_mem. > > * malloc/malloc.c (_int_malloc): Check top size. > --- > malloc/malloc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/malloc/malloc.c b/malloc/malloc.c > index f94d51c..4a30c42 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -4078,6 +4078,10 @@ _int_malloc (mstate av, size_t bytes) > > if ((unsigned long) (size) >= (unsigned long) (nb + MINSIZE)) > { > + if (__glibc_unlikely ((unsigned long) (size) > > + (unsigned long) (av->system_mem))) > + malloc_printerr("malloc(): corrupted top chunk"); > + Andreas already pointed out style issues. I'm somewhat surprised that we have accurate accounting in av->system_mem. Furthermore, for non-main arenas, I think the check should be against the size of a single heap, or maybe the minimum of av->system_mem and that size. Thanks, Florian
> Andreas already pointed out style issues. > > I'm somewhat surprised that we have accurate accounting in av->system_mem. > > Furthermore, for non-main arenas, I think the check should be against the > size of a single heap, or maybe the minimum of av->system_mem and that size. I thought about this and believe that we can ensure something more strict: that the end of the top chunk is the same as the end of the arena (contiguous main_arena case) or the heap (mmapped arena case), see below. Tests passed but I'm a bit uncertain if these invariants are always held. Ensure that the end of the top chunk is the same as the end of the arena/heap. * malloc/malloc.c (_int_malloc): Check top size. --- malloc/malloc.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/malloc/malloc.c b/malloc/malloc.c index f5aafd2..fd0f001 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -2251,6 +2251,33 @@ do_check_malloc_state (mstate av) } #endif +static bool +valid_top_chunk (mstate av, mchunkptr top) +{ + size_t size = chunksize(top); + + assert (av); + assert (av->top != initial_top (av)); + + if (av == &main_arena) + { + if ((contiguous (&main_arena) + && __glibc_unlikely ((uintptr_t) top + size + != (uintptr_t) mp_.sbrk_base + av->system_mem)) + || (!contiguous (&main_arena) + && __glibc_unlikely (size > av->system_mem))) + return false; + } + else + { + heap_info *heap = heap_for_ptr (top); + uintptr_t heap_end = (uintptr_t) heap + heap->size; + if (__glibc_unlikely ((uintptr_t) top + size != heap_end)) + return false; + } + + return true; +} /* ----------------- Support for debugging hooks -------------------- */ #include "hooks.c" @@ -4088,6 +4115,8 @@ _int_malloc (mstate av, size_t bytes) if ((unsigned long) (size) >= (unsigned long) (nb + MINSIZE)) { + if (__glibc_unlikely (!valid_top_chunk (av, victim))) + malloc_printerr ("malloc(): corrupted top chunk"); remainder_size = size - nb; remainder = chunk_at_offset (victim, nb); av->top = remainder;
On 01/16/2018 01:05 PM, Istvan Kurucsai wrote: >> Andreas already pointed out style issues. >> >> I'm somewhat surprised that we have accurate accounting in av->system_mem. >> >> Furthermore, for non-main arenas, I think the check should be against the >> size of a single heap, or maybe the minimum of av->system_mem and that size. > > I thought about this and believe that we can ensure something more > strict: that the end of the top chunk is the same as the end of the > arena (contiguous main_arena case) or the heap (mmapped arena case), > see below. Tests passed but I'm a bit uncertain if these invariants > are always held. > > > Ensure that the end of the top chunk is the same as > the end of the arena/heap. > > * malloc/malloc.c (_int_malloc): Check top size. > --- > malloc/malloc.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/malloc/malloc.c b/malloc/malloc.c > index f5aafd2..fd0f001 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -2251,6 +2251,33 @@ do_check_malloc_state (mstate av) > } > #endif > > +static bool > +valid_top_chunk (mstate av, mchunkptr top) > +{ > + size_t size = chunksize(top); > + > + assert (av); I think we can drop that assert, it is implied by the pointer dereference in the subsequent assert. > + assert (av->top != initial_top (av)); > + > + if (av == &main_arena) > + { > + if ((contiguous (&main_arena) > + && __glibc_unlikely ((uintptr_t) top + size > + != (uintptr_t) mp_.sbrk_base + av->system_mem)) > + || (!contiguous (&main_arena) > + && __glibc_unlikely (size > av->system_mem))) > + return false; > + } > + else > + { > + heap_info *heap = heap_for_ptr (top); > + uintptr_t heap_end = (uintptr_t) heap + heap->size; > + if (__glibc_unlikely ((uintptr_t) top + size != heap_end)) > + return false; > + } > + > + return true; > +} I wonder if it is possible to write this in a slightly clearer way. Maybe add a nested if (contiguous (&main_arena)) for the first branch, and directly return the value of the inner-most conditional? Thanks, Florian
On 02/20/2018 02:45 PM, Florian Weimer wrote: > On 01/16/2018 01:05 PM, Istvan Kurucsai wrote: >>> Andreas already pointed out style issues. >>> >>> I'm somewhat surprised that we have accurate accounting in >>> av->system_mem. >>> >>> Furthermore, for non-main arenas, I think the check should be against >>> the >>> size of a single heap, or maybe the minimum of av->system_mem and >>> that size. >> >> I thought about this and believe that we can ensure something more >> strict: that the end of the top chunk is the same as the end of the >> arena (contiguous main_arena case) or the heap (mmapped arena case), >> see below. Tests passed but I'm a bit uncertain if these invariants >> are always held. >> >> >> Ensure that the end of the top chunk is the same as >> the end of the arena/heap. >> >> * malloc/malloc.c (_int_malloc): Check top size. >> --- >> malloc/malloc.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/malloc/malloc.c b/malloc/malloc.c >> index f5aafd2..fd0f001 100644 >> --- a/malloc/malloc.c >> +++ b/malloc/malloc.c >> @@ -2251,6 +2251,33 @@ do_check_malloc_state (mstate av) >> } >> #endif >> >> +static bool >> +valid_top_chunk (mstate av, mchunkptr top) >> +{ >> + size_t size = chunksize(top); >> + >> + assert (av); > > I think we can drop that assert, it is implied by the pointer > dereference in the subsequent assert. > >> + assert (av->top != initial_top (av)); >> + >> + if (av == &main_arena) >> + { >> + if ((contiguous (&main_arena) >> + && __glibc_unlikely ((uintptr_t) top + size >> + != (uintptr_t) mp_.sbrk_base + >> av->system_mem)) >> + || (!contiguous (&main_arena) >> + && __glibc_unlikely (size > av->system_mem))) >> + return false; >> + } >> + else >> + { >> + heap_info *heap = heap_for_ptr (top); >> + uintptr_t heap_end = (uintptr_t) heap + heap->size; >> + if (__glibc_unlikely ((uintptr_t) top + size != heap_end)) >> + return false; >> + } >> + >> + return true; >> +} > > I wonder if it is possible to write this in a slightly clearer way. > > Maybe add a nested if (contiguous (&main_arena)) for the first branch, > and directly return the value of the inner-most conditional? Any further comments here? Thanks, Florian
diff --git a/malloc/malloc.c b/malloc/malloc.c index f94d51c..4a30c42 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -4078,6 +4078,10 @@ _int_malloc (mstate av, size_t bytes) if ((unsigned long) (size) >= (unsigned long) (nb + MINSIZE)) { + if (__glibc_unlikely ((unsigned long) (size) > + (unsigned long) (av->system_mem))) + malloc_printerr("malloc(): corrupted top chunk"); + remainder_size = size - nb; remainder = chunk_at_offset (victim, nb); av->top = remainder;
Ensure that the size of top is below av->system_mem. * malloc/malloc.c (_int_malloc): Check top size. --- malloc/malloc.c | 4 ++++ 1 file changed, 4 insertions(+)