> diff --git a/usr/lib/pkcs11/common/obj_mgr.c b/usr/lib/pkcs11/common/obj_mgr.c
> index 64eda19..f8492dc 100755
> --- a/usr/lib/pkcs11/common/obj_mgr.c
> +++ b/usr/lib/pkcs11/common/obj_mgr.c
> @@ -299,6 +299,8 @@
> #include <stdio.h>
> #include <string.h> // for memcmp() et al
> #include <strings.h>
> +#include <setjmp.h>
> +#include <signal.h>
>
> #include "pkcs11types.h"
> #include "defs.h"
> @@ -310,6 +312,13 @@
>
> pthread_rwlock_t obj_list_rw_mutex = PTHREAD_RWLOCK_INITIALIZER;
>
> +static jmp_buf objsigbuf;
> +
> +void objsegvhandler(int signum, siginfo_t *info, void *ptr)
> +{
> + siglongjmp(objsigbuf, 1);
Same comment here as I had in the last mail, just add some
indication this is the rc from a sigsegv.
> +}
> +
> CK_RV
> object_mgr_add( SESSION * sess,
> CK_ATTRIBUTE * pTemplate,
> @@ -536,7 +545,6 @@ object_mgr_add_to_map( SESSION * sess,
> st_err_log(0, __FILE__, __LINE__);
> return CKR_HOST_MEMORY;
> }
> - map_node->handle = next_object_handle++;
> map_node->session = sess;
> map_node->ptr = obj;
>
> @@ -551,6 +559,10 @@ object_mgr_add_to_map( SESSION * sess,
> return CKR_FUNCTION_FAILED;
> }
> object_map = dlist_add_as_first( object_map, map_node );
While looking into this code I saw a problem not with your patch,
but in the dlist add routines. If malloc() inside
dlist_add_as_first() fails, the global object_map variable is blown
away by getting set to NULL. Also no error is logged in that case. :-)
Yikes. dlist_add_as_last() does the same. Ping me if you want me to
open a bug for this...
> + map_node->handle = (CK_SESSION_HANDLE) object_map; // handle is the
> DL_NODE pointer and
> + //
> dlist_add_as_first() will return
> + // this newly added
> DL_NODE as it is the
> + // new head of the
> list
> pthread_rwlock_unlock(&obj_list_rw_mutex);
>
> *handle = map_node->handle;
> @@ -1176,13 +1188,23 @@ CK_RV
> object_mgr_find_in_map_nocache( CK_OBJECT_HANDLE handle,
> OBJECT ** ptr )
> {
> - DL_NODE * node = NULL;
> - OBJECT * obj = NULL;
> + DL_NODE * node = NULL;
> + OBJECT_MAP * map = NULL;
> + OBJECT * obj = NULL;
> + CK_RV result = CKR_OK;
> +
> + struct sigaction segvact, oldact;
>
> if (!ptr){
> st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> return CKR_FUNCTION_FAILED;
> }
> +
> + if (!handle) {
> + st_err_log(30, __FILE__, __LINE__);
> + return CKR_OBJECT_HANDLE_INVALID;
> + }
> +
> //
> // no mutex here. the calling function should have locked the mutex
> //
> @@ -1191,20 +1213,52 @@ object_mgr_find_in_map_nocache( CK_OBJECT_HANDLE
> handle,
> st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> return CKR_FUNCTION_FAILED;
> }
> - node = object_map;
> - while (node) {
> - OBJECT_MAP *map = (OBJECT_MAP *)node->data;
> + node = (DL_NODE *) handle;
> +
> + if (sigsetjmp(objsigbuf, 1) == 0) {
> + memset(&segvact, 0, sizeof(segvact));
> + segvact.sa_sigaction = objsegvhandler;
> + segvact.sa_flags = SA_SIGINFO | SA_RESETHAND;
>
> - if (map->handle == handle) {
> + // Register Segfault signal handler, so we can derefence 'node'
> + // safely (or sort of)
> + if (sigaction(SIGSEGV, &segvact, &oldact) != 0) {
> + st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> + result = CKR_FUNCTION_FAILED;
> + goto out;
> + }
> +
> + // Try dereferencing it, and check if we have the right handle
> + map = (OBJECT_MAP *) node->data;
> + if ( map->handle == handle ){
> obj = map->ptr;
> - break;
> }
>
> - node = node->next;
> + // restore the old signal handler
> + if (sigaction(SIGSEGV, &oldact, NULL) != 0) {
> + // Even if we *have* the right handle & could recover, failing to
> + // restore the handler might indicate problems.
> + st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> + result = CKR_FUNCTION_FAILED;
> + goto out;
> + }
> + }
> + else {
In this path, doesn't oldact stay registered as we exit? Shouldn't
the if block above be moved down to be common to both the segfault
case and the non-segfault case? I am new at reading the flow of these
signal catchers. :-)
> + // If we are here, means that a segfault was caught and we somehow
> recovered
> + // result: the object handle was invalid
> + result = CKR_OK;
> + obj = NULL;
> }
> +
> +
> +out:
> pthread_rwlock_unlock(&obj_list_rw_mutex);
> +
> + if (result != CKR_OK) {
> + return result;
> + }
>
> - if (obj == NULL || node == NULL) {
> + if (obj == NULL) {
> st_err_log(30, __FILE__, __LINE__);
> return CKR_OBJECT_HANDLE_INVALID;
> }
> @@ -1232,13 +1286,22 @@ CK_RV
> object_mgr_find_in_map1( CK_OBJECT_HANDLE handle,
> OBJECT ** ptr )
> {
> - DL_NODE * node = NULL;
> - OBJECT * obj = NULL;
> + DL_NODE * node = NULL;
> + OBJECT_MAP * map = NULL;
> + OBJECT * obj = NULL;
> + CK_RV result = CKR_OK;
> +
> + struct sigaction segvact, oldact;
>
> if (!ptr){
> st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> return CKR_FUNCTION_FAILED;
> }
> +
> + if (!handle){
> + st_err_log(30, __FILE__, __LINE__);
> + return CKR_OBJECT_HANDLE_INVALID;
> + }
> //
> // no mutex here. the calling function should have locked the mutex
> //
> @@ -1247,20 +1310,54 @@ object_mgr_find_in_map1( CK_OBJECT_HANDLE handle,
> st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> return CKR_FUNCTION_FAILED;
> }
> - node = object_map;
> - while (node) {
> - OBJECT_MAP *map = (OBJECT_MAP *)node->data;
> -
> - if (map->handle == handle) {
> + node = (DL_NODE *) handle;
> +
> + // FIXME: the below will segfault when the app gives us an invalid
> + // handle
> + if (sigsetjmp(objsigbuf, 1) == 0) {
> + memset(&segvact, 0, sizeof(segvact));
> + segvact.sa_sigaction = objsegvhandler;
> + segvact.sa_flags = SA_SIGINFO | SA_RESETHAND;
> +
> + // Register Segfault signal handler, so we can derefence 'node'
> + // safely (or sort of)
> + if (sigaction(SIGSEGV, &segvact, &oldact) != 0) {
> + st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> + result = CKR_FUNCTION_FAILED;
> + goto out;
> + }
> +
> + // Try dereferencing it, and check if we have the right handle
> + map = (OBJECT_MAP *) node->data;
> + if ( handle == map->handle ){
> obj = map->ptr;
> - break;
> }
>
> - node = node->next;
> + // restore the old signal handler
> + if (sigaction(SIGSEGV, &oldact, NULL) != 0) {
> + // Even if we *have* the right handle & could recover, failing to
> + // restore the handler might indicate problems.
> + st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> + result = CKR_FUNCTION_FAILED;
> + goto out;
> + }
> + }
> + else {
Same question here as above.
Kent
> + // If we are here, means that a segfault was caught and we somehow
> recovered
> + // result: the object handle was invalid
> + result = CKR_OK;
> + obj = NULL;
> }
> +
> +out:
> pthread_rwlock_unlock(&obj_list_rw_mutex);
>
> - if (obj == NULL || node == NULL) {
> + if (result != CKR_OK) {
> + return result;
> + }
> +
> +
> + if (obj == NULL) {
> st_err_log(30, __FILE__, __LINE__);
> return CKR_OBJECT_HANDLE_INVALID;
> }
> @@ -1646,34 +1743,39 @@ done:
> CK_BBOOL
> object_mgr_invalidate_handle1( CK_OBJECT_HANDLE handle )
> {
> - DL_NODE *node = NULL;
> + DL_NODE *node = NULL;
> + OBJECT_MAP *map = NULL;
> + CK_BBOOL result;
>
> //
> // no mutex stuff here. the calling routine should have locked the mutex
> //
>
> + if (!handle){
> + return FALSE;
> + }
> +
> if (pthread_rwlock_wrlock(&obj_list_rw_mutex)) {
> st_err_log(4, __FILE__, __LINE__, __FUNCTION__);
> - return CKR_FUNCTION_FAILED;
> + return FALSE; // FIXME: Proper error messages
> }
> - node = object_map;
> -
> - while (node) {
> - OBJECT_MAP *map = (OBJECT_MAP *)node->data;
>
> - // I think we can do this because even token objects exist in RAM
> - //
> - if (map->handle == handle) {
> - object_map = dlist_remove_node( object_map, node );
> - free( map );
> - pthread_rwlock_unlock(&obj_list_rw_mutex);
> - return TRUE;
> - }
> + node = (DL_NODE *)handle;
> + // FIXME: The below will segfault whenever the caller uses an invalid
> + // handle
> + map = (OBJECT_MAP *)node->data;
>
> - node = node->next;
> + if ( handle == map->handle ) {
> + object_map = dlist_remove_node( object_map, node );
> + free( map );
> + result = TRUE;
> }
> + else {
> + result = FALSE;
> + }
> +
> pthread_rwlock_unlock(&obj_list_rw_mutex);
> - return FALSE;
> + return result;
>
> }
>
> --
> 1.6.6.1
>
>
> ------------------------------------------------------------------------------
> ThinkGeek and WIRED's GeekDad team up for the Ultimate
> GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the
> lucky parental unit. See the prize list and enter to win:
> http://p.sf.net/sfu/thinkgeek-promo
> _______________________________________________
> Opencryptoki-tech mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opencryptoki-tech
>
------------------------------------------------------------------------------
ThinkGeek and WIRED's GeekDad team up for the Ultimate
GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the
lucky parental unit. See the prize list and enter to win:
http://p.sf.net/sfu/thinkgeek-promo
_______________________________________________
Opencryptoki-tech mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opencryptoki-tech
