]> granicus.if.org Git - p11-kit/commitdiff
p11-kit: Use pthread_atfork() in a safe manner
authorStef Walter <stefw@redhat.com>
Fri, 3 Oct 2014 07:42:27 +0000 (09:42 +0200)
committerStef Walter <stefw@redhat.com>
Fri, 3 Oct 2014 18:56:16 +0000 (20:56 +0200)
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
common/library.h
common/mock.c
p11-kit/modules.c
p11-kit/proxy.c
p11-kit/proxy.h
p11-kit/rpc-client.c
p11-kit/test-proxy.c
p11-kit/test-rpc.c

index b7d69236bceb67b2ac4e3baced378499981039de..502ea98e7a3126ac619a5fdcf46d2f3c1344166c 100644 (file)
@@ -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
index 33a33fb787b427cbfa66307eea9da41b8ac3f896..f87494d9909e0eaee157428ddc4d563c0347164f 100644 (file)
@@ -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);
index 01e095dd085548fa4782bbd26c26db43dbdaeb23..a73ae9dcdb4b3f0c83ee38f5b9099f0d6fd304df 100644 (file)
@@ -46,6 +46,7 @@
 #include "debug.h"
 #include "dict.h"
 #include "array.h"
+#include "library.h"
 
 #include <assert.h>
 #include <ctype.h>
index 8aaa769b02553b4bcd624ea12c028527e9c145ba..38c752b5faa88cb5be260a11a4ffadcdeb0fa61d 100644 (file)
@@ -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.
index 3e76f15a866f5234c41435d24d3b28bd08541928..db2acb811389f8381566a9da3b29773cc7556c1c 100644 (file)
@@ -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);
index df05be0a6413998a8ea1186e9ec62267430088d6..f3d56d74f4c9bcce0c1ed81f74c9f0b100e46a9c 100644 (file)
@@ -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);
index 23cfcfcdf2efcf5800552651b533181c0100bbbe..8607bb5cdd593724cf2dae0d0c3935426c7b6fec 100644 (file)
@@ -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);
 
index bf5007d8338a1d5d3f8955a88e928ac740ee466e..e4998be57368acd4ddd02c982ccee151928909b5 100644 (file)
@@ -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 ();
 }
index 8c20a406f969b6df553f1bafe682db636ff91443..c9f8333f50223e5810632d39ca05c22f304ade8e 100644 (file)
@@ -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);
 }