From: Florian Krohm <florian@eich-krohm.de>
Date: Mon, 23 Mar 2015 17:13:04 +0000 (+0000)
Subject: Add VG_(am_is_bogus_client_stack_pointer)(Addr).
X-Git-Tag: svn/VALGRIND_3_11_0~558
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e7674cfdbbd0a2105aaea413b6c7e161ec7b7c06;p=thirdparty%2Fvalgrind.git

Add VG_(am_is_bogus_client_stack_pointer)(Addr).
The function is used in VG_(client_syscall) to avoid extending the stack
when it is clear that the current value of the stack pointer does not
point into a segment that looks like a stack segment.
See the comments in the code there.
As a side effect of this we can now revert r15018 which increased
the stack size of the alternate stack in memcheck/tests/sigaltstack.c.
The reason is that the belief at the time: "alternate stack is too small"
was not correct. What instead happened was that VG_(client_syscall) called
VG_(extend_stack) without need (the syscall was tgkill) and the new stack
pointer happened to be in a file segment.
In other words: the current stack pointer was still within the alternate
stack, i.e. the alternate stack was (barely) large enough.


git-svn-id: svn://svn.valgrind.org/valgrind/trunk@15034
---

diff --git a/coregrind/m_aspacemgr/aspacemgr-linux.c b/coregrind/m_aspacemgr/aspacemgr-linux.c
index 936f9b01ab..2c750a8c21 100644
--- a/coregrind/m_aspacemgr/aspacemgr-linux.c
+++ b/coregrind/m_aspacemgr/aspacemgr-linux.c
@@ -1357,6 +1357,50 @@ static Bool any_Ts_in_range ( Addr start, SizeT len )
 }
 
 
+/* Check whether ADDR looks like a bogus stack pointer. Non-bogosity is
+   defined as follows: ADDR is not bogus if
+   (1) it points into an already mapped stack segment, OR
+   (2) it points into a reservation segment into which an abutting SkAnonC
+       segment can be extended. */
+Bool VG_(am_is_bogus_client_stack_pointer)( Addr addr )
+{
+   const NSegment *seg = nsegments + find_nsegment_idx(addr);
+
+   switch (seg->kind) {
+   case SkFree:
+   case SkAnonV:
+   case SkFileV:
+   case SkFileC:
+   case SkShmC:
+      return True;
+
+   case SkResvn: {
+      if (seg->smode != SmUpper) return True;
+      /* If the the abutting segment towards higher addresses is an SkAnonC
+         segment, then ADDR is a future stack pointer. */
+      const NSegment *next = VG_(am_next_nsegment)(seg, /*forward*/ True);
+      if (next == NULL || next->kind != SkAnonC) return True;
+
+      /* OK; looks like a stack segment */
+      return False;
+   }
+
+   case SkAnonC: {
+      /* If the abutting segment towards lower addresses is an SkResvn
+         segment, then ADDR is a stack pointer into mapped memory. */
+      const NSegment *next = VG_(am_next_nsegment)(seg, /*forward*/ False);
+      if (next == NULL || next->kind != SkResvn || seg->smode != SmUpper)
+         return True;
+
+      /* OK; looks like a stack segment */
+      return False;
+   }
+
+   default:
+      aspacem_assert(0);   // should never happen
+   }
+}
+
 /*-----------------------------------------------------------------*/
 /*---                                                           ---*/
 /*--- Modifying the segment array, and constructing segments.   ---*/
diff --git a/coregrind/m_signals.c b/coregrind/m_signals.c
index 4d0b96c19d..4c34c05d49 100644
--- a/coregrind/m_signals.c
+++ b/coregrind/m_signals.c
@@ -2239,14 +2239,16 @@ void async_signalhandler ( Int sigNo,
                    "while outside of scheduler");
 }
 
-/* Extend the stack of thread #tid to cover addr.
+/* Extend the stack of thread #tid to cover addr. It is expected that
+   addr either points into an already mapped anonymous segment or into a
+   reservation segment abutting the stack segment. Everything else is a bug.
 
    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 or in a free segment
+   - addr not below a growable segment
    - new stack size would exceed the stack limit for the given thread
    - mmap failed for some other reason
 */
@@ -2256,7 +2258,7 @@ Bool VG_(extend_stack)(ThreadId tid, Addr addr)
 
    /* Get the segment containing addr. */
    const NSegment* seg = VG_(am_find_nsegment)(addr);
-   if (seg == NULL) return False;   // addr in a SkFree segment
+   vg_assert(seg != NULL);
 
    /* TODO: the test "seg->kind == SkAnonC" is really inadequate,
       because although it tests whether the segment is mapped
@@ -2266,11 +2268,8 @@ Bool VG_(extend_stack)(ThreadId tid, Addr addr)
       /* addr is already mapped.  Nothing to do. */
       return True;
 
-   /* 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;
+   vg_assert(seg_next != NULL);
 
    udelta = VG_PGROUNDUP(seg_next->start - addr);
 
diff --git a/coregrind/m_syswrap/syswrap-main.c b/coregrind/m_syswrap/syswrap-main.c
index 04e52b6bba..d9c24a5000 100644
--- a/coregrind/m_syswrap/syswrap-main.c
+++ b/coregrind/m_syswrap/syswrap-main.c
@@ -1485,16 +1485,22 @@ void VG_(client_syscall) ( ThreadId tid, UInt trc )
    if (tid == 1/*ROOT THREAD*/) {
       Addr     stackMin   = VG_(get_SP)(tid) - VG_STACK_REDZONE_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
+      /* The precise thing to do here would be to extend the stack only
+         if the system call can be proven to access unmapped user stack
+         memory. That is an enormous amount of work even if a proper
+         spec of system calls was available. 
+
+         In the case where the system call does not access user memory
+         the stack pointer here can have any value. 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.
+
+         So the approximation we're taking here is to extend the stack only
+         if the client stack pointer does not look bogus. */
+      if (! VG_(am_is_bogus_client_stack_pointer)(stackMin))
+         VG_(extend_stack)( tid, stackMin );
    }
 #  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 01dbff9c15..fde0f3262f 100644
--- a/coregrind/pub_core_aspacemgr.h
+++ b/coregrind/pub_core_aspacemgr.h
@@ -87,6 +87,9 @@ extern Bool VG_(am_is_valid_for_valgrind)
 extern Bool VG_(am_is_valid_for_client_or_free_or_resvn)
    ( Addr start, SizeT len, UInt prot );
 
+/* Check whether ADDR looks like a bogus stack pointer. */
+extern Bool VG_(am_is_bogus_client_stack_pointer)( Addr addr );
+
 /* Trivial fn: return the total amount of space in anonymous mappings,
    both for V and the client.  Is used for printing stats in
    out-of-memory messages. */
diff --git a/memcheck/tests/sigaltstack.c b/memcheck/tests/sigaltstack.c
index 8cc6956c21..526a99aa5b 100644
--- a/memcheck/tests/sigaltstack.c
+++ b/memcheck/tests/sigaltstack.c
@@ -14,7 +14,7 @@ int main(int argv, char** argc) {
   int res, i;
   stack_t sigstk;
   struct sigaction act;
-  static const int size = SIGSTKSZ*4;
+  static const int size = SIGSTKSZ*2;
   // We give EXEC permissions because this won't work on ppc32 unless you
   // ask for an alt stack with EXEC permissions,
   // since signal returning requires execution of code on the stack.