summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStef Walter <stefw@redhat.com>2014-10-03 09:42:27 +0200
committerStef Walter <stefw@redhat.com>2014-10-03 20:56:16 +0200
commit16e25b2890927108ec15297aabb1d86a49792741 (patch)
tree495215b96e664348fa6651696d517b22215d8467
parenta3b1e1c2f2c8c1f14293d8158b6dfeb2a6560908 (diff)
downloadp11-kit-16e25b2890927108ec15297aabb1d86a49792741.tar.gz
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
-rw-r--r--common/library.c11
-rw-r--r--common/library.h2
-rw-r--r--common/mock.c1
-rw-r--r--p11-kit/modules.c55
-rw-r--r--p11-kit/proxy.c62
-rw-r--r--p11-kit/proxy.h2
-rw-r--r--p11-kit/rpc-client.c20
-rw-r--r--p11-kit/test-proxy.c2
-rw-r--r--p11-kit/test-rpc.c25
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 <assert.h>
#include <ctype.h>
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);
}