Fix an assertion in the address space manager. BZ #345887.

The VG_(extend_stack) call needs to be properly guarded because the
passed-in address is not necessarily part of an extensible stack
segment. And an extensible stack segment is the only thing that
function should have to deal with.
Previously, the function VG_(am_addr_is_in_extensible_client_stack)
was introduced to guard VG_(extend_stack) but it was not added in all
places it should have been.

Also, extending the client stack during signal delivery (in sigframe-common.c)
was simply calling VG_(extend_stack) hoping it would do the right thing.
But that was not always the case. The new testcase 
none/tests/linux/pthread-stack.c exercises this (3.10.1 errors out on it).

Renamed ML_(sf_extend_stack) to ML_(sf_maybe_extend_stack) and add
proper guard logic for VG_(extend_stack).

Testcases none/tests/{amd64|x86}-linux/bug345887.c by Ivo Raisr.


git-svn-id: svn://svn.valgrind.org/valgrind/trunk@15138 a5019735-40e9-0310-863c-91ae7b9d1cf9

NEWS

@@ -151,6 +151,7 @@ where XXXXXX is the bug number as listed below.
 345016  helgrind/tests/locked_vs_unlocked2 is failing sometimes
 345394  Fix memcheck/tests/strchr on OS X
 345637  Fix memcheck/tests/sendmsg on OS X
+345887  Fix an assertion in the address space manager
 346307  fuse filesystem syscall deadlocks
 n-i-bz  Provide implementations of certain compiler builtins to support
         compilers who may not provide those

configure.ac

