From: Stef Walter Date: Mon, 10 Oct 2011 15:32:34 +0000 (+0200) Subject: Only call C_Initialize and C_Finalize once per module X-Git-Tag: 0.8~5 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=73880f950a7dadf712730222ac1b6ea11400746f;p=p11-kit 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. --- 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 #include +#include #include #include #include @@ -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);