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

[DJ Delorie]

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?

>> +		  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?

>>  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) ?

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