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

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

On 2/11/19 4:22 PM, Florian Weimer wrote:
> * Andreas Schwab:
> 
>> On Feb 11 2019, Florian Weimer <fweimer@redhat.com> wrote:
>>
>>> 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);
>>
>> result and pd->result are not the same thing.
> 
> Oops.  What about this?
> 
> Or should I write essentially the same probe twice?  I don't want to
> introduce a new probe name because I want to backport this (along with
> the fix for bug 24164).
> 
> Thanks,
> Florian
> 
> nptl: Fix invalid Systemtap probe in pthread_join [BZ #24211]
> 
> 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 after the thread descriptor has been freed.
> 
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index ecb78ffba5..366feb376b 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -86,6 +86,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>        pthread_cleanup_pop (0);
>      }
>  
> +  void *pd_result = pd->result;
>    if (__glibc_likely (result == 0))
>      {
>        /* We mark the thread as terminated and as joined.  */
> @@ -93,7 +94,7 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
>  
>        /* Store the return value if the caller is interested.  */
>        if (thread_return != NULL)
> -	*thread_return = pd->result;
> +	*thread_return = pd_result;
>  
>        /* Free the TCB.  */
>        __free_tcb (pd);
> @@ -101,7 +102,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, pd_result);

What prevents the compiler from optimizing away pd_result and using
pd->result directly again?

The actions of __free_tcb() and their consequences are not visible
to the compiler.

Do we have to make pd_result volatile to avoid optimizations that
move that read? Or do we need a compiler barrier to ensure that
the read is complete before we free the stack?
  
>    return result;
>  }
> 


-- 
Cheers,
Carlos.