Attachment "tcl-thread-syncobj-leak.patch" to
ticket [1726873fff]
added by
twylite
2007-06-19 14:27:39.
diff -u -r tcl8.5a6\generic/tclThread.c tcl\generic/tclThread.c
--- tcl8.5a6\generic/tclThread.c 2006-11-09 00:53:39.000000000 +0200
+++ tcl\generic/tclThread.c 2007-06-05 14:40:01.262125000 +0200
@@ -23,23 +23,22 @@
* TclRememberThreadData, e.g., TclpThreadDataKeyInit
*/
+#define SYNC_OBJ_LIST_ELEMS 16
+
typedef struct {
- int num; /* Number of objects remembered */
- int max; /* Max size of the array */
- char **list; /* List of pointers */
+ char **list; /* List of pointers */
} SyncObjRecord;
-
-static SyncObjRecord keyRecord = {0, 0, NULL};
-static SyncObjRecord mutexRecord = {0, 0, NULL};
-static SyncObjRecord condRecord = {0, 0, NULL};
+
+static SyncObjRecord keyRecord = {NULL};
+static SyncObjRecord mutexRecord = {NULL};
+static SyncObjRecord condRecord = {NULL};
/*
* Prototypes of functions used only in this file.
*/
static void ForgetSyncObject(char *objPtr, SyncObjRecord *recPtr);
-static void RememberSyncObject(char *objPtr,
- SyncObjRecord *recPtr);
+static void RememberSyncObject(char *objPtr, SyncObjRecord *recPtr);
/*
* Several functions are #defined to nothing in tcl.h if TCL_THREADS is not
@@ -153,34 +152,56 @@
static void
RememberSyncObject(
- char *objPtr, /* Pointer to sync object */
- SyncObjRecord *recPtr) /* Record of sync objects */
+ char *objPtr, /* Pointer to sync object */
+ SyncObjRecord *recPtr) /* Record of sync objects */
{
- char **newList;
- int i, j;
-
- /*
- * Save the pointer to the allocated object so it can be finalized. Grow
- * the list of pointers if necessary, copying only non-NULL pointers to
- * the new list.
- */
-
- if (recPtr->num >= recPtr->max) {
- recPtr->max += 8;
- newList = (char **) ckalloc(recPtr->max * sizeof(char *));
- for (i=0,j=0 ; i<recPtr->num ; i++) {
- if (recPtr->list[i] != NULL) {
- newList[j++] = recPtr->list[i];
- }
- }
- if (recPtr->list != NULL) {
- ckfree((char *) recPtr->list);
- }
- recPtr->list = newList;
- recPtr->num = j;
- }
- recPtr->list[recPtr->num] = objPtr;
- recPtr->num++;
+ int i;
+ char** localList = NULL;
+ char*** listPtr = &(recPtr->list);
+
+ /* How this works: recPtr->list is an array of SYNC_OBJ_LIST_ELEMS pointers
+ * plus one extra pointer. The extra pointer is to a chained list (char**)
+ * of SYNC_OBJ_LIST_ELEMS pointers, plus an extra pointer. Etc.
+ * We find the first empty (NULL) slot in the chain of lists and stick
+ * objPtr into it. */
+
+ do {
+ /* This code is not thread safe -- it must be done under a lock.
+ * Usually the caller holds the MASTER_LOCK.
+ * If two threads were to enter this logic at the same time, they
+ * could use the same empty slot (losing one of the objPtrs) or
+ * allocate two new chained lists, one of which will be lost.
+ * It IS safe to call ForgetSyncObject at the same time as this
+ * logic though, as long as:
+ * 1. we zeroize the newList before assigning it to the *listPtr.
+ * 2. we never shrink the list by ckfree'ing a chained list. If you
+ * do that we're back to the race condition that required this fix
+ * in the first place.
+ */
+
+ if ( *listPtr == NULL ) {
+ size_t listSize = (SYNC_OBJ_LIST_ELEMS + 1) * sizeof(char *);
+
+ localList = (char **) ckalloc( listSize );
+ memset ( localList, 0, listSize );
+ *listPtr = localList;
+ }
+
+ localList = *listPtr;
+
+ for ( i = 0; i < SYNC_OBJ_LIST_ELEMS; ++i ) {
+ if ( localList[i] == NULL ) {
+ localList[i] = objPtr;
+ break;
+ }
+ }
+
+ if ( i == SYNC_OBJ_LIST_ELEMS ) {
+ listPtr = &((char **)localList[i]);
+ }
+
+ } while (i == SYNC_OBJ_LIST_ELEMS);
+
}
/*
@@ -201,17 +222,29 @@
static void
ForgetSyncObject(
- char *objPtr, /* Pointer to sync object */
- SyncObjRecord *recPtr) /* Record of sync objects */
+ char *objPtr, /* Pointer to sync object */
+ SyncObjRecord *recPtr) /* Record of sync objects */
{
int i;
-
- for (i=0 ; i<recPtr->num ; i++) {
- if (objPtr == recPtr->list[i]) {
- recPtr->list[i] = NULL;
- return;
- }
- }
+ char** localList = recPtr->list;
+
+ do {
+ /* Refer to comments in RememberSyncObject before altering this code.
+ * You have been warned. */
+
+ for ( i = 0; i < SYNC_OBJ_LIST_ELEMS; ++i ) {
+ if ( localList[i] == objPtr ) {
+ localList[i] = NULL;
+ break;
+ }
+ }
+
+ if ( i == SYNC_OBJ_LIST_ELEMS ) {
+ localList = (char **)localList[i];
+ }
+
+ } while (i == SYNC_OBJ_LIST_ELEMS);
+
}
/*
@@ -366,54 +399,78 @@
TclpMasterLock();
- /*
- * If we're running unthreaded, the TSD blocks are simply stored inside
- * their thread data keys. Free them here.
- */
+ /* Refer to comments in RememberSyncObject before altering this code.
+ * You have been warned. */
- for (i=0 ; i<keyRecord.num ; i++) {
- keyPtr = (Tcl_ThreadDataKey *) keyRecord.list[i];
- blockPtr = (void *) *keyPtr;
- ckfree(blockPtr);
- }
- if (keyRecord.list != NULL) {
- ckfree((char *) keyRecord.list);
- keyRecord.list = NULL;
+ /*
+ * If we're running unthreaded, the TSD blocks are simply stored inside
+ * their thread data keys. Free them here.
+ */
+ while (keyRecord.list != NULL) {
+
+ for ( i = 0; i < SYNC_OBJ_LIST_ELEMS; ++i ) {
+ if ( keyRecord.list[i] != NULL ) {
+ keyPtr = (Tcl_ThreadDataKey *) keyRecord.list[i];
+ blockPtr = (void *) *keyPtr;
+ ckfree(blockPtr);
+ }
+ }
+
+ if ( keyRecord.list[i] != NULL ) {
+ char *tmp = (char *) keyRecord.list;
+ keyRecord.list = (char **)keyRecord.list[i];
+ ckfree(tmp);
+ } else {
+ ckfree((char *) keyRecord.list);
+ keyRecord.list = NULL;
+ }
+
}
- keyRecord.max = 0;
- keyRecord.num = 0;
-
+
/*
- * Call thread storage master cleanup.
- */
-
+ * Call thread storage master cleanup.
+ */
+
TclFinalizeThreadStorage();
-
- for (i=0 ; i<mutexRecord.num ; i++) {
- mutexPtr = (Tcl_Mutex *)mutexRecord.list[i];
- if (mutexPtr != NULL) {
- TclpFinalizeMutex(mutexPtr);
- }
- }
- if (mutexRecord.list != NULL) {
- ckfree((char *) mutexRecord.list);
- mutexRecord.list = NULL;
- }
- mutexRecord.max = 0;
- mutexRecord.num = 0;
-
- for (i=0 ; i<condRecord.num ; i++) {
- condPtr = (Tcl_Condition *) condRecord.list[i];
- if (condPtr != NULL) {
- TclpFinalizeCondition(condPtr);
- }
- }
- if (condRecord.list != NULL) {
- ckfree((char *) condRecord.list);
- condRecord.list = NULL;
+
+ while (mutexRecord.list != NULL) {
+
+ for ( i = 0; i < SYNC_OBJ_LIST_ELEMS; ++i ) {
+ mutexPtr = (Tcl_Mutex *)mutexRecord.list[i];
+ if ( mutexPtr != NULL ) {
+ TclpFinalizeMutex(mutexPtr);
+ }
+ }
+
+ if ( mutexRecord.list[i] != NULL ) {
+ char *tmp = (char *) mutexRecord.list;
+ mutexRecord.list = (char **)mutexRecord.list[i];
+ ckfree(tmp);
+ } else {
+ ckfree((char *) mutexRecord.list);
+ mutexRecord.list = NULL;
+ }
+
+ }
+
+ while (condRecord.list != NULL) {
+
+ for ( i = 0; i < SYNC_OBJ_LIST_ELEMS; ++i ) {
+ condPtr = (Tcl_Condition *) condRecord.list[i];
+ if ( condPtr != NULL ) {
+ TclpFinalizeCondition(condPtr);
+ }
+ }
+
+ if ( condRecord.list[i] != NULL ) {
+ char *tmp = (char *) condRecord.list;
+ condRecord.list = (char **)condRecord.list[i];
+ ckfree(tmp);
+ } else {
+ ckfree((char *) condRecord.list);
+ condRecord.list = NULL;
+ }
}
- condRecord.max = 0;
- condRecord.num = 0;
TclpMasterUnlock();
#else /* TCL_THREADS */
@@ -421,8 +478,6 @@
ckfree((char *) keyRecord.list);
keyRecord.list = NULL;
}
- keyRecord.max = 0;
- keyRecord.num = 0;
#endif /* TCL_THREADS */
}
