From 1f8ced27c389c05c35e575abd7890707a186d4c6 Mon Sep 17 00:00:00 2001 From: Florian Krohm Date: Tue, 3 Mar 2015 14:56:17 +0000 Subject: [PATCH] Produce a user message in case of stack overflow. Change VG_(extend_stack) and VG_(am_extend_into_adjacent_reservation_client) accordingly. Remove some redundant checking. Add testcase. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@14974 --- coregrind/m_aspacemgr/aspacemgr-linux.c | 32 ++++++---- coregrind/m_sigframe/sigframe-amd64-linux.c | 2 +- coregrind/m_sigframe/sigframe-arm-linux.c | 2 +- coregrind/m_sigframe/sigframe-arm64-linux.c | 2 +- coregrind/m_sigframe/sigframe-mips32-linux.c | 7 +-- coregrind/m_sigframe/sigframe-mips64-linux.c | 2 +- coregrind/m_sigframe/sigframe-ppc32-linux.c | 2 +- coregrind/m_sigframe/sigframe-ppc64-linux.c | 2 +- coregrind/m_sigframe/sigframe-s390x-linux.c | 2 +- coregrind/m_sigframe/sigframe-x86-linux.c | 2 +- coregrind/m_signals.c | 63 +++++++++----------- coregrind/m_syswrap/syswrap-generic.c | 4 +- coregrind/m_syswrap/syswrap-main.c | 11 +++- coregrind/pub_core_aspacemgr.h | 2 +- coregrind/pub_core_signals.h | 2 +- none/tests/linux/Makefile.am | 7 ++- none/tests/linux/stack-overflow.c | 9 +++ none/tests/linux/stack-overflow.stderr.exp | 13 ++++ none/tests/linux/stack-overflow.vgtest | 1 + 19 files changed, 100 insertions(+), 67 deletions(-) create mode 100644 none/tests/linux/stack-overflow.c create mode 100644 none/tests/linux/stack-overflow.stderr.exp create mode 100644 none/tests/linux/stack-overflow.vgtest diff --git a/coregrind/m_aspacemgr/aspacemgr-linux.c b/coregrind/m_aspacemgr/aspacemgr-linux.c index d0a49eb3e7..573f57d4ae 100644 --- a/coregrind/m_aspacemgr/aspacemgr-linux.c +++ b/coregrind/m_aspacemgr/aspacemgr-linux.c @@ -2789,12 +2789,15 @@ Bool VG_(am_create_reservation) ( Addr start, SizeT length, page long. The function returns a pointer to the resized segment. */ const NSegment *VG_(am_extend_into_adjacent_reservation_client)( Addr addr, - SSizeT delta ) + SSizeT delta, + Bool *overflow) { Int segA, segR; UInt prot; SysRes sres; + *overflow = False; + segA = find_nsegment_idx(addr); aspacem_assert(nsegments[segA].kind == SkAnonC); @@ -2813,11 +2816,14 @@ const NSegment *VG_(am_extend_into_adjacent_reservation_client)( Addr addr, segR = segA+1; if (segR >= nsegments_used || nsegments[segR].kind != SkResvn - || nsegments[segR].smode != SmLower - || nsegments[segR].start != nsegments[segA].end + 1 - || delta + VKI_PAGE_SIZE - > (nsegments[segR].end - nsegments[segR].start + 1)) - return NULL; + || nsegments[segR].smode != SmLower) + return NULL; + + if (delta + VKI_PAGE_SIZE + > (nsegments[segR].end - nsegments[segR].start + 1)) { + *overflow = True; + return NULL; + } /* Extend the kernel's mapping. */ // DDD: #warning GrP fixme MAP_FIXED can clobber memory! @@ -2849,11 +2855,14 @@ const NSegment *VG_(am_extend_into_adjacent_reservation_client)( Addr addr, segR = segA-1; if (segR < 0 || nsegments[segR].kind != SkResvn - || nsegments[segR].smode != SmUpper - || nsegments[segR].end + 1 != nsegments[segA].start - || delta + VKI_PAGE_SIZE - > (nsegments[segR].end - nsegments[segR].start + 1)) - return NULL; + || nsegments[segR].smode != SmUpper) + return NULL; + + if (delta + VKI_PAGE_SIZE + > (nsegments[segR].end - nsegments[segR].start + 1)) { + *overflow = True; + return NULL; + } /* Extend the kernel's mapping. */ // DDD: #warning GrP fixme MAP_FIXED can clobber memory! @@ -2875,7 +2884,6 @@ const NSegment *VG_(am_extend_into_adjacent_reservation_client)( Addr addr, nsegments[segR].end -= delta; nsegments[segA].start -= delta; aspacem_assert(nsegments[segR].start <= nsegments[segR].end); - } AM_SANITY_CHECK; diff --git a/coregrind/m_sigframe/sigframe-amd64-linux.c b/coregrind/m_sigframe/sigframe-amd64-linux.c index 7473fa744d..62698aab93 100644 --- a/coregrind/m_sigframe/sigframe-amd64-linux.c +++ b/coregrind/m_sigframe/sigframe-amd64-linux.c @@ -381,7 +381,7 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) { + if (VG_(extend_stack)(tid, addr)) { stackseg = VG_(am_find_nsegment)(addr); if (0 && stackseg) VG_(printf)("frame=%#lx seg=%#lx-%#lx\n", diff --git a/coregrind/m_sigframe/sigframe-arm-linux.c b/coregrind/m_sigframe/sigframe-arm-linux.c index f949fb1310..a5c8c07ef5 100644 --- a/coregrind/m_sigframe/sigframe-arm-linux.c +++ b/coregrind/m_sigframe/sigframe-arm-linux.c @@ -86,7 +86,7 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) { + if (VG_(extend_stack)(tid, addr)) { stackseg = VG_(am_find_nsegment)(addr); if (0 && stackseg) VG_(printf)("frame=%#lx seg=%#lx-%#lx\n", diff --git a/coregrind/m_sigframe/sigframe-arm64-linux.c b/coregrind/m_sigframe/sigframe-arm64-linux.c index a09025766f..0bea57d0bb 100644 --- a/coregrind/m_sigframe/sigframe-arm64-linux.c +++ b/coregrind/m_sigframe/sigframe-arm64-linux.c @@ -84,7 +84,7 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) { + if (VG_(extend_stack)(tid, addr)) { stackseg = VG_(am_find_nsegment)(addr); if (0 && stackseg) VG_(printf)("frame=%#lx seg=%#lx-%#lx\n", diff --git a/coregrind/m_sigframe/sigframe-mips32-linux.c b/coregrind/m_sigframe/sigframe-mips32-linux.c index 466728873d..4c8c8aa519 100644 --- a/coregrind/m_sigframe/sigframe-mips32-linux.c +++ b/coregrind/m_sigframe/sigframe-mips32-linux.c @@ -83,10 +83,9 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) - { - stackseg = VG_(am_find_nsegment)(addr); - } + if (VG_(extend_stack)(tid, addr)) { + stackseg = VG_(am_find_nsegment)(addr); + } if (stackseg == NULL || !stackseg->hasR || !stackseg->hasW) { diff --git a/coregrind/m_sigframe/sigframe-mips64-linux.c b/coregrind/m_sigframe/sigframe-mips64-linux.c index fc8e613c20..bb178ecf94 100644 --- a/coregrind/m_sigframe/sigframe-mips64-linux.c +++ b/coregrind/m_sigframe/sigframe-mips64-linux.c @@ -80,7 +80,7 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) { + if (VG_(extend_stack)(tid, addr)) { stackseg = VG_(am_find_nsegment)(addr); } diff --git a/coregrind/m_sigframe/sigframe-ppc32-linux.c b/coregrind/m_sigframe/sigframe-ppc32-linux.c index 7f35ac08b5..e48846c648 100644 --- a/coregrind/m_sigframe/sigframe-ppc32-linux.c +++ b/coregrind/m_sigframe/sigframe-ppc32-linux.c @@ -511,7 +511,7 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) { + if (VG_(extend_stack)(tid, addr)) { stackseg = VG_(am_find_nsegment)(addr); if (0 && stackseg) VG_(printf)("frame=%#lx seg=%#lx-%#lx\n", diff --git a/coregrind/m_sigframe/sigframe-ppc64-linux.c b/coregrind/m_sigframe/sigframe-ppc64-linux.c index ce3224a4eb..1bf3f48127 100644 --- a/coregrind/m_sigframe/sigframe-ppc64-linux.c +++ b/coregrind/m_sigframe/sigframe-ppc64-linux.c @@ -141,7 +141,7 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) { + if (VG_(extend_stack)(tid, addr)) { stackseg = VG_(am_find_nsegment)(addr); if (0 && stackseg) VG_(printf)("frame=%#lx seg=%#lx-%#lx\n", diff --git a/coregrind/m_sigframe/sigframe-s390x-linux.c b/coregrind/m_sigframe/sigframe-s390x-linux.c index 5210cebb04..b8a58949f6 100644 --- a/coregrind/m_sigframe/sigframe-s390x-linux.c +++ b/coregrind/m_sigframe/sigframe-s390x-linux.c @@ -267,7 +267,7 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) { + if (VG_(extend_stack)(tid, addr)) { stackseg = VG_(am_find_nsegment)(addr); if (0 && stackseg) VG_(printf)("frame=%#lx seg=%#lx-%#lx\n", diff --git a/coregrind/m_sigframe/sigframe-x86-linux.c b/coregrind/m_sigframe/sigframe-x86-linux.c index 5e255aef93..2c7c8d001c 100644 --- a/coregrind/m_sigframe/sigframe-x86-linux.c +++ b/coregrind/m_sigframe/sigframe-x86-linux.c @@ -402,7 +402,7 @@ static Bool extend ( ThreadState *tst, Addr addr, SizeT size ) ThreadId tid = tst->tid; NSegment const* stackseg = NULL; - if (VG_(extend_stack)(addr, tst->client_stack_szB)) { + if (VG_(extend_stack)(tid, addr)) { stackseg = VG_(am_find_nsegment)(addr); if (0 && stackseg) VG_(printf)("frame=%#lx seg=%#lx-%#lx\n", diff --git a/coregrind/m_signals.c b/coregrind/m_signals.c index 1cd3179d7b..4d0b96c19d 100644 --- a/coregrind/m_signals.c +++ b/coregrind/m_signals.c @@ -1,3 +1,4 @@ +/* -*- mode: C; c-basic-offset: 3; -*- */ /*--------------------------------------------------------------------*/ /*--- Implementation of POSIX signals. m_signals.c ---*/ @@ -1715,7 +1716,7 @@ static void default_action(const vki_siginfo_t *info, ThreadId tid) if (tid == 1) { // main thread Addr esp = VG_(get_SP)(tid); Addr base = VG_PGROUNDDN(esp - VG_STACK_REDZONE_SZB); - if (VG_(extend_stack)(base, VG_(threads)[tid].client_stack_szB)) { + if (VG_(extend_stack)(tid, base)) { if (VG_(clo_trace_signals)) VG_(dmsg)(" -> extended stack base to %#lx\n", VG_PGROUNDDN(esp)); @@ -2238,54 +2239,54 @@ void async_signalhandler ( Int sigNo, "while outside of scheduler"); } -/* Extend the stack to cover addr. maxsize is the limit the stack can grow to. +/* Extend the stack of thread #tid to cover addr. Returns True on success, False on failure. Succeeds without doing anything if addr is already within a segment. Failure could be caused by: - - addr not below a growable segment - - new stack size would exceed maxsize + - addr not below a growable segment or in a free segment + - new stack size would exceed the stack limit for the given thread - mmap failed for some other reason - */ -Bool VG_(extend_stack)(Addr addr, UInt maxsize) +*/ +Bool VG_(extend_stack)(ThreadId tid, Addr addr) { SizeT udelta; - /* Find the next Segment above addr */ - NSegment const* seg - = VG_(am_find_nsegment)(addr); - NSegment const* seg_next - = seg ? VG_(am_next_nsegment)( seg, True/*fwds*/ ) - : NULL; + /* Get the segment containing addr. */ + const NSegment* seg = VG_(am_find_nsegment)(addr); + if (seg == NULL) return False; // addr in a SkFree segment /* TODO: the test "seg->kind == SkAnonC" is really inadequate, because although it tests whether the segment is mapped _somehow_, it doesn't check that it has the right permissions (r,w, maybe x) ? */ - if (seg && seg->kind == SkAnonC) + if (seg->kind == SkAnonC) /* addr is already mapped. Nothing to do. */ return True; - /* Check that the requested new base is in a shrink-down - reservation section which abuts an anonymous mapping that - belongs to the client. */ - if ( ! (seg - && seg->kind == SkResvn - && seg->smode == SmUpper - && seg_next - && seg_next->kind == SkAnonC - && seg->end+1 == seg_next->start)) - return False; + /* Find the next Segment above addr. This will return NULL if ADDR + is bogus -- which it may be. See comment at the call site in function + VG_(client_syscall) */ + const NSegment* seg_next = VG_(am_next_nsegment)( seg, True/*fwds*/ ); + if (seg_next == NULL || seg_next->kind != SkAnonC) return False; udelta = VG_PGROUNDUP(seg_next->start - addr); + VG_(debugLog)(1, "signals", "extending a stack base 0x%llx down by %lld\n", (ULong)seg_next->start, (ULong)udelta); + Bool overflow; if (! VG_(am_extend_into_adjacent_reservation_client) - ( seg_next->start, -(SSizeT)udelta )) { - VG_(debugLog)(1, "signals", "extending a stack base: FAILED\n"); + ( seg_next->start, -(SSizeT)udelta, &overflow )) { + Addr new_stack_base = seg_next->start - udelta; + if (overflow) + VG_(umsg)("Stack overflow in thread #%d: can't grow stack to %#lx\n", + tid, new_stack_base); + else + VG_(umsg)("Cannot map memory to grow the stack for thread #%d " + "to %#lx\n", tid, new_stack_base); return False; } @@ -2407,7 +2408,6 @@ static Bool extend_stack_if_appropriate(ThreadId tid, vki_siginfo_t* info) Addr fault; Addr esp; NSegment const* seg; - NSegment const* seg_next; if (info->si_signo != VKI_SIGSEGV) return False; @@ -2415,8 +2415,6 @@ static Bool extend_stack_if_appropriate(ThreadId tid, vki_siginfo_t* info) fault = (Addr)info->VKI_SIGINFO_si_addr; esp = VG_(get_SP)(tid); seg = VG_(am_find_nsegment)(fault); - seg_next = seg ? VG_(am_next_nsegment)( seg, True/*fwds*/ ) - : NULL; if (VG_(clo_trace_signals)) { if (seg == NULL) @@ -2431,11 +2429,6 @@ static Bool extend_stack_if_appropriate(ThreadId tid, vki_siginfo_t* info) if (info->si_code == VKI_SEGV_MAPERR && seg - && seg->kind == SkResvn - && seg->smode == SmUpper - && seg_next - && seg_next->kind == SkAnonC - && seg->end+1 == seg_next->start && fault >= fault_mask(esp - VG_STACK_REDZONE_SZB)) { /* If the fault address is above esp but below the current known stack segment base, and it was a fault because there was @@ -2443,14 +2436,12 @@ static Bool extend_stack_if_appropriate(ThreadId tid, vki_siginfo_t* info) then extend the stack segment. */ Addr base = VG_PGROUNDDN(esp - VG_STACK_REDZONE_SZB); - if (VG_(extend_stack)(base, VG_(threads)[tid].client_stack_szB)) { + if (VG_(extend_stack)(tid, base)) { if (VG_(clo_trace_signals)) VG_(dmsg)(" -> extended stack base to %#lx\n", VG_PGROUNDDN(fault)); return True; } else { - VG_(umsg)("Stack overflow in thread %d: can't grow stack to %#lx\n", - tid, fault); return False; } } else { diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index be32e7a0f3..3ed96f5d81 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -1254,7 +1254,9 @@ static Addr do_brk ( Addr newbrk ) vg_assert(delta > 0); vg_assert(VG_IS_PAGE_ALIGNED(delta)); - if (! VG_(am_extend_into_adjacent_reservation_client)( aseg->start, delta )) + Bool overflow; // ignored here + if (! VG_(am_extend_into_adjacent_reservation_client)( aseg->start, delta, + &overflow)) goto bad; VG_(brk_limit) = newbrk; diff --git a/coregrind/m_syswrap/syswrap-main.c b/coregrind/m_syswrap/syswrap-main.c index 0d42d16f02..4aa0a03d23 100644 --- a/coregrind/m_syswrap/syswrap-main.c +++ b/coregrind/m_syswrap/syswrap-main.c @@ -1479,7 +1479,16 @@ void VG_(client_syscall) ( ThreadId tid, UInt trc ) if (tid == 1/*ROOT THREAD*/) { Addr stackMin = VG_(get_SP)(tid) - VG_STACK_REDZONE_SZB; - VG_(extend_stack)( stackMin, tst->client_stack_szB ); + /* Note, that the stack pointer can be bogus at this point. This is + extremely rare. A legitimate testcase that exercises this is + none/tests/s390x/stmg.c: The stack pointer happens to be in the + reservation segment near the end of the addressable memory and + there is no SkAnonC segment above. + + We could do slightly better here by not extending the stack for + system calls that do not access user space memory. That's busy + work with very little gain... */ + VG_(extend_stack)( tid, stackMin ); // may fail } # endif /* END ensure root thread's stack is suitably mapped */ diff --git a/coregrind/pub_core_aspacemgr.h b/coregrind/pub_core_aspacemgr.h index c156d4d314..529ed8cf9c 100644 --- a/coregrind/pub_core_aspacemgr.h +++ b/coregrind/pub_core_aspacemgr.h @@ -282,7 +282,7 @@ extern Bool VG_(am_create_reservation) the reservation segment after the operation must be at least one page long. The function returns a pointer to the resized segment. */ extern const NSegment *VG_(am_extend_into_adjacent_reservation_client) - ( Addr addr, SSizeT delta ); + ( Addr addr, SSizeT delta, /*OUT*/Bool *overflow ); /* --- --- --- resizing/move a mapping --- --- --- */ diff --git a/coregrind/pub_core_signals.h b/coregrind/pub_core_signals.h index dd92a2b938..1b1c33772a 100644 --- a/coregrind/pub_core_signals.h +++ b/coregrind/pub_core_signals.h @@ -81,7 +81,7 @@ extern void VG_(synth_sigbus) (ThreadId tid); extern void VG_(synth_sigfpe) (ThreadId tid, UInt code); /* Extend the stack to cover addr, if possible */ -extern Bool VG_(extend_stack)(Addr addr, UInt maxsize); +extern Bool VG_(extend_stack)(ThreadId tid, Addr addr); /* Forces the client's signal handler to SIG_DFL - generally just before using that signal to kill the process. */ diff --git a/none/tests/linux/Makefile.am b/none/tests/linux/Makefile.am index 04b6f7b4db..87f780ccfa 100644 --- a/none/tests/linux/Makefile.am +++ b/none/tests/linux/Makefile.am @@ -8,15 +8,16 @@ EXTRA_DIST = \ mremap.stderr.exp mremap.stderr.exp-glibc27 mremap.stdout.exp \ mremap.vgtest \ mremap2.stderr.exp mremap2.stdout.exp mremap2.vgtest \ - mremap3.stderr.exp mremap3.stdout.exp mremap3.vgtest + mremap3.stderr.exp mremap3.stdout.exp mremap3.vgtest \ + stack-overflow.stderr.exp stack-overflow.vgtest check_PROGRAMS = \ blockfault \ mremap \ mremap2 \ - mremap3 + mremap3 \ + stack-overflow AM_CFLAGS += $(AM_FLAG_M3264_PRI) AM_CXXFLAGS += $(AM_FLAG_M3264_PRI) - diff --git a/none/tests/linux/stack-overflow.c b/none/tests/linux/stack-overflow.c new file mode 100644 index 0000000000..86f9e4af8b --- /dev/null +++ b/none/tests/linux/stack-overflow.c @@ -0,0 +1,9 @@ +/* There should be a user message about the overflow. + Wrtten in a single line so there is no confusion on what line + the overflow occurs. */ + +int main(int argc, char *argv[]) \ +{ \ + volatile int arr[1000]; \ + return main(arr[argc%2], 0); \ +} diff --git a/none/tests/linux/stack-overflow.stderr.exp b/none/tests/linux/stack-overflow.stderr.exp new file mode 100644 index 0000000000..b1f26ac007 --- /dev/null +++ b/none/tests/linux/stack-overflow.stderr.exp @@ -0,0 +1,13 @@ + +Stack overflow in thread #1: can't grow stack to 0x........ + +Process terminating with default action of signal 11 (SIGSEGV) + Access not within mapped region at address 0x........ +Stack overflow in thread #1: can't grow stack to 0x........ + at 0x........: main (stack-overflow.c:6) + If you believe this happened as a result of a stack + overflow in your program's main thread (unlikely but + possible), you can try to increase the size of the + main thread stack using the --main-stacksize= flag. + The main thread stack size used in this run was .... + diff --git a/none/tests/linux/stack-overflow.vgtest b/none/tests/linux/stack-overflow.vgtest new file mode 100644 index 0000000000..3b9ffd49b6 --- /dev/null +++ b/none/tests/linux/stack-overflow.vgtest @@ -0,0 +1 @@ +prog: stack-overflow -- 2.47.3