find_key(): This routine wasn't thread-correct, and accounts for the

release-build failures noted in bug 1041645.

This is a critical bugfix.  I'm not going to backport it, though (no time).

1 files changed, 17 insertions, 5 deletions

diff --git a/Python/thread.c b/Python/thread.c
index b1ddf53..4c0edfb 100644
--- a/Python/thread.c
+++ b/Python/thread.c
@@ -209,6 +209,15 @@ static int nkeys = 0;  /* PyThread_create_key() hands out nkeys+1 next */
  * So when value==NULL, this acts like a pure lookup routine, and when
  * value!=NULL, this acts like dict.setdefault(), returning an existing
  * mapping if one exists, else creating a new mapping.
+ *
+ * Caution:  this used to be too clever, trying to hold keymutex only
+ * around the "p->next = keyhead; keyhead = p" pair.  That allowed
+ * another thread to mutate the list, via key deletion, concurrent with
+ * find_key() crawling over the list.  Hilarity ensued.  For example, when
+ * the for-loop here does "p = p->next", p could end up pointing at a
+ * record that PyThread_delete_key_value() was concurrently free()'ing.
+ * That could lead to anything, from failing to find a key that exists, to
+ * segfaults.  Now we lock the whole routine.
  */
 static struct key *
 find_key(int key, void *value)
@@ -216,22 +225,25 @@ find_key(int key, void *value)
 	struct key *p;
 	long id = PyThread_get_thread_ident();
 
+	PyThread_acquire_lock(keymutex, 1);
 	for (p = keyhead; p != NULL; p = p->next) {
 		if (p->id == id && p->key == key)
-			return p;
+			goto Done;
+	}
+	if (value == NULL) {
+		assert(p == NULL);
+		goto Done;
 	}
-	if (value == NULL)
-		return NULL;
 	p = (struct key *)malloc(sizeof(struct key));
 	if (p != NULL) {
 		p->id = id;
 		p->key = key;
 		p->value = value;
-		PyThread_acquire_lock(keymutex, 1);
 		p->next = keyhead;
 		keyhead = p;
-		PyThread_release_lock(keymutex);
 	}
+ Done:
+	PyThread_release_lock(keymutex);
 	return p;
 }