Re: [PATCH][BZ 21672] fix pthread_create crash in ia64

[Sergei Trofimovich <slyfox at gentoo dot org>]

On Wed, 2 Aug 2017 22:26:45 +0100
Sergei Trofimovich <slyfox@gentoo.org> wrote:

> On Wed, 2 Aug 2017 17:59:57 -0300
> Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
> > On 02/08/2017 17:53, Sergei Trofimovich wrote:  
> > > On Wed, 5 Jul 2017 22:47:17 +0100
> > > Sergei Trofimovich <slyfox@gentoo.org> wrote:
> > >     
> > >> On Sun, 25 Jun 2017 23:07:15 +0100
> > >> Sergei Trofimovich <slyfox@gentoo.org> wrote:
> > >>    
> > >>> Minimal reproducer:
> > >>>
> > >>>     #include <pthread.h>
> > >>>
> > >>>     static void * f (void * p) { return NULL; }
> > >>>
> > >>>     int main (int argc, const char ** argv) {
> > >>>         pthread_t t;
> > >>>         pthread_create (&t, NULL, &f, NULL);
> > >>>
> > >>>         pthread_join (t, NULL);
> > >>>         return 0;
> > >>>     }
> > >>>
> > >>>     $ gcc -O0 -ggdb3 -o r bug.c -pthread && ./r
> > >>>
> > >>>     Program terminated with signal SIGSEGV, Segmentation fault.
> > >>>     #0  0x2000000000077da0 in start_thread (arg=0x0) at pthread_create.c:432
> > >>>     432         __madvise (pd->stackblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED);
> > >>>
> > >>> Here crash happens right after attempt to free unused part of
> > >>> thread's stack.
> > >>>
> > >>> On most architectures stack grows only down or grows only up.
> > >>> And there glibc decides which of unused ends of stack blocks can be freed.
> > >>>
> > >>> ia64 maintans two stacks. Both of them grow from the opposite directions:
> > >>>  - normal "sp" stack (stack for local variables) grows down
> > >>>  - register stack "bsp" grows up from the opposite end of stack block
> > >>>
> > >>> In this failure case we have prematurely freed "rsp" stack.
> > >>>
> > >>> The change leaves a few pages from both sides of stack block.
> > >>>
> > >>> Bug: https://sourceware.org/PR21672
> > >>> Bug: https://bugs.gentoo.org/622694
> > >>> Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
> > >>> ---
> > >>>  nptl/pthread_create.c | 18 ++++++++++++++++--
> > >>>  1 file changed, 16 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> > >>> index 7a970ffc5b..6e3f6db5b1 100644
> > >>> --- a/nptl/pthread_create.c
> > >>> +++ b/nptl/pthread_create.c
> > >>> @@ -555,10 +555,24 @@ START_THREAD_DEFN
> > >>>    size_t pagesize_m1 = __getpagesize () - 1;
> > >>>  #ifdef _STACK_GROWS_DOWN
> > >>>    char *sp = CURRENT_STACK_FRAME;
> > >>> -  size_t freesize = (sp - (char *) pd->stackblock) & ~pagesize_m1;
> > >>> +  char *freeblock = (char *) pd->stackblock;
> > >>> +  size_t freesize = (sp - freeblock) & ~pagesize_m1;
> > >>>    assert (freesize < pd->stackblock_size);
> > >>> +# ifdef __ia64__
> > >>>    if (freesize > PTHREAD_STACK_MIN)
> > >>> -    __madvise (pd->stackblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED);
> > >>> +    {
> > >>> +      /* On ia64 stack grows both ways!
> > >>> +         - normal "sp" stack (stack for local variables) grows down
> > >>> +         - register stack "bsp" grows up from the opposite end of stack block
> > >>> +
> > >>> +         Thus we leave PTHREAD_STACK_MIN bytes from stack block top
> > >>> +         and leave same PTHREAD_STACK_MIN at stack block bottom.  */
> > >>> +      freeblock += PTHREAD_STACK_MIN;
> > >>> +      freesize -= PTHREAD_STACK_MIN;
> > >>> +    }
> > >>> +# endif
> > >>> +  if (freesize > PTHREAD_STACK_MIN)
> > >>> +    __madvise (freeblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED);
> > >>>  #else
> > >>>    /* Page aligned start of memory to free (higher than or equal
> > >>>       to current sp plus the minimum stack size).  */
> > >>> -- 
> > >>> 2.13.1
> > >>>       
> > >>
> > >> Ping :)    
> > > 
> > > Ping^2
> > >     
> > 
> > What about https://sourceware.org/ml/libc-alpha/2017-07/msg00302.html ?  
> 
> Oh, I didn't get that email back in my inbox. Fetching from UI.
> Will testboth in SKI and real machine and report back in that thread.
> 
> [preliminary] Fix looks good to me.
> 
> Thank you!

Got to testing your patch today on glibc-master.

The traces below are traces of 'nptl/tst-align' glibc test.

Without your patch pthread_create() returns an error due to
guard page being way off:

    4645  mmap2(NULL, 8388608, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0x2000000000334000
    4645  mprotect(0x2000000000734000, 8372224, PROT_READ|PROT_WRITE) = -1 ENOMEM (Cannot allocate memory)
    4645  munmap(0x2000000000334000, 8388608) = 0

With your patch thread is being created fine!
But crashes at pthread_join() shutdown (the same way I've reported originally).

    // pthread_create
    5315  mmap2(NULL, 8388608, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0x2000000000334000
    5315  mprotect(0x2000000000334000, 4177920, PROT_READ|PROT_WRITE) = 0
    5315  mprotect(0x2000000000734000, 4194304, PROT_READ|PROT_WRITE) = 0
    ...
    // pthread_join
    5316  madvise(0x2000000000334000, 8175616, MADV_DONTNEED) = 0
    5316  --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_ACCERR, si_addr=0x8} ---
    5315  <... futex resumed>)              = ?
    5316  +++ killed by SIGSEGV +++

-- 

  Sergei

Attachment: pgpevef6d4P7t.pgp
Description: Цифровая подпись OpenPGP