Attachment 107246 Details for Bug 84567 – p11-kit: Use pthread_atfork() in a safe manner

[patch] p11-kit: Use pthread_atfork() in a safe manner

p11-kit-Use-pthreadatfork-in-a-safe-manner.patch (text/plain), 15.66 KB, created by Stef Walter on 2014-10-03 07:44:16 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stef Walter

Created: 2014-10-03 07:44:16 UTC

Size: 15.66 KB

patch

obsolete

>From 386cd9e05c247525f3e698efc0ca84506ae81c0d Mon Sep 17 00:00:00 2001
>From: Stef Walter <stefw@redhat.com>
>Date: Fri, 3 Oct 2014 09:42:27 +0200
>Subject: [PATCH] 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 <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..fdfeba8 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
>+			if (state->px)
>+				proxy_free (state->px);
>+		} 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..35491ec 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 == 0)
> 		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 != 0, 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);
> }
>-- 
>2.1.0

Actions: View | Splinter Review

Attachments on bug 84567: 107220 | 107222 | 107246 | 107280 | 109773

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.