threads/135462: [PATCH] _thread_cleanupspecific() doesn't handle deleted keys

[Marc Unangst]

>Number:         135462
>Category:       threads
>Synopsis:       [PATCH] _thread_cleanupspecific() doesn't handle deleted keys
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    freebsd-threads
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Jun 11 02:10:02 UTC 2009
>Closed-Date:
>Last-Modified:
>Originator:     Marc Unangst
>Release:        FreeBSD 7.0-RELEASE amd64
>Organization:
Panasas, Inc.
>Environment:
FreeBSD perf-x4 7.0-RELEASE FreeBSD 7.0-RELEASE #0: Sun Feb 24 10:35:36 UTC 2008     root at driscoll.cse.buffalo.edu:/usr/obj/usr/src/sys/GENERIC  amd64
>Description:
Programs that use thread-specific data occasionally exit with a warning like this:

Thread 801202c70 has exited with leftover thread-specific data after 4 destructor iterations

This occurs because the thread-specific data cleanup routine that runs when a thread exits (_thread_cleanupspecific()) doesn't handle cleaning up keys that were deleted via pthread_key_delete() while the exiting thread still had a non-NULL value set for that key.  The test is

			if (_thread_keytable[key].allocated &&
			    (curthread->specific[key].data != NULL)) {

i.e., if the key exists globally and the current thread has a non-NULL key.  However, POSIX allows the application to delete the key without clearing the value in all threads; http://www.opengroup.org/onlinepubs/009695399/functions/pthread_key_delete.html says "The thread-specific data values associated with key need not be NULL at the time pthread_key_delete() is called."  POSIX also specifies that in this case, the destructor routine (if one is defined) is not invoked.  In this scenario, the cleanup routine should just set the thread-specific data value for that key to NULL and decrement curthread->specific_data_count.
>How-To-Repeat:
Compile and run the following program:

/*
 * bug_93732.c
 *
 * @author mju
 *
 * Unit test for bug 93732.
 *
 * Compile like this:
 *      gcc -Wall -pthread -o bug_93732 bug_93732.c
 */

#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <pthread.h>
#include <string.h>

void *test_thread(void *arg_ignored);

int main(int argc, char **argv)
{
  int ret;
  pthread_t thr;

  ret = pthread_create(&thr, NULL, test_thread, NULL);
  if (ret != 0) {
    fprintf(stderr, "pthread_create failed: %s\n", strerror(errno));
    exit(1);
  }
  printf("Thread created, waiting for exit\n");
  (void) pthread_join(thr, NULL);
  printf("Thread terminated, exiting\n");

  exit(0);
}

void *test_thread(void *arg_ignored)
{
  int ret;
  pthread_key_t key;
  int i = 2;

  ret = pthread_key_create(&key, NULL);
  if (ret != 0) {
    fprintf(stderr, "pthread_key_create failed: %s\n", strerror(errno));
    exit(1);
  }
  printf("Created key %d\n", (int) key);

  ret = pthread_setspecific(key, &i);
  if (ret != 0) {
    fprintf(stderr, "pthread_setspecific failed: %s\n", strerror(errno));
    exit(1);
  }
  printf("Set key to %p\n", &i);

  ret = pthread_key_delete(key);
  if (ret != 0) {
    fprintf(stderr, "pthread_key_delete failed: %s\n", strerror(errno));
    exit(1);
  }
  printf("Deleted key %d\n", (int) key);
  return NULL;
}

>Fix:
See attached patch file.

Patch attached with submission follows:

--- /net/paneast/sb7/mju/freebsd-svn/head/lib/libthr/thread/thr_spec.c	2009-05-26 17:45:27.000000000 -0400
+++ thr_spec.c	2009-06-10 14:57:06.000000000 -0400
@@ -131,9 +131,19 @@
 				curthread->specific[key].data = NULL;
 				curthread->specific_data_count--;
 			}
+			else if (curthread->specific[key].data != NULL) {
+				/* 
+				 * This can happen if the key is deleted via
+				 * pthread_key_delete without first setting the value
+				 * to NULL in all threads.  POSIX says that the
+				 * destructor is not invoked in this case.
+				 */
+				curthread->specific[key].data = NULL;
+				curthread->specific_data_count--;
+			}
 
 			/*
-			 * If there is a destructore, call it
+			 * If there is a destructor, call it
 			 * with the key table entry unlocked:
 			 */
 			if (destructor != NULL) {


>Release-Note:
>Audit-Trail:
>Unformatted: