From 16e25b2890927108ec15297aabb1d86a49792741 Mon Sep 17 00:00:00 2001 From: Stef Walter Date: Fri, 3 Oct 2014 09:42:27 +0200 Subject: p11-kit: Use pthread_atfork() in a safe manner Instead of trying to perform actions in pthread_atfork() which are not async-signal-safe, just increment a counter so we can later tell if the process has forked. Note this does not make it safe to mix threads and forking without immediately execing. This is a far broader problem that p11-kit, however we now do the right thing when fork+exec is used from a thread. https://bugs.freedesktop.org/show_bug.cgi?id=84567 --- common/library.c | 11 ++++++++++ common/library.h | 2 ++ common/mock.c | 1 + p11-kit/modules.c | 55 ++++++++++------------------------------------ p11-kit/proxy.c | 62 ++++++++++++++++------------------------------------ p11-kit/proxy.h | 2 -- p11-kit/rpc-client.c | 20 ++++++++--------- p11-kit/test-proxy.c | 2 +- p11-kit/test-rpc.c | 25 +++++++-------------- 9 files changed, 63 insertions(+), 117 deletions(-) diff --git a/common/library.c b/common/library.c index b7d6923..502ea98 100644 --- a/common/library.c +++ b/common/library.c @@ -63,6 +63,8 @@ p11_mutex_t p11_library_mutex; pthread_once_t p11_library_once = PTHREAD_ONCE_INIT; #endif +unsigned int p11_forkid = 1; + static char * thread_local_message (void) { @@ -103,6 +105,13 @@ _p11_library_get_thread_local (void) return local; } +static void +count_forks (void) +{ + /* Thread safe, executed in child, one thread exists */ + p11_forkid++; +} + void p11_library_init_impl (void) { @@ -111,6 +120,8 @@ p11_library_init_impl (void) p11_mutex_init (&p11_library_mutex); pthread_key_create (&thread_local, free); p11_message_storage = thread_local_message; + + pthread_atfork (NULL, NULL, count_forks); } void diff --git a/common/library.h b/common/library.h index 33a33fb..f87494d 100644 --- a/common/library.h +++ b/common/library.h @@ -44,6 +44,8 @@ extern p11_mutex_t p11_library_mutex; +extern unsigned int p11_forkid; + #define p11_lock() p11_mutex_lock (&p11_library_mutex); #define p11_unlock() p11_mutex_unlock (&p11_library_mutex); diff --git a/common/mock.c b/common/mock.c index 01e095d..a73ae9d 100644 --- a/common/mock.c +++ b/common/mock.c @@ -46,6 +46,7 @@ #include "debug.h" #include "dict.h" #include "array.h" +#include "library.h" #include #include diff --git a/p11-kit/modules.c b/p11-kit/modules.c index 8aaa769..38c752b 100644 --- a/p11-kit/modules.c +++ b/p11-kit/modules.c @@ -158,7 +158,7 @@ typedef struct _Module { /* Initialization, mutex must be held */ p11_mutex_t initialize_mutex; - bool initialize_called; + unsigned int initialize_called; p11_thread_id_t initialize_thread; } Module; @@ -247,7 +247,6 @@ free_module_unlocked (void *data) p11_debug_precond ("module unloaded without C_Finalize having been " "called for each C_Initialize"); } else { - assert (!mod->initialize_called); assert (mod->initialize_thread == 0); } @@ -633,7 +632,7 @@ initialize_module_inlock_reentrant (Module *mod) p11_unlock (); p11_mutex_lock (&mod->initialize_mutex); - if (!mod->initialize_called) { + if (mod->initialize_called != p11_forkid) { p11_debug ("C_Initialize: calling"); rv = mod->virt.funcs.C_Initialize (&mod->virt.funcs, @@ -643,10 +642,12 @@ initialize_module_inlock_reentrant (Module *mod) /* Module was initialized and C_Finalize should be called */ if (rv == CKR_OK) - mod->initialize_called = true; + mod->initialize_called = p11_forkid; + else + mod->initialize_called = 0; /* Module was already initialized, we don't call C_Finalize */ - else if (rv == CKR_CRYPTOKI_ALREADY_INITIALIZED) + if (rv == CKR_CRYPTOKI_ALREADY_INITIALIZED) rv = CKR_OK; } @@ -665,31 +666,6 @@ initialize_module_inlock_reentrant (Module *mod) return rv; } -#ifdef OS_UNIX - -static void -reinitialize_after_fork (void) -{ - p11_dictiter iter; - Module *mod; - - p11_debug ("forked"); - - p11_lock (); - - if (gl.modules) { - p11_dict_iterate (gl.modules, &iter); - while (p11_dict_next (&iter, (void **)&mod, NULL)) - mod->initialize_called = false; - } - - p11_unlock (); - - p11_proxy_after_fork (); -} - -#endif /* OS_UNIX */ - static CK_RV init_globals_unlocked (void) { @@ -719,9 +695,6 @@ init_globals_unlocked (void) if (once) return CKR_OK; -#ifdef OS_UNIX - pthread_atfork (NULL, NULL, reinitialize_after_fork); -#endif once = true; return CKR_OK; @@ -777,9 +750,9 @@ finalize_module_inlock_reentrant (Module *mod) p11_unlock (); p11_mutex_lock (&mod->initialize_mutex); - if (mod->initialize_called) { + if (mod->initialize_called == p11_forkid) { mod->virt.funcs.C_Finalize (&mod->virt.funcs, NULL); - mod->initialize_called = false; + mod->initialize_called = 0; } p11_mutex_unlock (&mod->initialize_mutex); @@ -1437,7 +1410,7 @@ cleanup: typedef struct { p11_virtual virt; Module *mod; - pid_t initialized; + unsigned int initialized; p11_dict *sessions; } Managed; @@ -1447,14 +1420,12 @@ managed_C_Initialize (CK_X_FUNCTION_LIST *self, { Managed *managed = ((Managed *)self); p11_dict *sessions; - pid_t pid; CK_RV rv; p11_debug ("in"); p11_lock (); - pid = getpid (); - if (managed->initialized == pid) { + if (managed->initialized == p11_forkid) { rv = CKR_CRYPTOKI_ALREADY_INITIALIZED; } else { @@ -1467,7 +1438,7 @@ managed_C_Initialize (CK_X_FUNCTION_LIST *self, rv = initialize_module_inlock_reentrant (managed->mod); if (rv == CKR_OK) { managed->sessions = sessions; - managed->initialized = pid; + managed->initialized = p11_forkid; } else { p11_dict_free (sessions); } @@ -1568,18 +1539,16 @@ managed_C_Finalize (CK_X_FUNCTION_LIST *self, { Managed *managed = ((Managed *)self); CK_SESSION_HANDLE *sessions; - pid_t pid; int count; CK_RV rv; p11_debug ("in"); p11_lock (); - pid = getpid (); if (managed->initialized == 0) { rv = CKR_CRYPTOKI_NOT_INITIALIZED; - } else if (managed->initialized != pid) { + } else if (managed->initialized != p11_forkid) { /* * In theory we should be returning CKR_CRYPTOKI_NOT_INITIALIZED here * but enough callers are not completely aware of their forking. diff --git a/p11-kit/proxy.c b/p11-kit/proxy.c index 3e76f15..db2acb8 100644 --- a/p11-kit/proxy.c +++ b/p11-kit/proxy.c @@ -82,6 +82,7 @@ typedef struct { unsigned int n_mappings; p11_dict *sessions; CK_FUNCTION_LIST **inited; + unsigned int forkid; } Proxy; typedef struct _State { @@ -96,6 +97,8 @@ static CK_FUNCTION_LIST **all_modules = NULL; static State *all_instances = NULL; static State global = { { { { -1, -1 }, NULL, }, }, NULL, NULL, FIRST_HANDLE, NULL }; +#define PROXY_VALID(px) ((px) && (px)->forkid == p11_forkid) + #define MANUFACTURER_ID "PKCS#11 Kit " #define LIBRARY_DESCRIPTION "PKCS#11 Kit Proxy Module " #define LIBRARY_VERSION_MAJOR 1 @@ -137,7 +140,7 @@ map_slot_to_real (Proxy *px, p11_lock (); - if (!px) + if (!PROXY_VALID (px)) rv = CKR_CRYPTOKI_NOT_INITIALIZED; else rv = map_slot_unlocked (px, *slot, mapping); @@ -163,7 +166,7 @@ map_session_to_real (Proxy *px, p11_lock (); - if (!px) { + if (!PROXY_VALID (px)) { rv = CKR_CRYPTOKI_NOT_INITIALIZED; } else { assert (px->sessions); @@ -195,40 +198,6 @@ proxy_free (Proxy *py) } } -void -p11_proxy_after_fork (void) -{ - p11_array *array; - State *state; - unsigned int i; - - /* - * After a fork the callers are supposed to call C_Initialize and all. - * In addition the underlying libraries may change their state so free - * up any mappings and all - */ - - array = p11_array_new (NULL); - - p11_lock (); - - if (global.px) - p11_array_push (array, global.px); - global.px = NULL; - - for (state = all_instances; state != NULL; state = state->next) { - if (state->px) - p11_array_push (array, state->px); - state->px = NULL; - } - - p11_unlock (); - - for (i = 0; i < array->num; i++) - proxy_free (array->elem[i]); - p11_array_free (array); -} - static CK_RV proxy_C_Finalize (CK_X_FUNCTION_LIST *self, CK_VOID_PTR reserved) @@ -247,8 +216,10 @@ proxy_C_Finalize (CK_X_FUNCTION_LIST *self, } else { p11_lock (); - if (!state->px) { + if (!PROXY_VALID (state->px)) { rv = CKR_CRYPTOKI_NOT_INITIALIZED; + py = state->px; + state->px = NULL; } else if (state->px->refs-- == 1) { py = state->px; state->px = NULL; @@ -287,6 +258,8 @@ proxy_create (Proxy **res) py = calloc (1, sizeof (Proxy)); return_val_if_fail (py != NULL, CKR_HOST_MEMORY); + py->forkid = p11_forkid; + py->inited = modules_dup (all_modules); return_val_if_fail (py->inited != NULL, CKR_HOST_MEMORY); @@ -357,10 +330,13 @@ proxy_C_Initialize (CK_X_FUNCTION_LIST *self, p11_lock (); - if (state->px == NULL) + if (!PROXY_VALID (state->px)) { initialize = true; - else + proxy_free (state->px); + state->px = NULL; + } else { state->px->refs++; + } p11_unlock (); @@ -402,7 +378,7 @@ proxy_C_GetInfo (CK_X_FUNCTION_LIST *self, p11_lock (); - if (!state->px) + if (!PROXY_VALID (state->px)) rv = CKR_CRYPTOKI_NOT_INITIALIZED; p11_unlock (); @@ -438,7 +414,7 @@ proxy_C_GetSlotList (CK_X_FUNCTION_LIST *self, p11_lock (); - if (!state->px) { + if (!PROXY_VALID (state->px)) { rv = CKR_CRYPTOKI_NOT_INITIALIZED; } else { index = 0; @@ -586,7 +562,7 @@ proxy_C_OpenSession (CK_X_FUNCTION_LIST *self, if (rv == CKR_OK) { p11_lock (); - if (!state->px) { + if (!PROXY_VALID (state->px)) { /* * The underlying module should have returned an error, so this * code should never be reached with properly behaving modules. @@ -650,7 +626,7 @@ proxy_C_CloseAllSessions (CK_X_FUNCTION_LIST *self, p11_lock (); - if (!state->px) { + if (!PROXY_VALID (state->px)) { rv = CKR_CRYPTOKI_NOT_INITIALIZED; } else { assert (state->px->sessions != NULL); diff --git a/p11-kit/proxy.h b/p11-kit/proxy.h index df05be0..f3d56d7 100644 --- a/p11-kit/proxy.h +++ b/p11-kit/proxy.h @@ -35,8 +35,6 @@ #ifndef __P11_PROXY_H__ #define __P11_PROXY_H__ -void p11_proxy_after_fork (void); - bool p11_proxy_module_check (CK_FUNCTION_LIST_PTR module); void p11_proxy_module_cleanup (void); diff --git a/p11-kit/rpc-client.c b/p11-kit/rpc-client.c index 23cfcfc..8607bb5 100644 --- a/p11-kit/rpc-client.c +++ b/p11-kit/rpc-client.c @@ -56,7 +56,7 @@ typedef struct { p11_mutex_t mutex; p11_rpc_client_vtable *vtable; - pid_t initialized_pid; + unsigned int initialized_forkid; bool initialize_done; } rpc_client; @@ -80,7 +80,7 @@ call_prepare (rpc_client *module, assert (module != NULL); assert (msg != NULL); - if (module->initialized_pid == 0) + if (module->initialized_forkid != p11_forkid) return CKR_CRYPTOKI_NOT_INITIALIZED; if (!module->initialize_done) return CKR_DEVICE_REMOVED; @@ -850,7 +850,6 @@ rpc_C_Initialize (CK_X_FUNCTION_LIST *self, void *reserved = NULL; CK_RV ret = CKR_OK; p11_rpc_message msg; - pid_t pid; assert (module != NULL); p11_debug ("C_Initialize: enter"); @@ -886,10 +885,9 @@ rpc_C_Initialize (CK_X_FUNCTION_LIST *self, p11_mutex_lock (&module->mutex); - pid = getpid (); - if (module->initialized_pid != 0) { + if (module->initialized_forkid != 0) { /* This process has called C_Initialize already */ - if (pid == module->initialized_pid) { + if (p11_forkid == module->initialized_forkid) { p11_message ("C_Initialize called twice for same process"); ret = CKR_CRYPTOKI_ALREADY_INITIALIZED; goto done; @@ -902,12 +900,12 @@ rpc_C_Initialize (CK_X_FUNCTION_LIST *self, /* Successfully initialized */ if (ret == CKR_OK) { - module->initialized_pid = pid; + module->initialized_forkid = p11_forkid; module->initialize_done = true; /* Server doesn't exist, initialize but don't call */ } else if (ret == CKR_DEVICE_REMOVED) { - module->initialized_pid = pid; + module->initialized_forkid = p11_forkid; module->initialize_done = false; ret = CKR_OK; goto done; @@ -928,7 +926,7 @@ rpc_C_Initialize (CK_X_FUNCTION_LIST *self, done: /* If failed then unmark initialized */ if (ret != CKR_OK && ret != CKR_CRYPTOKI_ALREADY_INITIALIZED) - module->initialized_pid = 0; + module->initialized_forkid = 0; /* If we told our caller that we're initialized, but not really, then finalize */ if (ret != CKR_OK && module->initialize_done) { @@ -952,7 +950,7 @@ rpc_C_Finalize (CK_X_FUNCTION_LIST *self, p11_rpc_message msg; p11_debug ("C_Finalize: enter"); - return_val_if_fail (module->initialized_pid != 0, CKR_CRYPTOKI_NOT_INITIALIZED); + return_val_if_fail (module->initialized_forkid == p11_forkid, CKR_CRYPTOKI_NOT_INITIALIZED); return_val_if_fail (!reserved, CKR_ARGUMENTS_BAD); p11_mutex_lock (&module->mutex); @@ -970,7 +968,7 @@ rpc_C_Finalize (CK_X_FUNCTION_LIST *self, (module->vtable->disconnect) (module->vtable, reserved); } - module->initialized_pid = 0; + module->initialized_forkid = 0; p11_mutex_unlock (&module->mutex); diff --git a/p11-kit/test-proxy.c b/p11-kit/test-proxy.c index bf5007d..e4998be 100644 --- a/p11-kit/test-proxy.c +++ b/p11-kit/test-proxy.c @@ -76,7 +76,7 @@ test_initialize_finalize (void) assert (rv == CKR_OK); rv = proxy->C_Finalize (NULL); - assert (rv == CKR_OK); + assert_num_eq (rv, CKR_OK); p11_proxy_module_cleanup (); } diff --git a/p11-kit/test-rpc.c b/p11-kit/test-rpc.c index 8c20a40..c9f8333 100644 --- a/p11-kit/test-rpc.c +++ b/p11-kit/test-rpc.c @@ -353,17 +353,15 @@ test_byte_array_static (void) } static p11_virtual base; -static pid_t rpc_initialized = 0; +static unsigned int rpc_initialized = 0; static CK_RV rpc_initialize (p11_rpc_client_vtable *vtable, void *init_reserved) { - pid_t pid = getpid (); - assert_str_eq (vtable->data, "vtable-data"); - assert_num_cmp (pid, !=, rpc_initialized); - rpc_initialized = pid; + assert_num_cmp (p11_forkid, !=, rpc_initialized); + rpc_initialized = p11_forkid; return CKR_OK; } @@ -372,10 +370,8 @@ static CK_RV rpc_initialize_fails (p11_rpc_client_vtable *vtable, void *init_reserved) { - pid_t pid = getpid (); - assert_str_eq (vtable->data, "vtable-data"); - assert_num_cmp (pid, !=, rpc_initialized); + assert_num_cmp (p11_forkid, !=, rpc_initialized); return CKR_FUNCTION_FAILED; } @@ -383,10 +379,8 @@ static CK_RV rpc_initialize_device_removed (p11_rpc_client_vtable *vtable, void *init_reserved) { - pid_t pid = getpid (); - assert_str_eq (vtable->data, "vtable-data"); - assert_num_cmp (pid, !=, rpc_initialized); + assert_num_cmp (p11_forkid, !=, rpc_initialized); return CKR_DEVICE_REMOVED; } @@ -410,10 +404,8 @@ static void rpc_finalize (p11_rpc_client_vtable *vtable, void *fini_reserved) { - pid_t pid = getpid (); - assert_str_eq (vtable->data, "vtable-data"); - assert_num_cmp (pid, ==, rpc_initialized); + assert_num_cmp (p11_forkid, ==, rpc_initialized); rpc_initialized = 0; } @@ -421,7 +413,6 @@ static void test_initialize (void) { p11_rpc_client_vtable vtable = { "vtable-data", rpc_initialize, rpc_transport, rpc_finalize }; - pid_t pid = getpid (); p11_virtual mixin; bool ret; CK_RV rv; @@ -435,11 +426,11 @@ test_initialize (void) rv = mixin.funcs.C_Initialize (&mixin.funcs, NULL); assert (rv == CKR_OK); - assert_num_eq (pid, rpc_initialized); + assert_num_eq (p11_forkid, rpc_initialized); rv = mixin.funcs.C_Finalize (&mixin.funcs, NULL); assert (rv == CKR_OK); - assert_num_cmp (pid, !=, rpc_initialized); + assert_num_cmp (p11_forkid, !=, rpc_initialized); p11_virtual_uninit (&mixin); } -- cgit v1.2.3