[patch v1] nptl: namespace-safe pthread keys implementation

[Adhemerval Zanella Netto]

On 12/05/26 22:00, DJ Delorie wrote:
> 
> Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>> The old design stored a generation sequence number in both the global slot and
>> the per-thread slot (KEY_UNUSED and KEY_USABLE). When a key was deleted and its
>> slot reused, per-thread values from the old key failed the sequence-number check
>> in pthread_getspecific and deallocate_tsd, and were silently ignored.
> 
> Were the ramifications of this memory leak fully understood?

But this new scheme adds a different race scenario:

1. Thread 1 issues setspecific(K, v).
2. Thread 2 issues pthread_key_delete(K) then pthread_key_create(&K') reusing
   the same slot/bucket, registering destructor destr'.
3. T1 exits, deallocate_tsd sees destr' at the slot and T1's per-thread keys[i] == v,
   calling destr'(v) — wrong destructor + wrong data.

The mitigating walk in *_create clears per-thread slots in threads on 
dl_stack_used/dl_stack_user, but only at the *create* moment; a thread that does 
pthread_setspecific between the delete and the create (legal? technically undefined 
per POSIX) is fine, but any in-flight value escaping the walk is mis-destroyed.

So current code leaks (POSIX-permitted, deterministic), while this proposal adds
an undefined behavior on stale data (calls wrong destructor with wrong-type pointer).
I am not sure if this is an improvement.

Maybe a better alternative would be to split the patch in two, one that fixes the
*namespace* share issue (move __pthread_keys[PTHREAD_KEYS_MAX] from libc into 
GL(dl_pthread_keys), nptl_db keeps working with just a symbol-relocation fix),
and then *another* to fix the scalability issue.

I am not sure if we can get away from the sequence number, not without adding
potential other race issues. Maybe we can a different sentinel value, where 
pthread_key_delete sets stating that this bucket is being freed, but deallocate_tsd
is not being called yet (so it will be moved to the free-slots only when the thread
finishes).  I need to think more about it.

Another potential issue which I just realized is destructors can call setspecific,
so it would be good to check the new scheme with this scenario.

> 
>>> +		  for (i = 0; i <slots; i ++)
>>> +		    {
>>> +		      void (*d)(void *) = atomic_load_relaxed (&destr_buckets[b]->keys[i]);
>>> +		      if (d != PTHREAD_KEY_SLOT_FREE && d != NULL
>>> +			  && bucket->keys[i] != NULL)
>>
>> Wouldn't this skip keys created with dest == NULL? I think
>> get_cached_stack might reuses it for a new thread without calling
>> _pthread_key_init.
> 
> We can't call a NULL destructor.  The only side effect would be that a
> key with a NULL destructor doesn't get cleared, but that also doesn't
> set specific_used, so the loop would ignore it a fixed number of times
> before the non-NULL-destructor keys are all destructed.  Then we unmap
> the memory, so all trace of the non-cleared keys vanishes.
> 
> Unless the destructor for one key calls setspecific for a different
> key... but the destructor is only called on thread exit, so why would
> it?

Does every reuse goes through allocate_stack which now unconditionally calls 
_pthread_key_init, even for stack from the cache (get_cached_stack)?

> 
>>>  int
>>>  ___pthread_key_create (pthread_key_t *key, void (*destr) (void *))
>>>  {
>>> -  /* Find a slot in __pthread_keys which is unused.  */
>>> -  for (size_t cnt = 0; cnt < PTHREAD_KEYS_MAX; ++cnt)
>>> +  int b, i;
>>> +  list_t *runp;
>>> +
>>> +  PTHREAD_KEY_LOCK;
>>> +  struct pthread_key_bucket **buckets = GL(dl_pthread_keys_data);
>>> +  if (buckets == NULL)
>>
>> I think this does not address the namespace issue you are trying to
>> fix.  The __pthread_key_lock is defined as attribute_hidden in libc.so
>> and when multiple libc instances are loaded with dlmopen, each
>> namespace has its own copy of the lock.  However, they all share the
>> same key table in ldso via GL(dl_pthread_keys_data).
> 
> Right, move lock to ld.so too...
> 
>>> -  if (__glibc_likely (key < PTHREAD_KEYS_MAX))
>>> +  if (buckets[b] == NULL)
>>>      {
>>> -      unsigned int seq = __pthread_keys[key].seq;
>>> +      PTHREAD_KEY_UNLOCK;
>>> +      return EAGAIN;
>>> +    }
>>
>> Not sure if POSIX allows return EGAIN here, I think it should be EINVAL.
> 
> 1003.1-2024 allows but doesn't list explicit errors, other than to
> forbid EINTR.  I could switch them all to EINVAL though, but do we want
> some way to tell the difference between a key which *could* be valid (in
> bounds but not created), vs a key which *couldn't* be valid (out of
> bounds) ?

But EAGAIN means "transient, retry may succeed.", this is not usually the
expected semantic for pthread_key_delete (retry until is works).

> 
> https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/functions/pthread_key_delete.html
>