Re: [PATCH] nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211]

[Carlos O'Donell <carlos at redhat dot com>]

On 2/11/19 3:54 PM, Florian Weimer wrote:
> After commit f1ac7455831546e5dca0ed98fe8af2686fae7ce6 ("arm: Use "nr"
> constraint for Systemtap probes [BZ #24164]"), we load pd->result into
> a register in the probe below:
> 
>       /* Free the TCB.  */
>       __free_tcb (pd);
>     }
>   else
>     pd->joinid = NULL;
> 
>   LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);
> 
> However, at this point, the thread descriptor has been freed.  If the
> thread stack does not fit into the thread stack cache, the memory will
> have been unmapped, and the program will crash in the probe.
> 
> 2019-02-11  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24211]
> 	* nptl/pthread_join_common.c (__pthread_timedjoin_ex): Do not read
> 	pd->result again after the thread descriptor has been freed.
> 
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index ecb78ffba5..45deba6a74 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -101,7 +101,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>    else
>      pd->joinid = NULL;
>  
> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, result);

Why not move the probe up?

 77   if (block)
 78     {
 79       /* If abstime is NULL we switch to asynchronous cancellation.  If we
 80          are cancelled then the thread we are waiting for must be marked as
 81          un-wait-ed for again.  */
 82       pthread_cleanup_push (cleanup, &pd->joinid);
 83 
 84       result = lll_wait_tid (pd->tid, abstime);
 85 
 86       pthread_cleanup_pop (0);
 87     }
 88 

LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
^^^^

At this point the join is complete.

We have the threadid (constant).

We have the result (just returned from lll_wait_tid).

We have pd->result (the thread returned).

We aren't waiting for anything else?

 89   if (__glibc_likely (result == 0))
 90     {
 91       /* We mark the thread as terminated and as joined.  */
 92       pd->tid = -1;
 93 
 94       /* Store the return value if the caller is interested.  */
 95       if (thread_return != NULL)
 96         *thread_return = pd->result;
 97 
 98       /* Free the TCB.  */
 99       __free_tcb (pd);
100     }
101   else
102     pd->joinid = NULL;
103 
104   LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);


>  
>    return result;
>  }
> 

As an orthogonal issue the probe needs documenting in manual/probes.texi
under a new "POSIX Thread Probes" section ;-)

-- 
Cheers,
Carlos.