Only call C_Initialize and C_Finalize once per module

 * Do not concurretnly call C_Initialize or C_Finalize in a module
 * The PKCS#11 spec indicates that mone thread should call those functions.
 * It's reasonable for a module to expect to only be initialized or
   finalized in one thread.
 * In particular NSS does not lock its C_Initialize or C_Finalize.

3 files changed, 168 insertions, 58 deletions

diff --git a/p11-kit/modules.c b/p11-kit/modules.c
index d8b7814..1394532 100644
--- a/p11-kit/modules.c
+++ b/p11-kit/modules.c
@@ -97,6 +97,7 @@
 
 typedef struct _Module {
 	CK_FUNCTION_LIST_PTR funcs;
+	CK_C_INITIALIZE_ARGS init_args;
 	int ref_count;
 
 	/* Registered modules */
@@ -106,10 +107,10 @@ typedef struct _Module {
 	/* Loaded modules */
 	void *dl_module;
 
-	/* Initialized modules */
-	CK_C_INITIALIZE_ARGS init_args;
-	int initialize_count;
-	int initializing;
+	/* Initialization, mutex must be held */
+	pthread_mutex_t initialize_mutex;
+	int initialize_called;
+	pthread_t initialize_thread;
 } Module;
 
 /*
@@ -216,13 +217,16 @@ free_module_unlocked (void *data)
 	assert (mod);
 
 	/* Module must be finalized */
-	assert (mod->initialize_count == 0);
+	assert (mod->initialize_called == 0);
+	assert (mod->initialize_thread == 0);
 
 	/* Module must have no outstanding references */
 	assert (mod->ref_count == 0);
 
 	if (mod->dl_module)
 		dlclose (mod->dl_module);
+
+	pthread_mutex_destroy (&mod->initialize_mutex);
 	hash_free (mod->config);
 	free (mod->name);
 	free (mod);
@@ -242,6 +246,7 @@ alloc_module_unlocked (void)
 	mod->init_args.LockMutex = lock_mutex;
 	mod->init_args.UnlockMutex = unlock_mutex;
 	mod->init_args.flags = CKF_OS_LOCKING_OK;
+	pthread_mutex_init (&mod->initialize_mutex, NULL);
 
 	return mod;
 }
@@ -410,22 +415,27 @@ take_config_and_load_module_unlocked (char **name, hashmap **config)
 
 	prev = hash_get (gl.modules, mod->funcs);
 
-	/* Replace previous module that was loaded explicitly? */
-	if (prev && !prev->name) {
-		mod->ref_count = prev->ref_count;
-		mod->initialize_count = prev->initialize_count;
-		prev->ref_count = 0;
-		prev->initialize_count = 0;
-		prev = NULL; /* freed by hash_set below */
-	}
+	/* If same module was loaded previously, just take over config */
+	if (prev && !prev->name && !prev->config) {
+		prev->name = mod->name;
+		mod->name = NULL;
+		prev->config = mod->config;
+		mod->config = NULL;
+		free_module_unlocked (mod);
 
-	/* Refuse to load duplicate module */
-	if (prev) {
+	/* Ignore duplicate module */
+	} else if (prev) {
 		_p11_message ("duplicate configured module: %s: %s", mod->name, path);
 		free_module_unlocked (mod);
-		return CKR_OK;
-	}
 
+	/* Add this new module to our hash table */
+	} else {
+		if (!hash_set (gl.modules, mod->funcs, mod)) {
+			free_module_unlocked (mod);
+			return CKR_HOST_MEMORY;
+		}
+
+	}
 	/*
 	 * We support setting of CK_C_INITIALIZE_ARGS.pReserved from
 	 * 'x-init-reserved' setting in the config. This only works with specific
@@ -433,11 +443,6 @@ take_config_and_load_module_unlocked (char **name, hashmap **config)
 	 */
 	mod->init_args.pReserved = hash_get (mod->config, "x-init-reserved");
 
-	if (!hash_set (gl.modules, mod->funcs, mod)) {
-		free_module_unlocked (mod);
-		return CKR_HOST_MEMORY;
-	}
-
 	return CKR_OK;
 }
 
