From 3168fa1e2a8914d9a3c71238eb39c267023b9526 Mon Sep 17 00:00:00 2001 From: Philippe Waroquiers Date: Sun, 21 Jul 2013 16:04:05 +0000 Subject: [PATCH] fix 321960 pthread_create() then alloca() causing invalid stack write errors Problem created by a discrepancy between the initial main stack anon segment, and the main stack registered in m_stacks.c Looking at some tracing; we see that there are two pages of stack: --9078:2:main tell tool about 0ffefff000-0fff000fff rw- The stack between the base and the current sp is marked as not accessible: --9078:2:main mark stack inaccessible 0ffefff000-0fff0004bf This is matching the aspacemgr view: --9078:1:aspacem 22: RSVN 0ffe801000-0ffeffefff 8380416 ----- SmUpper --9078:1:aspacem 23: anon 0ffefff000-0fff000fff 8192 rw--- (all the above is normal/as expected) However, the main stack is registered in m_stacks.c as having only one page: --9078:2:stacks register 0xFFF000000-0xFFF000FFF as stack 0 When the main stack is grown, m_stacks.c is informed by m_signals.c that the stack is grown. This is done by trapping the signal 11 when a not mapped page is accessed. However, the 2nd page does not cause a signal (as it is mapped). So, m_stacks.c still believes the main has one page stack. This then gives problems in the tracking of the SP and current_stack in m_stacks.c. Only one page was registered for the main stack, as the registration was done with values computed before possibly adding a page needed for the ABI redzone. The fix is to properly register the main stack with the size of the stack segment, once all aspects have been taken into account. With the fix, the stack is registered as: --31501:2:stacks register 0xFFEFFF000-0xFFF000FFF as stack 0 Another possible fix would be to always register the main stack with the full size of the aspacemgr stack segment (i.e. the anon+RSVN above) (idea is that this is similar to non main threads, for which the full thread stack is registered from the beginning, even if not fully used yet). The first fix was preferred, assuming it is better to keep registering the main stack "physical" size (and not its maximal size). Test memcheck/tests/thread_alloca added, based on reproducer done by Daniel Stodden. The bug might be triggered or not depending on the initial value of the SP, which is influenced by the size of the "env". So, the test execs itself, growing each time the environment. This has given a reasonable chance/way to reproduce the bug on Ubuntu 12 and on a Debian 6. (tested on amd64/Ubuntu 12 and Debian 6 x86/fedora12 ppc64/fedora18 Note that while investigating this bug, another strange thing was seen: thread stacks are registered in m_stacks.c but are never unregistered. It is not very clear that it is needed or not to unregister them: thread stack segments are not freed when a thread terminates : when a thread slot is re-used, its thread stack will also be re-used. (Is that good for address space mgt ? A process that has created many temporary threads will have the thread stacks lost forever ???). git-svn-id: svn://svn.valgrind.org/valgrind/trunk@13467 --- coregrind/m_clientstate.c | 4 ++ coregrind/m_initimg/initimg-linux.c | 9 +-- coregrind/m_stacks.c | 16 +++++- memcheck/tests/Makefile.am | 3 + memcheck/tests/thread_alloca.c | 73 +++++++++++++++++++++++++ memcheck/tests/thread_alloca.stderr.exp | 0 memcheck/tests/thread_alloca.vgtest | 2 + 7 files changed, 102 insertions(+), 5 deletions(-) create mode 100644 memcheck/tests/thread_alloca.c create mode 100644 memcheck/tests/thread_alloca.stderr.exp create mode 100644 memcheck/tests/thread_alloca.vgtest diff --git a/coregrind/m_clientstate.c b/coregrind/m_clientstate.c index 01292bbf4..5046c3b7c 100644 --- a/coregrind/m_clientstate.c +++ b/coregrind/m_clientstate.c @@ -44,7 +44,11 @@ /* Client address space, lowest to highest (see top of ume.c) */ // TODO: get rid of as many of these as possible. +/* ***Initial*** lowest address of the stack segment of the main thread. + The main stack will grow if needed but VG_(clstk_base) will + not be changed according to the growth. */ Addr VG_(clstk_base) = 0; +/* Initial highest address of the stack segment of the main thread. */ Addr VG_(clstk_end) = 0; UWord VG_(clstk_id) = 0; diff --git a/coregrind/m_initimg/initimg-linux.c b/coregrind/m_initimg/initimg-linux.c index 653ce1a9e..0d493ec2c 100644 --- a/coregrind/m_initimg/initimg-linux.c +++ b/coregrind/m_initimg/initimg-linux.c @@ -491,10 +491,6 @@ Addr setup_client_stack( void* init_sp, /* The max stack size */ clstack_max_size = VG_PGROUNDUP(clstack_max_size); - /* Record stack extent -- needed for stack-change code. */ - VG_(clstk_base) = clstack_start; - VG_(clstk_end) = clstack_end; - if (0) VG_(printf)("stringsize=%d auxsize=%d stacksize=%d maxsize=0x%x\n" "clstack_start %p\n" @@ -572,6 +568,11 @@ Addr setup_client_stack( void* init_sp, vg_assert(ok); vg_assert(!sr_isError(res)); + + /* Record stack extent -- needed for stack-change code. */ + VG_(clstk_base) = anon_start -inner_HACK; + VG_(clstk_end) = VG_(clstk_base) + anon_size +inner_HACK -1; + } /* ==================== create client stack ==================== */ diff --git a/coregrind/m_stacks.c b/coregrind/m_stacks.c index 21089fdc2..186d83f4a 100644 --- a/coregrind/m_stacks.c +++ b/coregrind/m_stacks.c @@ -37,6 +37,9 @@ #include "pub_core_stacks.h" #include "pub_core_tooliface.h" +// For expensive debugging +#define EDEBUG(fmt, args...) //VG_(debugLog)(2, "stacks", fmt, ## args) + /* The stack ~~~~~~~~~ @@ -310,6 +313,12 @@ static void complaints_stack_switch (Addr old_SP, Addr new_SP) #define IF_STACK_SWITCH_SET_current_stack_AND_RETURN \ Word delta = (Word)new_SP - (Word)old_SP; \ \ + EDEBUG("current_stack %p-%p %lu new_SP %p old_SP %p\n", \ + (void *) (current_stack ? current_stack->start : 0x0), \ + (void *) (current_stack ? current_stack->end : 0x0), \ + current_stack ? current_stack->id : 0, \ + (void *)new_SP, (void *)old_SP); \ + \ /* Check if the stack pointer is still in the same stack as before. */ \ if (UNLIKELY(current_stack == NULL || \ new_SP < current_stack->start || new_SP > current_stack->end)) { \ @@ -319,8 +328,13 @@ static void complaints_stack_switch (Addr old_SP, Addr new_SP) /* The stack pointer is now in another stack. Update the current */ \ /* stack information and return without doing anything else. */ \ current_stack = new_stack; \ + EDEBUG("new current_stack %p-%p %lu \n", \ + (void *) current_stack->start, \ + (void *) current_stack->end, \ + current_stack->id); \ return; \ - } \ + } else \ + EDEBUG("new current_stack not found\n"); \ } #define IF_BIG_DELTA_complaints_AND_RETURN \ diff --git a/memcheck/tests/Makefile.am b/memcheck/tests/Makefile.am index eaf64b47c..9511dc86d 100644 --- a/memcheck/tests/Makefile.am +++ b/memcheck/tests/Makefile.am @@ -225,6 +225,7 @@ EXTRA_DIST = \ test-plo-yes.vgtest test-plo-yes.stdout.exp \ test-plo-yes.stderr.exp-le64 test-plo-yes.stderr.exp-le32 \ test-plo-no.stderr.exp-s390x-mvc \ + thread_alloca.stderr.exp thread_alloca.vgtest \ trivialleak.stderr.exp trivialleak.vgtest trivialleak.stderr.exp2 \ undef_malloc_args.stderr.exp undef_malloc_args.vgtest \ unit_libcbase.stderr.exp unit_libcbase.vgtest \ @@ -321,6 +322,7 @@ check_PROGRAMS = \ supp_unknown supp1 supp2 suppfree \ test-plo \ trivialleak \ + thread_alloca \ undef_malloc_args \ unit_libcbase unit_oset \ varinfo1 varinfo2 varinfo3 varinfo4 \ @@ -360,6 +362,7 @@ dw4_CFLAGS = $(AM_CFLAGS) -gdwarf-4 -fdebug-types-section err_disable3_LDADD = -lpthread err_disable4_LDADD = -lpthread +thread_alloca_LDADD = -lpthread error_counts_CFLAGS = $(AM_CFLAGS) @FLAG_W_NO_UNINITIALIZED@ diff --git a/memcheck/tests/thread_alloca.c b/memcheck/tests/thread_alloca.c new file mode 100644 index 000000000..fa0c18558 --- /dev/null +++ b/memcheck/tests/thread_alloca.c @@ -0,0 +1,73 @@ +/* Reproduces bug 321960 (based on test from Daniel Stodden). + At least on Ubuntu 12 and 13, causes invalid write errors + in __yell or the memset call (due to some part of the main + stack being marked as not addressable in memcheck). + Bug seems extremely sensitive to initial conditions: + Depending on the size of the env, bug is triggered or not. + Also, a high nr of threads in thr[] is needed to get + the problem. */ +#include +#include +#include +#include +#include +#include +#include + +void * +nop(void *nil) +{ + return NULL; +} + +void +__yell(void) +{ + char buf[256]; + memset(buf, 0, sizeof(buf)); +} + +/* Without argument, executes once. + Otherwise first arg indicates nr of times the process will exec + itself, each time increasing the size of the environment + by about 50 characters. */ +int main(int argc, char **argv, char** envp) +{ + pthread_t thr[50]; + int i, err; + + for (i = 0; i < sizeof(thr) / sizeof(*thr); i++) { + err = pthread_create(&thr[i], NULL, nop, NULL); + assert(!err); + } + + alloca(4096); + __yell(); + + for (i = 0; i < sizeof(thr) / sizeof(*thr); i++) + pthread_join(thr[i], NULL); + + if ( argc == 2 && atoi(argv[1]) > 0) { + /* exec ourselves with some more env */ + char** new_env; + char more_env[100]; + char n[10]; + int j; + + sprintf(more_env, "N%d=ZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZZ", atoi(argv[1])); + for (j = 0; envp[j]; j++) + ; + new_env = malloc((j+2) * sizeof(char*)); + assert (new_env != NULL); + for (i = 0; i < j; i++) + new_env[i] = envp[i]; + new_env[i++] = more_env; + new_env[i++] = NULL; + assert(i == j+2); + sprintf (n, "%d", atoi(argv[1]) - 1); + // system ("env | wc"); + execle(argv[0], argv[0], n, NULL, new_env); + assert(0); + } else + return 0; +} diff --git a/memcheck/tests/thread_alloca.stderr.exp b/memcheck/tests/thread_alloca.stderr.exp new file mode 100644 index 000000000..e69de29bb diff --git a/memcheck/tests/thread_alloca.vgtest b/memcheck/tests/thread_alloca.vgtest new file mode 100644 index 000000000..6e7e38b43 --- /dev/null +++ b/memcheck/tests/thread_alloca.vgtest @@ -0,0 +1,2 @@ +prog: thread_alloca 30 +vgopts: -q --trace-children=yes