@@ -3033,6 +3033,7 @@ AC_CONFIG_FILES([
    none/tests/tilegx/Makefile
    none/tests/linux/Makefile
    none/tests/darwin/Makefile
+   none/tests/amd64-linux/Makefile
    none/tests/x86-linux/Makefile
    exp-sgcheck/Makefile
    exp-sgcheck/tests/Makefile

coregrind/m_sigframe/priv_sigframe.h

@@ -37,7 +37,8 @@
 
 /* --------------- Implemented in sigframe-common.c ---------------*/
 
-Bool ML_(sf_extend_stack)( const ThreadState *tst, Addr addr, SizeT size );
+Bool ML_(sf_maybe_extend_stack)( const ThreadState *tst, Addr addr,
+                                 SizeT size, UInt flags );
 
 #endif   // __PRIV_SIGFRAME_H
 

coregrind/m_sigframe/sigframe-amd64-darwin.c

@@ -109,7 +109,7 @@ void VG_(sigframe_create) ( ThreadId tid,
                 entry to a function. */
 
    tst = VG_(get_ThreadState)(tid);
-   if (! ML_(sf_extend_stack)(tst, rsp, sp_top_of_frame - rsp))
+   if (! ML_(sf_maybe_extend_stack)(tst, rsp, sp_top_of_frame - rsp, flags))
       return;
 
    vg_assert(VG_IS_16_ALIGNED(rsp+8));

coregrind/m_sigframe/sigframe-amd64-linux.c

@@ -412,7 +412,7 @@ static Addr build_rt_sigframe(ThreadState *tst,
    rsp = VG_ROUNDDN(rsp, 16) - 8;
    frame = (struct rt_sigframe *)rsp;
 
-   if (! ML_(sf_extend_stack)(tst, rsp, sizeof(*frame)))
+   if (! ML_(sf_maybe_extend_stack)(tst, rsp, sizeof(*frame), flags))
       return rsp_top_of_frame;
 
    /* retaddr, siginfo, uContext fields are to be written */

coregrind/m_sigframe/sigframe-arm-linux.c

@@ -185,7 +185,7 @@ void VG_(sigframe_create)( ThreadId tid,
    sp -= size;
    sp = VG_ROUNDDN(sp, 16);
 
-   if(! ML_(sf_extend_stack)(tst, sp, size))
+   if (! ML_(sf_maybe_extend_stack)(tst, sp, size, flags))
       I_die_here; // XXX Incorrect behavior
 
 

coregrind/m_sigframe/sigframe-arm64-linux.c

@@ -173,7 +173,7 @@ void VG_(sigframe_create)( ThreadId tid,
    sp -= size;
    sp = VG_ROUNDDN(sp, 16);
 
-   if (! ML_(sf_extend_stack)(tst, sp, size))
+   if (! ML_(sf_maybe_extend_stack)(tst, sp, size, flags))
       return; // Give up.  No idea if this is correct
 
    struct rt_sigframe *rsf = (struct rt_sigframe *)sp;

coregrind/m_sigframe/sigframe-common.c

@@ -54,16 +54,31 @@ static void track_frame_memory ( Addr addr, SizeT size, ThreadId tid )
 /* Extend the stack segment downwards if needed so as to ensure the
    new signal frames are mapped to something.  Return a Bool
    indicating whether or not the operation was successful. */
-Bool ML_(sf_extend_stack) ( const ThreadState *tst, Addr addr, SizeT size )
+Bool ML_(sf_maybe_extend_stack) ( const ThreadState *tst, Addr addr,
+                                  SizeT size, UInt flags )
 {
    ThreadId        tid = tst->tid;
    const NSegment *stackseg = NULL;
 
-   if (VG_(extend_stack)(tid, addr)) {
+   if (flags & VKI_SA_ONSTACK) {
+      /* If the sigframe is allocated on an alternate stack, then we cannot
+         extend that stack. Nothing to do here. */
       stackseg = VG_(am_find_nsegment)(addr);
-      if (0 && stackseg)
-	 VG_(printf)("frame=%#lx seg=%#lx-%#lx\n",
-		     addr, stackseg->start, stackseg->end);
+   } else if (VG_(am_addr_is_in_extensible_client_stack)(addr)) {
+      if (VG_(extend_stack)(tid, addr)) {
+         stackseg = VG_(am_find_nsegment)(addr);
+         if (0 && stackseg)
+            VG_(printf)("frame=%#lx seg=%#lx-%#lx\n",
+                        addr, stackseg->start, stackseg->end);
+      }
+   } else if ((stackseg = VG_(am_find_nsegment)(addr)) &&
+              VG_(am_is_valid_for_client)(addr, 1,
+                                          VKI_PROT_READ | VKI_PROT_WRITE)) {
+      /* We come here for explicitly defined pthread-stacks which can be
+         located in any client segment. */
+   } else {
+      /* Something unexpected */
+      stackseg = NULL;
    }
 
    if (stackseg == NULL || !stackseg->hasR || !stackseg->hasW) {

coregrind/m_sigframe/sigframe-mips32-linux.c

@@ -148,7 +148,7 @@ void VG_(sigframe_create)( ThreadId tid,
     }
 
   tst = VG_(get_ThreadState)(tid);
-  if (! ML_(sf_extend_stack)(tst, sp, sp_top_of_frame - sp))
+  if (! ML_(sf_maybe_extend_stack)(tst, sp, sp_top_of_frame - sp, flags))
     return;
 
   vg_assert(VG_IS_8_ALIGNED(sp));

coregrind/m_sigframe/sigframe-mips64-linux.c

@@ -135,7 +135,7 @@ void VG_(sigframe_create) ( ThreadId tid,
    sp = sp_top_of_frame - sizeof(struct rt_sigframe);
 
    tst = VG_(get_ThreadState)(tid);
-   if (! ML_(sf_extend_stack)(tst, sp, sp_top_of_frame - sp))
+   if (! ML_(sf_maybe_extend_stack)(tst, sp, sp_top_of_frame - sp, flags))
       return;
 
    sp = VG_ROUNDDN(sp, 16);

coregrind/m_sigframe/sigframe-ppc32-linux.c

@@ -650,7 +650,7 @@ void VG_(sigframe_create)( ThreadId tid,
 
    tst = VG_(get_ThreadState)(tid);
 
-   if (! ML_(sf_extend_stack)(tst, sp, sp_top_of_frame - sp))
+   if (! ML_(sf_maybe_extend_stack)(tst, sp, sp_top_of_frame - sp, flags))
       return;
 
    vg_assert(VG_IS_16_ALIGNED(sp));

coregrind/m_sigframe/sigframe-ppc64-linux.c

@@ -158,7 +158,7 @@ void VG_(sigframe_create)( ThreadId tid,
    sp = sp_top_of_frame - sizeof(struct rt_sigframe);
 
    tst = VG_(get_ThreadState)(tid);
-   if (! ML_(sf_extend_stack)(tst, sp, sp_top_of_frame - sp))
+   if (! ML_(sf_maybe_extend_stack)(tst, sp, sp_top_of_frame - sp, flags))
       return;
 
    vg_assert(VG_IS_16_ALIGNED(sp));

coregrind/m_sigframe/sigframe-s390x-linux.c

@@ -298,7 +298,7 @@ static Addr build_sigframe(ThreadState *tst,
    sp -= sizeof(*frame);
    frame = (struct sigframe *)sp;
 
-   if (! ML_(sf_extend_stack)(tst, sp, sizeof(*frame)))
+   if (! ML_(sf_maybe_extend_stack)(tst, sp, sizeof(*frame), flags))
       return sp_top_of_frame;
 
    /* retcode, sigNo, sc, sregs fields are to be written */
@@ -358,7 +358,7 @@ static Addr build_rt_sigframe(ThreadState *tst,
    sp -= sizeof(*frame);
    frame = (struct rt_sigframe *)sp;
 
-   if (! ML_(sf_extend_stack)(tst, sp, sizeof(*frame)))
+   if (! ML_(sf_maybe_extend_stack)(tst, sp, sizeof(*frame), flags))
       return sp_top_of_frame;
 
    /* retcode, sigNo, sc, sregs fields are to be written */

coregrind/m_sigframe/sigframe-tilegx-linux.c

@@ -158,7 +158,7 @@ void VG_(sigframe_create)( ThreadId tid,
   sp = sp_top_of_frame - sizeof(struct rt_sigframe);
 
   tst = VG_(get_ThreadState)(tid);
-  if (! ML_(sf_extend_stack)(tst, sp, sizeof(struct rt_sigframe)))
+  if (! ML_(sf_maybe_extend_stack)(tst, sp, sizeof(struct rt_sigframe), flags))
     return;
 
   vg_assert(VG_IS_8_ALIGNED(sp));

coregrind/m_sigframe/sigframe-x86-darwin.c

@@ -112,7 +112,7 @@ void VG_(sigframe_create) ( ThreadId tid,
                 entry to a function. */
 
    tst = VG_(get_ThreadState)(tid);
-   if (! ML_(sf_extend_stack)(tst, esp, sp_top_of_frame - esp))
+   if (! ML_(sf_maybe_extend_stack)(tst, esp, sp_top_of_frame - esp, flags))
       return;
 
    vg_assert(VG_IS_16_ALIGNED(esp+4));

coregrind/m_sigframe/sigframe-x86-linux.c

@@ -434,7 +434,7 @@ static Addr build_sigframe(ThreadState *tst,
    esp = VG_ROUNDDN(esp, 16);
    frame = (struct sigframe *)esp;
 
-   if (! ML_(sf_extend_stack)(tst, esp, sizeof(*frame)))
+   if (! ML_(sf_maybe_extend_stack)(tst, esp, sizeof(*frame), flags))
       return esp_top_of_frame;
 
    /* retaddr, sigNo, siguContext fields are to be written */
@@ -491,7 +491,7 @@ static Addr build_rt_sigframe(ThreadState *tst,
    esp = VG_ROUNDDN(esp, 16);
    frame = (struct rt_sigframe *)esp;
 
-   if (! ML_(sf_extend_stack)(tst, esp, sizeof(*frame)))
+   if (! ML_(sf_maybe_extend_stack)(tst, esp, sizeof(*frame), flags))
       return esp_top_of_frame;
 
    /* retaddr, sigNo, pSiginfo, puContext fields are to be written */

coregrind/m_signals.c

@@ -1737,7 +1737,8 @@ 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)(tid, base)) {
+            if (VG_(am_addr_is_in_extensible_client_stack)(base) &&
+                VG_(extend_stack)(tid, base)) {
                if (VG_(clo_trace_signals))
                   VG_(dmsg)("       -> extended stack base to %#lx\n",
                             VG_PGROUNDDN(esp));
@@ -2462,7 +2463,8 @@ 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)(tid, base)) {
+      if (VG_(am_addr_is_in_extensible_client_stack)(base) &&
+          VG_(extend_stack)(tid, base)) {
          if (VG_(clo_trace_signals))
             VG_(dmsg)("       -> extended stack base to %#lx\n",
                       VG_PGROUNDDN(fault));

none/tests/Makefile.am

@@ -45,12 +45,15 @@ SUBDIRS += darwin
 endif
 
 # Platform-specific tests
+if VGCONF_PLATFORMS_INCLUDE_AMD64_LINUX
+SUBDIRS += amd64-linux
+endif
 if VGCONF_PLATFORMS_INCLUDE_X86_LINUX
 SUBDIRS += x86-linux
 endif
 
 DIST_SUBDIRS = x86 amd64 ppc32 ppc64 arm arm64 s390x mips32 mips64 tilegx \
-               linux darwin x86-linux scripts .
+               linux darwin amd64-linux x86-linux scripts .
 
 dist_noinst_SCRIPTS = \
 	filter_cmdline0 \

none/tests/amd64-linux/Makefile.am

@@ -0,0 +1,15 @@
+
+include $(top_srcdir)/Makefile.tool-tests.am
+
+dist_noinst_SCRIPTS = \
+	filter_stderr filter_minimal
+
+EXTRA_DIST = \
+	bug345887.stderr.exp bug345887.vgtest
+
+check_PROGRAMS = \
+	bug345887
+
+AM_CFLAGS    += @FLAG_M64@
+AM_CXXFLAGS  += @FLAG_M64@
+AM_CCASFLAGS += @FLAG_M64@

none/tests/amd64-linux/bug345887.c

@@ -0,0 +1,42 @@
+/* This test used to cause an assertion in the address space manager */
+
+__attribute__((noinline))
+static void inner(void)
+{
+   /* Set registers to apriori known values. */
+   __asm__ __volatile__(
+      "movq $0x101, %%rax\n"
+      "movq $0x102, %%rbx\n"
+      "movq $0x103, %%rcx\n"
+      "movq $0x104, %%rdx\n"
+      "movq $0x105, %%rsi\n"
+      "movq $0x106, %%rdi\n"
+      "movq $0x107, %%r8\n"
+      "movq $0x108, %%r9\n"
+      "movq $0x109, %%r10\n"
+      "movq $0x10a, %%r11\n"
+      "movq $0x10b, %%r12\n"
+      "movq $0x10c, %%r13\n"
+      "movq $0x10d, %%r14\n"
+      "movq $0x10e, %%r15\n"
+      // not %rbp as mdb is then not able to reconstruct stack trace
+      "movq $0x10f, %%rsp\n"
+      "movq $0x1234, (%%rax)\n"  // should cause SEGV here
+      "ud2"                      // should never get here
+      : // no output registers
+      : // no input registers
+      : "memory", "%rax", "%rbx", "%rcx", "%rdx", "%rsi", "%rdi",
+        "%r8", "%r9", "%r10", "%r11", "%r12", "%r13", "%r14", "%r15", "%rsp");
+}
+
+__attribute__((noinline))
+static void outer(void)
+{
+   inner();
+}
+
+int main(int argc, const char *argv[])
+{
+   outer();
+   return 0;
+}

none/tests/amd64-linux/bug345887.stderr.exp

@@ -0,0 +1,9 @@
+
+Process terminating with default action of signal 11 (SIGSEGV)
+ Access not within mapped region at address 0x........
+   at 0x........: inner (bug345887.c:7)
+ 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 ....

none/tests/amd64-linux/bug345887.vgtest

@@ -0,0 +1,4 @@
+prog: bug345887
+vgopts: -q
+stderr_filter: filter_minimal
+cleanup: rm -f vgcore.*

none/tests/amd64-linux/filter_minimal

@@ -0,0 +1,20 @@
+#! /bin/sh
+
+dir=`dirname $0`
+
+# Remove ==pid== and **pid** strings 
+perl -p -e 's/(==|\*\*)[0-9]{1,7}\1 //' |
+
+perl -p -e 's/0x[0-9A-Fa-f]+/0x......../g' |
+
+# Older bash versions print abnormal termination messages on the stderr
+# of the bash process. Newer bash versions redirect such messages properly.
+# Suppress any redirected abnormal termination messages. You can find the
+# complete list of messages in the bash source file siglist.c.
+perl -n -e 'print if !/^(Segmentation fault|Alarm clock|Aborted|Bus error)( \(core dumped\))?$/' |
+
+# Remove the size in "The main thread stack size..." message.
+sed "s/The main thread stack size used in this run was [0-9]*/The main thread stack size used in this run was .../"
+
+# NOTE: it is essential for the bug345887 testcase that the stderr
+#       filtering does *not* remove lines beginning with --

none/tests/amd64-linux/filter_stderr

@@ -0,0 +1,11 @@
+#! /bin/sh
+
+dir=`dirname $0`
+
+# Remove ==pid== and --pid-- and **pid** strings 
+perl -p -e 's/(==|--|\*\*)[0-9]{1,7}\1 //' |
+
+perl -p -e 's/0x[0-9A-Fa-f]+/0x......../g'
+
+# NOTE: it is essential for the bug345887 testcase that the stderr
+#       filtering does *not* remove lines beginning with --

none/tests/linux/Makefile.am

@@ -9,15 +9,20 @@ EXTRA_DIST = \
 	    mremap.vgtest \
 	mremap2.stderr.exp mremap2.stdout.exp mremap2.vgtest \
 	mremap3.stderr.exp mremap3.stdout.exp mremap3.vgtest \
+	pthread-stack.stderr.exp pthread-stack.vgtest \
 	stack-overflow.stderr.exp stack-overflow.vgtest
 
 check_PROGRAMS = \
 	blockfault \
 	mremap \
 	mremap2 \
 	mremap3 \
+	pthread-stack \
 	stack-overflow
 
 
 AM_CFLAGS   += $(AM_FLAG_M3264_PRI)
 AM_CXXFLAGS += $(AM_FLAG_M3264_PRI)
+
+# Special needs
+pthread_stack_LDADD = -lpthread