@@ -510,9 +515,12 @@ static CK_RV
 initialize_module_unlocked_reentrant (Module *mod)
 {
 	CK_RV rv = CKR_OK;
+	pthread_t self;
 	assert (mod);
 
-	if (mod->initializing) {
+	self = pthread_self ();
+
+	if (mod->initialize_thread == self) {
 		_p11_message ("p11-kit initialization called recursively");
 		return CKR_FUNCTION_FAILED;
 	}
@@ -522,40 +530,38 @@ initialize_module_unlocked_reentrant (Module *mod)
 	 * underneath us when the mutex is unlocked below.
 	 */
 	++mod->ref_count;
+	mod->initialize_thread = self;
 
-	if (!mod->initialize_count) {
-
-		mod->initializing = 1;
-		debug ("C_Initialize: calling");
+	/* Change over to the module specific mutex */
+	pthread_mutex_lock (&mod->initialize_mutex);
+	_p11_unlock ();
 
-		_p11_unlock ();
+	if (!mod->initialize_called) {
 
-			assert (mod->funcs);
-			rv = mod->funcs->C_Initialize (&mod->init_args);
+		debug ("C_Initialize: calling");
 
-		_p11_lock ();
+		assert (mod->funcs);
+		rv = mod->funcs->C_Initialize (&mod->init_args);
 
 		debug ("C_Initialize: result: %lu", rv);
-		mod->initializing = 0;
-
-		/*
-		 * Because we have the mutex unlocked above, two initializes could
-		 * race. Therefore we need to take CKR_CRYPTOKI_ALREADY_INITIALIZED
-		 * into account.
-		 *
-		 * We also need to take into account where in a race both calls return
-		 * CKR_OK (which is not according to the spec but may happen, I mean we
-		 * do it in this module, so it's not unimaginable).
-		 */
 
+		/* Module was initialized and C_Finalize should be called */
 		if (rv == CKR_OK)
-			++mod->initialize_count;
+			mod->initialize_called = 1;
+
+		/* Module was already initialized, we don't call C_Finalize */
 		else if (rv == CKR_CRYPTOKI_ALREADY_INITIALIZED)
 			rv = CKR_OK;
-		else
-			--mod->ref_count;
 	}
 
+	pthread_mutex_unlock (&mod->initialize_mutex);
+	_p11_lock ();
+
+	/* Don't claim reference if failed */
+	if (rv != CKR_OK)
+		--mod->ref_count;
+
+	mod->initialize_thread = 0;
 	return rv;
 }
 
@@ -573,8 +579,8 @@ reinitialize_after_fork (void)
 		if (gl.modules) {
 			hash_iterate (gl.modules, &iter);
 			while (hash_next (&iter, NULL, (void **)&mod)) {
-				if (mod->initialize_count > 0) {
-					mod->initialize_count = 0;
+				if (mod->initialize_called) {
+					mod->initialize_called = 0;
 
 					/* WARNING: Reentrancy can occur here */
 					initialize_module_unlocked_reentrant (mod);
@@ -648,19 +654,20 @@ finalize_module_unlocked_reentrant (Module *mod)
 	 */
 	++mod->ref_count;
 
-	while (mod->initialize_count > 0) {
-
-		_p11_unlock ();
+	pthread_mutex_lock (&mod->initialize_mutex);
+	_p11_unlock ();
 
-			assert (mod->funcs);
-			mod->funcs->C_Finalize (NULL);
+	if (mod->initialize_called) {
 
-		_p11_lock ();
+		assert (mod->funcs);
+		mod->funcs->C_Finalize (NULL);
 
-		if (mod->initialize_count > 0)
-			--mod->initialize_count;
+		mod->initialize_called = 0;
 	}
 
+	pthread_mutex_unlock (&mod->initialize_mutex);
+	_p11_lock ();
+
 	/* Match the increment above */
 	--mod->ref_count;
 
diff --git a/tests/mock-module.c b/tests/mock-module.c
index a1a6a7e..201fffc 100644
--- a/tests/mock-module.c
+++ b/tests/mock-module.c
@@ -156,8 +156,8 @@ CK_RV
 mock_C_Finalize (CK_VOID_PTR reserved)
 {
 	debug (("C_Finalize: enter"));
-	return_val_if_fail (pkcs11_initialized, CKR_CRYPTOKI_NOT_INITIALIZED);
-	return_val_if_fail (!reserved, CKR_ARGUMENTS_BAD);
+	return_val_if_fail (pkcs11_initialized != 0, CKR_CRYPTOKI_NOT_INITIALIZED);
+	return_val_if_fail (reserved == NULL, CKR_ARGUMENTS_BAD);
 
 	pthread_mutex_lock (&init_mutex);
 
diff --git a/tests/test-init.c b/tests/test-init.c
index 5d96d81..d57b49f 100644
--- a/tests/test-init.c
+++ b/tests/test-init.c
@@ -39,6 +39,7 @@
 #include <sys/wait.h>
 
 #include <assert.h>
+#include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -94,7 +95,6 @@ test_fork_initialization (CuTest *tc)
 	CuAssertTrue (tc, rv == CKR_OK);
 }
 
-
 static CK_RV
 mock_C_Initialize__with_recursive (CK_VOID_PTR init_args)
 {
@@ -120,6 +120,108 @@ test_recursive_initialization (CuTest *tc)
 	CuAssertTrue (tc, rv == CKR_FUNCTION_FAILED);
 }
 
+static pthread_mutex_t race_mutex = PTHREAD_MUTEX_INITIALIZER;
+static int initialization_count = 0;
+static int finalization_count = 0;
+
+#include "private.h"
+
+static CK_RV
+mock_C_Initialize__threaded_race (CK_VOID_PTR init_args)
+{
+	struct timespec ts = { 0, 100 * 1000 * 1000 };
+
+	/* Atomically increment value */
+	pthread_mutex_lock (&race_mutex);
+	initialization_count += 1;
+	pthread_mutex_unlock (&race_mutex);
+
+	nanosleep (&ts, NULL);
+	return CKR_OK;
+}
+
+static CK_RV
+mock_C_Finalize__threaded_race (CK_VOID_PTR reserved)
+{
+	struct timespec ts = { 0, 100 * 1000 * 1000 };
+
+	/* Atomically increment value */
+	pthread_mutex_lock (&race_mutex);
+	finalization_count += 1;
+	pthread_mutex_unlock (&race_mutex);
+
+	nanosleep (&ts, NULL);
+	return CKR_OK;
+}
+
+static void *
+initialization_thread (void *data)
+{
+	CuTest *tc = data;
+	CK_RV rv;
+
+	rv = p11_kit_initialize_module (&module);
+	CuAssertTrue (tc, rv == CKR_OK);
+
+	return tc;
+}
+
+static void *
+finalization_thread (void *data)
+{
+	CuTest *tc = data;
+	CK_RV rv;
+
+	rv = p11_kit_finalize_module (&module);
+	CuAssertTrue (tc, rv == CKR_OK);
+
+	return tc;
+}
+
+static void
+test_threaded_initialization (CuTest *tc)
+{
+	static const int num_threads = 100;
+	pthread_t threads[num_threads];
+	void *retval;
+	int ret;
+	int i;
+
+	/* Build up our own function list */
+	memcpy (&module, &mock_module_no_slots, sizeof (CK_FUNCTION_LIST));
+	module.C_Initialize = mock_C_Initialize__threaded_race;
+	module.C_Finalize = mock_C_Finalize__threaded_race;
+
+	initialization_count = 0;
+	finalization_count = 0;
+
+	for (i = 0; i < num_threads; i++) {
+		ret = pthread_create (&threads[i], NULL, initialization_thread, tc);
+		CuAssertIntEquals (tc, 0, ret);
+	}
+
+	for (i = 0; i < num_threads; i++) {
+		ret = pthread_join (threads[i], &retval);
+		CuAssertIntEquals (tc, 0, ret);
+		CuAssertPtrEquals (tc, tc, retval);
+	}
+
+	for (i = 0; i < num_threads; i++) {
+		ret = pthread_create (&threads[i], NULL, finalization_thread, tc);
+		CuAssertIntEquals (tc, 0, ret);
+	}
+
+	for (i = 0; i < num_threads; i++) {
+		ret = pthread_join (threads[i], &retval);
+		CuAssertIntEquals (tc, 0, ret);
+		CuAssertPtrEquals (tc, tc, retval);
+	}
+
+	/* C_Initialize should have been called exactly once */
+	CuAssertIntEquals (tc, 1, initialization_count);
+	CuAssertIntEquals (tc, 1, finalization_count);
+}
+
 int
 main (void)
 {
@@ -129,6 +231,7 @@ main (void)
 
 	SUITE_ADD_TEST (suite, test_fork_initialization);
 	SUITE_ADD_TEST (suite, test_recursive_initialization);
+	SUITE_ADD_TEST (suite, test_threaded_initialization);
 
 	CuSuiteRun (suite);
 	CuSuiteSummary (suite, output);