Merge branch 'bugfix/pthread_destructor_sequencing' into 'master'

pthread: Fix behaviour when pthread destructor calls pthread_getspecific/pthread_setspecific

Closes IDFGH-4842

See merge request espressif/esp-idf!13567

components/pthread/pthread_local_storage.c

@@ -113,12 +113,17 @@ int pthread_key_delete(pthread_key_t key)
    This is called from one of two places:
 
    If the thread was created via pthread_create() then it's called by pthread_task_func() when that thread ends,
-   and the FreeRTOS thread-local-storage is removed before the FreeRTOS task is deleted.
+   or calls pthread_exit(), and the FreeRTOS thread-local-storage is removed before the FreeRTOS task is deleted.
 
    For other tasks, this is called when the FreeRTOS idle task performs its task cleanup after the task is deleted.
 
-   (The reason for calling it early for pthreads is to keep the timing consistent with "normal" pthreads, so after
-   pthread_join() the task's destructors have all been called even if the idle task hasn't run cleanup yet.)
+   There are two reasons for calling it early for pthreads:
+
+   - To keep the timing consistent with "normal" pthreads, so after pthread_join() the task's destructors have all
+     been called even if the idle task hasn't run cleanup yet.
+
+   - The destructor is always called in the context of the thread itself - which is important if the task then calls
+     pthread_getspecific() or pthread_setspecific() to update the state further, as allowed for in the spec.
 */
 static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls)
 {
@@ -126,8 +131,13 @@ static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls
     assert(tls != NULL);
 
     /* Walk the list, freeing all entries and calling destructors if they are registered */
-    value_entry_t *entry = SLIST_FIRST(tls);
-    while(entry != NULL) {
+    while (1) {
+        value_entry_t *entry = SLIST_FIRST(tls);
+        if (entry == NULL) {
+            break;
+        }
+        SLIST_REMOVE_HEAD(tls, next);
+
         // This is a little slow, walking the linked list of keys once per value,
         // but assumes that the thread's value list will have less entries
         // than the keys list
@@ -135,9 +145,7 @@ static void pthread_local_storage_thread_deleted_callback(int index, void *v_tls
         if (key != NULL && key->destructor != NULL) {
             key->destructor(entry->value);
         }
-        value_entry_t *next_entry = SLIST_NEXT(entry, next);
         free(entry);
-        entry = next_entry;
     }
     free(tls);
 }
@@ -250,7 +258,22 @@ int pthread_setspecific(pthread_key_t key, const void *value)
         }
         entry->key = key;
         entry->value = (void *) value; // see note above about cast
-        SLIST_INSERT_HEAD(tls, entry, next);
+
+        // insert the new entry at the end of the list. this is important because
+        // a destructor may call pthread_setspecific() to add a new non-NULL value
+        // to the list, and this should be processed after all other entries.
+        //
+        // See pthread_local_storage_thread_deleted_callback()
+        value_entry_t *last_entry = NULL;
+        value_entry_t *it;
+        SLIST_FOREACH(it, tls, next) {
+            last_entry = it;
+        }
+        if (last_entry == NULL) {
+            SLIST_INSERT_HEAD(tls, entry, next);
+        } else {
+            SLIST_INSERT_AFTER(last_entry, entry, next);
+        }
     }
 
     return 0;

components/pthread/test/test_pthread_local_storage.c

@@ -162,3 +162,90 @@ TEST_CASE("pthread local storage stress test", "[pthread]")
         TEST_ASSERT_EQUAL(0, pthread_join(threads[i], NULL));
     }
 }
+
+
+#define NUM_KEYS 4 // number of keys used in repeat destructor test
+#define NUM_REPEATS 17 // number of times we re-set a key to a non-NULL value to re-trigger destructor
+
+typedef struct {
+    pthread_key_t keys[NUM_KEYS]; // pthread local storage keys used in test
+    unsigned count; // number of times the destructor has been called
+    int last_idx; // index of last key where destructor was called
+} destr_test_state_t;
+
+static void s_test_repeat_destructor(void *vp_state);
+static void *s_test_repeat_destructor_thread(void *vp_state);
+
+// Test the correct behaviour of a pthread destructor function that uses
+// pthread_setspecific() to set another value when it runs, and also
+//
+// As described in https://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_key_create.html
+TEST_CASE("pthread local storage 'repeat' destructor test", "[pthread]")
+{
+    int r;
+    destr_test_state_t state = { .last_idx = -1 };
+    pthread_t thread;
+
+    for (int i = 0; i < NUM_KEYS; i++) {
+        r = pthread_key_create(&state.keys[i], s_test_repeat_destructor);
+        TEST_ASSERT_EQUAL(0, r);
+    }
+
+    r = pthread_create(&thread, NULL, s_test_repeat_destructor_thread, &state);
+    TEST_ASSERT_EQUAL(0, r);
+
+    r = pthread_join(thread, NULL);
+    TEST_ASSERT_EQUAL(0 ,r);
+
+    // Cheating here to make sure compiler reads the value of 'count' from memory not from a register
+    //
+    // We expect the destructor was called NUM_REPEATS times when it repeated, then NUM_KEYS times when it didn't
+    TEST_ASSERT_EQUAL(NUM_REPEATS + NUM_KEYS, ((volatile destr_test_state_t)state).count);
+
+    // cleanup
+    for (int i = 0; i < NUM_KEYS; i++) {
+        r = pthread_key_delete(state.keys[i]);
+        TEST_ASSERT_EQUAL(0, r);
+    }
+}
+
+static void s_test_repeat_destructor(void *vp_state)
+{
+    destr_test_state_t *state = vp_state;
+
+    state->count++;
+    printf("Destructor! Arg %p Count %d\n", state, state->count);
+    if (state->count > NUM_REPEATS) {
+        return; // Stop replacing values after NUM_REPEATS destructors have been called, they will be NULLed out now
+    }
+
+    // Find the key which has a NULL value, this is the key for this destructor. We will set it back to 'state' to repeat later.
+    // At this point only one key should have a NULL value
+    int null_idx = -1;
+    for (int i = 0; i < NUM_KEYS; i++) {
+        if (pthread_getspecific(state->keys[i]) == NULL) {
+            TEST_ASSERT_EQUAL(-1, null_idx); // If more than one key has a NULL value, something has gone wrong
+            null_idx = i;
+            // don't break, verify the other keys have non-NULL values
+        }
+    }
+
+    TEST_ASSERT_NOT_EQUAL(-1, null_idx); // One key should have a NULL value
+
+    // The same key shouldn't be destroyed twice in a row, as new non-NULL values should be destroyed
+    // after existing non-NULL values (to match spec behaviour)
+    TEST_ASSERT_NOT_EQUAL(null_idx, state->last_idx);
+
+    printf("Re-setting index %d\n", null_idx);
+    pthread_setspecific(state->keys[null_idx], state);
+    state->last_idx = null_idx;
+}
+
+static void *s_test_repeat_destructor_thread(void *vp_state)
+{
+    destr_test_state_t *state = vp_state;
+    for (int i = 0; i < NUM_KEYS; i++) {
+        pthread_setspecific(state->keys[i], state);
+    }
+    pthread_exit(NULL);
+}