From caa953cba4d2d0cdd4823eb2f1c4f24bbf18a231 Mon Sep 17 00:00:00 2001 From: Stef Walter Date: Sun, 13 May 2012 22:27:07 +0200 Subject: Preconditions to check for input probs and out of memory * We don't try to guarantee completely robust and problem free behavior in cases where the caller or process isn't behaving. We consider these to be outside of our control. --- HACKING | 31 ++++++++++++ p11-kit/conf.c | 76 +++++++++--------------------- p11-kit/debug.c | 19 ++++++++ p11-kit/debug.h | 30 ++++++++++++ p11-kit/modules.c | 85 +++++++++++++++++---------------- p11-kit/pin.c | 138 ++++++++++++++++++++++++++---------------------------- p11-kit/proxy.c | 27 ++++------- p11-kit/uri.c | 112 ++++++++++++++++++++------------------------ p11-kit/uri.h | 4 +- p11-kit/util.c | 9 ---- p11-kit/util.h | 2 - tests/Makefile.am | 3 +- tests/test-init.c | 68 ++++++++++++++++++++++++++- tests/uri-test.c | 12 ++++- 14 files changed, 352 insertions(+), 264 deletions(-) create mode 100644 HACKING diff --git a/HACKING b/HACKING new file mode 100644 index 0000000..63454f8 --- /dev/null +++ b/HACKING @@ -0,0 +1,31 @@ +HACKING p11-kit + + * Website: http://p11-glue.freedesktop.org/p11-kit.html + + * Mailing list: p11-glue@lists.freedesktop.org + + * Bugs: https://bugs.freedesktop.org/enter_bug.cgi?product=p11-glue + +PRECONDITIONS and UNEXPECTED SYSTEM ISSUES + +We don't try to guarantee completely robust and problem free behavior in cases +where the caller or process isn't behaving. We consider these to be outside of +our control: + + * Broken input from callers. We use preconditions to check input + and immediately return. + + * Out of memory. It is pretty much impossible to handle out of memory + errors correctly. Handling them alongside other errors is naive and + broken. + + We do check the results from all memory allocations. + + As a nod to the behavior of callers of this library, we don't abort on + memory allocation failures. We use preconditions with somewhat sane results. + + We don't try to guarantee library state (such as locks or memory leaks) + when memory allocation fails. + + Exception: when reading files or allocating potentially unbounded amounts + of memory, we should respond robustly to memory allocation failures. diff --git a/p11-kit/conf.c b/p11-kit/conf.c index 917ce4c..fdb591d 100644 --- a/p11-kit/conf.c +++ b/p11-kit/conf.c @@ -132,10 +132,7 @@ strconcat (const char *first, va_end (va); at = result = malloc (length + 1); - if (!result) { - errno = ENOMEM; - return NULL; - } + return_val_if_fail (result != NULL, NULL); va_start (va, first); @@ -172,8 +169,7 @@ read_config_file (const char* filename, int flags) (error == ENOENT || error == ENOTDIR)) { _p11_debug ("config file does not exist"); config = strdup ("\n"); - if (!config) - errno = ENOMEM; + return_val_if_fail (config != NULL, NULL); return config; } _p11_message ("couldn't open config file: %s: %s", filename, @@ -192,8 +188,9 @@ read_config_file (const char* filename, int flags) return NULL; } - if ((config = (char*)malloc (len + 2)) == NULL) { - _p11_message ("out of memory"); + config = malloc (len + 2); + if (config == NULL) { + _p11_message ("config file is too large to read into memory: %lu", len); errno = ENOMEM; return NULL; } @@ -231,24 +228,11 @@ _p11_conf_merge_defaults (hashmap *map, hashmap *defaults) if (_p11_hash_get (map, key)) continue; key = strdup (key); - if (key == NULL) { - errno = ENOMEM; - return -1; - } + return_val_if_fail (key != NULL, -1); value = strdup (value); - if (value == NULL) { - free (key); - errno = ENOMEM; - return -1; - } - if (!_p11_hash_set (map, key, value)) { - free (key); - free (value); - errno = ENOMEM; - return -1; - } - key = NULL; - value = NULL; + return_val_if_fail (key != NULL, -1); + if (!_p11_hash_set (map, key, value)) + return_val_if_reached (-1); } return 0; @@ -275,11 +259,8 @@ _p11_conf_parse_file (const char* filename, int flags) return NULL; map = _p11_hash_create (_p11_hash_string_hash, _p11_hash_string_equal, free, free); - if (map == NULL) { - free (data); - errno = ENOMEM; - return NULL; - } + return_val_if_fail (map != NULL, NULL); + next = data; /* Go through lines and process them */ @@ -308,25 +289,15 @@ _p11_conf_parse_file (const char* filename, int flags) value = strtrim (value); name = strdup (name); - if (!name) { - error = ENOMEM; - break; - } + return_val_if_fail (name != NULL, NULL); + value = strdup (value); - if (!value) { - free (name); - error = ENOMEM; - break; - } + return_val_if_fail (value != NULL, NULL); _p11_debug ("config value: %s: %s", name, value); - if (!_p11_hash_set (map, name, value)) { - free (name); - free (value); - error = ENOMEM; - break; - } + if (!_p11_hash_set (map, name, value)) + return_val_if_reached (NULL); } free (data); @@ -504,12 +475,10 @@ load_config_from_file (const char *configfile, const char *name, hashmap *config prev = _p11_hash_get (configs, name); if (prev == NULL) { key = strdup (name); - if (key == NULL) - error = ENOMEM; - else if (!_p11_hash_set (configs, key, config)) - error = errno; - else - config = NULL; + return_val_if_fail (key != NULL, -1); + if (!_p11_hash_set (configs, key, config)) + return_val_if_reached (-1); + config = NULL; } else { if (_p11_conf_merge_defaults (prev, config) < 0) error = errno; @@ -554,10 +523,7 @@ load_configs_from_directory (const char *directory, hashmap *configs) /* We're within a global mutex, so readdir is safe */ while ((dp = readdir(dir)) != NULL) { path = strconcat (directory, "/", dp->d_name, NULL); - if (!path) { - error = ENOMEM; - break; - } + return_val_if_fail (path != NULL, -1); is_dir = 0; #ifdef HAVE_STRUCT_DIRENT_D_TYPE diff --git a/p11-kit/debug.c b/p11-kit/debug.c index 37095be..a665d0f 100644 --- a/p11-kit/debug.c +++ b/p11-kit/debug.c @@ -59,6 +59,7 @@ static struct DebugKey debug_keys[] = { }; static int debug_inited = 0; +static int debug_strict = 0; /* global variable exported in debug.h */ int _p11_debug_current_flags = ~0; @@ -72,6 +73,10 @@ parse_environ_flags (void) const char *q; int i; + env = getenv ("P11_KIT_STRICT"); + if (env && env[0] != '\0') + debug_strict = 1; + env = getenv ("P11_KIT_DEBUG"); if (!env) return 0; @@ -130,3 +135,17 @@ _p11_debug_message (int flag, fprintf (stderr, "(p11-kit:%d) %s\n", getpid(), buffer); } } + +void +_p11_debug_precond (const char *format, + ...) +{ + va_list va; + + va_start (va, format); + vfprintf (stderr, format, va); + va_end (va); + + if (debug_strict) + abort (); +} diff --git a/p11-kit/debug.h b/p11-kit/debug.h index 7fa9854..c6e26f5 100644 --- a/p11-kit/debug.h +++ b/p11-kit/debug.h @@ -53,6 +53,36 @@ void _p11_debug_message (int flag, const char *format, ...) GNUC_PRINTF (2, 3); +void _p11_debug_precond (const char *format, + ...) GNUC_PRINTF (1, 2); + +#define assert_not_reached() \ + (assert (0 && "this code should not be reached")) + +#define return_val_if_fail(x, v) \ + do { if (!(x)) { \ + _p11_debug_precond ("p11-kit: '%s' not true at %s\n", #x, __func__); \ + return v; \ + } } while (0) + +#define return_if_fail(x) \ + do { if (!(x)) { \ + _p11_debug_precond ("p11-kit: '%s' not true at %s\n", #x, __func__); \ + return; \ + } } while (0) + +#define return_if_reached() \ + do { \ + _p11_debug_precond ("p11-kit: shouldn't be reached at %s\n", __func__); \ + return; \ + } while (0) + +#define return_val_if_reached(v) \ + do { \ + _p11_debug_precond ("p11-kit: shouldn't be reached at %s\n", __func__); \ + return v; \ + } while (0) + #endif /* DEBUG_H */ /* ----------------------------------------------------------------------------- diff --git a/p11-kit/modules.c b/p11-kit/modules.c index 5eb97fd..0694895 100644 --- a/p11-kit/modules.c +++ b/p11-kit/modules.c @@ -130,12 +130,11 @@ create_mutex (CK_VOID_PTR_PTR mut) { mutex_t *pmutex; - if (mut == NULL) - return CKR_ARGUMENTS_BAD; + return_val_if_fail (mut != NULL, CKR_ARGUMENTS_BAD); pmutex = malloc (sizeof (mutex_t)); - if (!pmutex) - return CKR_HOST_MEMORY; + return_val_if_fail (pmutex != NULL, CKR_HOST_MEMORY); + _p11_mutex_init (pmutex); *mut = pmutex; return CKR_OK; @@ -146,8 +145,7 @@ destroy_mutex (CK_VOID_PTR mut) { mutex_t *pmutex = mut; - if (mut == NULL) - return CKR_MUTEX_BAD; + return_val_if_fail (mut != NULL, CKR_MUTEX_BAD); _p11_mutex_uninit (pmutex); free (pmutex); @@ -159,8 +157,7 @@ lock_mutex (CK_VOID_PTR mut) { mutex_t *pmutex = mut; - if (mut == NULL) - return CKR_MUTEX_BAD; + return_val_if_fail (mut != NULL, CKR_MUTEX_BAD); _p11_mutex_lock (pmutex); return CKR_OK; @@ -171,8 +168,7 @@ unlock_mutex (CK_VOID_PTR mut) { mutex_t *pmutex = mut; - if (mut == NULL) - return CKR_MUTEX_BAD; + return_val_if_fail (mut != NULL, CKR_MUTEX_BAD); _p11_mutex_unlock (pmutex); return CKR_OK; @@ -183,7 +179,7 @@ free_module_unlocked (void *data) { Module *mod = data; - assert (mod); + assert (mod != NULL); /* Module must be finalized */ assert (mod->initialize_called == 0); @@ -201,14 +197,13 @@ free_module_unlocked (void *data) free (mod); } -static Module* +static Module * alloc_module_unlocked (void) { Module *mod; mod = calloc (1, sizeof (Module)); - if (!mod) - return NULL; + return_val_if_fail (mod != NULL, NULL); mod->init_args.CreateMutex = create_mutex; mod->init_args.DestroyMutex = destroy_mutex; @@ -238,16 +233,15 @@ build_path (const char *dir, const char *filename) assert (filename); len = snprintf (NULL, 0, "%s/%s", dir, filename) + 1; - if (len <= 0) - return NULL; + return_val_if_fail (len > 0, NULL); #ifdef PATH_MAX if (len > PATH_MAX) return NULL; #endif - if (!(path = malloc (len))) - return NULL; + path = malloc (len); + return_val_if_fail (path != NULL, NULL); sprintf (path, "%s/%s", dir, filename); @@ -295,8 +289,7 @@ load_module_from_file_unlocked (const char *path, Module **result) CK_RV rv; mod = alloc_module_unlocked (); - if (!mod) - return CKR_HOST_MEMORY; + return_val_if_fail (mod != NULL, CKR_HOST_MEMORY); rv = dlopen_and_get_function_list (mod, path); if (rv != CKR_OK) { @@ -313,8 +306,7 @@ load_module_from_file_unlocked (const char *path, Module **result) mod = prev; } else if (!_p11_hash_set (gl.modules, mod->funcs, mod)) { - free_module_unlocked (mod); - return CKR_HOST_MEMORY; + return_val_if_reached (CKR_HOST_MEMORY); } if (result) @@ -415,18 +407,14 @@ take_config_and_load_module_unlocked (char **name, hashmap **config) } path = expand_module_path (module_filename); - if (!path) - return CKR_HOST_MEMORY; + return_val_if_fail (path != NULL, CKR_HOST_MEMORY); /* The hash map will take ownership of the variable */ - if (!_p11_hash_set (*config, "module", path)) { - free (path); - return CKR_HOST_MEMORY; - } + if (!_p11_hash_set (*config, "module", path)) + return_val_if_reached (CKR_HOST_MEMORY); mod = alloc_module_unlocked (); - if (!mod) - return CKR_HOST_MEMORY; + return_val_if_fail (mod != NULL, CKR_HOST_MEMORY); /* Take ownership of thes evariables */ mod->config = *config; @@ -464,11 +452,8 @@ take_config_and_load_module_unlocked (char **name, hashmap **config) /* Add this new module to our hash table */ } else { - if (!_p11_hash_set (gl.modules, mod->funcs, mod)) { - free_module_unlocked (mod); - return CKR_HOST_MEMORY; - } - + if (!_p11_hash_set (gl.modules, mod->funcs, mod)) + return_val_if_reached (CKR_HOST_MEMORY); } return CKR_OK; @@ -492,14 +477,14 @@ load_registered_modules_unlocked (void) /* Load the global configuration files */ config = _p11_conf_load_globals (P11_SYSTEM_CONFIG_FILE, P11_USER_CONFIG_FILE, &mode); if (config == NULL) - return (errno == ENOMEM) ? CKR_HOST_MEMORY : CKR_GENERAL_ERROR; + return CKR_GENERAL_ERROR; assert (mode != CONF_USER_INVALID); configs = _p11_conf_load_modules (mode, P11_SYSTEM_CONFIG_MODULES, P11_USER_CONFIG_MODULES); if (configs == NULL) { - rv = (errno == ENOMEM) ? CKR_HOST_MEMORY : CKR_GENERAL_ERROR; + rv = CKR_GENERAL_ERROR; _p11_hash_free (config); return rv; } @@ -514,7 +499,7 @@ load_registered_modules_unlocked (void) _p11_hash_iterate (configs, &iter); while (_p11_hash_next (&iter, &key, NULL)) { if (!_p11_hash_steal (configs, key, (void**)&name, (void**)&config)) - assert (0 && "not reached"); + assert_not_reached (); /* Is this a critical module, should abort loading of others? */ critical = _p11_conf_parse_boolean (_p11_hash_get (config, "critical"), 0); @@ -626,11 +611,11 @@ init_globals_unlocked (void) { static int once = 0; - if (!gl.modules) + if (!gl.modules) { gl.modules = _p11_hash_create (_p11_hash_direct_hash, _p11_hash_direct_equal, NULL, free_module_unlocked); - if (!gl.modules) - return CKR_HOST_MEMORY; + return_val_if_fail (gl.modules != NULL, CKR_HOST_MEMORY); + } if (once) return CKR_OK; @@ -890,9 +875,10 @@ _p11_kit_registered_modules_unlocked (void) hashiter iter; int i = 0; - if (gl.modules) + if (gl.modules) { result = calloc (_p11_hash_size (gl.modules) + 1, sizeof (CK_FUNCTION_LIST_PTR)); - if (result) { + return_val_if_fail (result != NULL, NULL); + _p11_hash_iterate (gl.modules, &iter); while (_p11_hash_next (&iter, NULL, (void **)&mod)) { @@ -966,6 +952,8 @@ p11_kit_registered_module_to_name (CK_FUNCTION_LIST_PTR module) Module *mod; char *name = NULL; + return_val_if_fail (module != NULL, NULL); + _p11_library_init_once (); _p11_lock (); @@ -997,6 +985,8 @@ p11_kit_registered_name_to_module (const char *name) CK_FUNCTION_LIST_PTR module = NULL; Module *mod; + return_val_if_fail (name != NULL, NULL); + _p11_lock (); _p11_kit_clear_message (); @@ -1032,6 +1022,8 @@ p11_kit_registered_option (CK_FUNCTION_LIST_PTR module, const char *field) char *option = NULL; hashmap *config = NULL; + return_val_if_fail (field != NULL, NULL); + _p11_library_init_once (); _p11_lock (); @@ -1095,6 +1087,8 @@ p11_kit_initialize_module (CK_FUNCTION_LIST_PTR module) Module *mod; CK_RV rv = CKR_OK; + return_val_if_fail (module != NULL, CKR_ARGUMENTS_BAD); + _p11_library_init_once (); /* WARNING: This function must be reentrant for the same arguments */ @@ -1180,6 +1174,8 @@ p11_kit_finalize_module (CK_FUNCTION_LIST_PTR module) Module *mod; CK_RV rv = CKR_OK; + return_val_if_fail (module != NULL, CKR_ARGUMENTS_BAD); + _p11_library_init_once (); /* WARNING: This function must be reentrant for the same arguments */ @@ -1246,6 +1242,9 @@ p11_kit_load_initialize_module (const char *module_path, Module *mod; CK_RV rv = CKR_OK; + return_val_if_fail (module_path != NULL, CKR_ARGUMENTS_BAD); + return_val_if_fail (module != NULL, CKR_ARGUMENTS_BAD); + _p11_library_init_once (); /* WARNING: This function must be reentrant for the same arguments */ diff --git a/p11-kit/pin.c b/p11-kit/pin.c index 7c3a1d8..ff09d57 100644 --- a/p11-kit/pin.c +++ b/p11-kit/pin.c @@ -171,6 +171,38 @@ unref_pin_callback (void *pointer) } } +static int +register_callback_unlocked (const char *pin_source, + PinCallback *cb) +{ + ptr_array_t *callbacks = NULL; + char *name; + + name = strdup (pin_source); + return_val_if_fail (name != NULL, -1); + + if (gl.pin_sources == NULL) { + gl.pin_sources = _p11_hash_create (_p11_hash_string_hash, _p11_hash_string_equal, + free, (hash_destroy_func)_p11_ptr_array_free); + return_val_if_fail (gl.pin_sources != NULL, -1); + } + + if (gl.pin_sources != NULL) + callbacks = _p11_hash_get (gl.pin_sources, name); + + if (callbacks == NULL) { + callbacks = _p11_ptr_array_create (unref_pin_callback); + return_val_if_fail (callbacks != NULL, -1); + if (!_p11_hash_set (gl.pin_sources, name, callbacks)) + return_val_if_reached (-1); + } + + if (_p11_ptr_array_add (callbacks, cb) < 0) + return_val_if_reached (-1); + + return 0; +} + /** * p11_kit_pin_register_callback: * @pin_source: the 'pin-source' attribute this this callback is for @@ -187,30 +219,22 @@ unref_pin_callback (void *pointer) * If multiple callbacks are registered for the same @pin_source value, then * the last registered callback will be the first to be called. * - * Returns: Returns negative if registering fails. This can only happen if - * memory cannot be allocated. + * Returns: Returns negative if registering fails. */ int -p11_kit_pin_register_callback (const char *pin_source, p11_kit_pin_callback callback, - void *callback_data, p11_kit_pin_destroy_func callback_destroy) +p11_kit_pin_register_callback (const char *pin_source, + p11_kit_pin_callback callback, + void *callback_data, + p11_kit_pin_destroy_func callback_destroy) { - ptr_array_t *callbacks = NULL; PinCallback *cb; - char *name; int ret; - cb = calloc (1, sizeof (PinCallback)); - if (cb == NULL) { - errno = ENOMEM; - return -1; - } + return_val_if_fail (pin_source != NULL, -1); + return_val_if_fail (callback != NULL, -1); - name = strdup (pin_source); - if (name == NULL) { - free (cb); - errno = ENOMEM; - return -1; - } + cb = calloc (1, sizeof (PinCallback)); + return_val_if_fail (cb != NULL, -1); cb->refs = 1; cb->func = callback; @@ -219,51 +243,10 @@ p11_kit_pin_register_callback (const char *pin_source, p11_kit_pin_callback call _p11_lock (); - if (gl.pin_sources == NULL) { - gl.pin_sources = _p11_hash_create (_p11_hash_string_hash, _p11_hash_string_equal, - free, (hash_destroy_func)_p11_ptr_array_free); - if (gl.pin_sources == NULL) { - errno = ENOMEM; - ret = -1; - } - } - - if (gl.pin_sources != NULL) - callbacks = _p11_hash_get (gl.pin_sources, pin_source); - - if (callbacks == NULL) { - callbacks = _p11_ptr_array_create (unref_pin_callback); - if (callbacks == NULL) { - errno = ENOMEM; - ret = -1; - } else if (!_p11_hash_set (gl.pin_sources, name, callbacks)) { - _p11_ptr_array_free (callbacks); - callbacks = NULL; - errno = ENOMEM; - ret = -1; - } else { - /* Note that we've consumed the name */ - name = NULL; - } - } - - if (callbacks != NULL) { - if (_p11_ptr_array_add (callbacks, cb) < 0) { - errno = ENOMEM; - ret = -1; - } else { - /* Note that we've consumed the callback */ - cb = NULL; - } - } + ret = register_callback_unlocked (pin_source, cb); _p11_unlock (); - /* Unless consumed above */ - free (name); - if (cb != NULL) - unref_pin_callback (cb); - return ret; } @@ -279,13 +262,17 @@ p11_kit_pin_register_callback (const char *pin_source, p11_kit_pin_callback call * removed. */ void -p11_kit_pin_unregister_callback (const char *pin_source, p11_kit_pin_callback callback, +p11_kit_pin_unregister_callback (const char *pin_source, + p11_kit_pin_callback callback, void *callback_data) { PinCallback *cb; ptr_array_t *callbacks; unsigned int i; + return_if_fail (pin_source != NULL); + return_if_fail (callback != NULL); + _p11_lock (); if (gl.pin_sources) { @@ -344,9 +331,11 @@ p11_kit_pin_unregister_callback (const char *pin_source, p11_kit_pin_callback ca * Returns: the PIN which should be released with p11_kit_pin_unref(), or %NULL * if no callback was registered or could proivde a PIN */ -P11KitPin* -p11_kit_pin_request (const char *pin_source, P11KitUri *pin_uri, - const char *pin_description, P11KitPinFlags pin_flags) +P11KitPin * +p11_kit_pin_request (const char *pin_source, + P11KitUri *pin_uri, + const char *pin_description, + P11KitPinFlags pin_flags) { PinCallback **snapshot = NULL; unsigned int snapshot_count = 0; @@ -354,6 +343,8 @@ p11_kit_pin_request (const char *pin_source, P11KitUri *pin_uri, P11KitPin *pin; unsigned int i; + return_val_if_fail (pin_source != NULL, NULL); + _p11_lock (); /* Find and ref the pin source data */ @@ -449,7 +440,7 @@ p11_kit_pin_request (const char *pin_source, P11KitUri *pin_uri, * Returns: a referenced PIN with the pinfile contents, or %NULL if the file * could not be read */ -P11KitPin* +P11KitPin * p11_kit_pin_file_callback (const char *pin_source, P11KitUri *pin_uri, const char *pin_description, @@ -457,11 +448,14 @@ p11_kit_pin_file_callback (const char *pin_source, void *callback_data) { unsigned char *buffer; + unsigned char *memory; size_t used, allocated; int error = 0; int fd; int res; + return_val_if_fail (pin_source != NULL, NULL); + /* We don't support retries */ if (pin_flags & P11_KIT_PIN_FLAGS_RETRY) return NULL; @@ -476,11 +470,13 @@ p11_kit_pin_file_callback (const char *pin_source, for (;;) { if (used + 256 > allocated) { - buffer = _p11_realloc (buffer, used + 1024); - if (buffer == NULL) { + memory = realloc (buffer, used + 1024); + if (memory == NULL) { + free (buffer); error = ENOMEM; break; } + buffer = memory; allocated = used + 1024; } @@ -542,13 +538,12 @@ p11_kit_pin_new (const unsigned char *value, size_t length) P11KitPin *pin; copy = malloc (length); - if (copy == NULL) - return NULL; + return_val_if_fail (copy != NULL, NULL); memcpy (copy, value, length); pin = p11_kit_pin_new_for_buffer (copy, length, free); - if (pin == NULL) - free (copy); + return_val_if_fail (pin != NULL, NULL); + return pin; } @@ -605,8 +600,7 @@ p11_kit_pin_new_for_buffer (unsigned char *buffer, size_t length, P11KitPin *pin; pin = calloc (1, sizeof (P11KitPin)); - if (pin == NULL) - return NULL; + return_val_if_fail (pin != NULL, NULL); pin->ref_count = 1; pin->buffer = buffer; diff --git a/p11-kit/proxy.c b/p11-kit/proxy.c index 882924e..bae25d3 100644 --- a/p11-kit/proxy.c +++ b/p11-kit/proxy.c @@ -257,10 +257,7 @@ initialize_mappings_unlocked_reentrant (void) rv = (funcs->C_GetSlotList) (FALSE, NULL, &count); if (rv == CKR_OK && count) { slots = calloc (sizeof (CK_SLOT_ID), count); - if (!slots) - rv = CKR_HOST_MEMORY; - else - rv = (funcs->C_GetSlotList) (FALSE, slots, &count); + rv = (funcs->C_GetSlotList) (FALSE, slots, &count); } _p11_lock (); @@ -270,12 +267,10 @@ initialize_mappings_unlocked_reentrant (void) break; } - mappings = _p11_realloc (mappings, sizeof (Mapping) * (n_mappings + count)); - if (!mappings) { - free (slots); - rv = CKR_HOST_MEMORY; - break; - } + return_val_if_fail (count == 0 || slots != NULL, CKR_GENERAL_ERROR); + + mappings = realloc (mappings, sizeof (Mapping) * (n_mappings + count)); + return_val_if_fail (mappings != NULL, CKR_HOST_MEMORY); /* And now add a mapping for each of those slots */ for (i = 0; i < count; ++i) { @@ -342,8 +337,7 @@ proxy_C_GetInfo (CK_INFO_PTR info) _p11_library_init_once (); - if (info == NULL) - return CKR_ARGUMENTS_BAD; + return_val_if_fail (info != NULL, CKR_ARGUMENTS_BAD); _p11_lock (); @@ -370,8 +364,7 @@ proxy_C_GetFunctionList (CK_FUNCTION_LIST_PTR_PTR list) { /* Can be called before C_Initialize */ - if (!list) - return CKR_ARGUMENTS_BAD; + return_val_if_fail (list != NULL, CKR_ARGUMENTS_BAD); *list = &proxy_function_list; return CKR_OK; } @@ -386,8 +379,7 @@ proxy_C_GetSlotList (CK_BBOOL token_present, CK_SLOT_ID_PTR slot_list, CK_RV rv = CKR_OK; int i; - if (!count) - return CKR_ARGUMENTS_BAD; + return_val_if_fail (count != NULL, CKR_ARGUMENTS_BAD); _p11_lock (); @@ -503,8 +495,7 @@ proxy_C_OpenSession (CK_SLOT_ID id, CK_FLAGS flags, CK_VOID_PTR user_data, Mapping map; CK_RV rv; - if (handle == NULL) - return CKR_ARGUMENTS_BAD; + return_val_if_fail (handle != NULL, CKR_ARGUMENTS_BAD); rv = map_slot_to_real (&id, &map); if (rv != CKR_OK) diff --git a/p11-kit/uri.c b/p11-kit/uri.c index ffcf6cc..b9e2554 100644 --- a/p11-kit/uri.c +++ b/p11-kit/uri.c @@ -105,7 +105,7 @@ /** * P11KitUriResult: * @P11_KIT_URI_OK: Success - * @P11_KIT_URI_NO_MEMORY: Memory allocation failed + * @P11_KIT_URI_UNEXPECTED: Unexpected or internal system error * @P11_KIT_URI_BAD_SCHEME: The URI had a bad scheme * @P11_KIT_URI_BAD_ENCODING: The URI had a bad encoding * @P11_KIT_URI_BAD_SYNTAX: The URI had a bad syntax @@ -161,8 +161,7 @@ url_decode (const char *value, const char *end, /* String can only get shorter */ result = malloc ((end - value) + 1); - if (!result) - return P11_KIT_URI_NO_MEMORY; + return_val_if_fail (result != NULL, P11_KIT_URI_UNEXPECTED); /* Now loop through looking for escapes */ p = result; @@ -216,8 +215,7 @@ url_encode (const unsigned char *value, const unsigned char *end, size_t *length /* Just allocate for worst case */ result = malloc (((end - value) * 3) + 1); - if (!result) - return NULL; + return_val_if_fail (result != NULL, NULL); /* Now loop through looking for escapes */ p = result; @@ -250,8 +248,7 @@ key_decode (const char *value, const char *end) char *key; key = malloc (length + 1); - if (key == NULL) - return NULL; + return_val_if_fail (key != NULL, NULL); memcpy (key, value, length); key[length] = '\0'; @@ -312,7 +309,7 @@ match_struct_version (CK_VERSION_PTR inuri, CK_VERSION_PTR real) CK_INFO_PTR p11_kit_uri_get_module_info (P11KitUri *uri) { - assert (uri); + return_val_if_fail (uri != NULL, NULL); return &uri->module; } @@ -333,8 +330,8 @@ p11_kit_uri_get_module_info (P11KitUri *uri) int p11_kit_uri_match_module_info (P11KitUri *uri, CK_INFO_PTR info) { - assert (uri); - assert (info); + return_val_if_fail (uri != NULL, 0); + return_val_if_fail (info != NULL, 0); if (uri->unrecognized) return 0; @@ -368,7 +365,7 @@ p11_kit_uri_match_module_info (P11KitUri *uri, CK_INFO_PTR info) CK_TOKEN_INFO_PTR p11_kit_uri_get_token_info (P11KitUri *uri) { - assert (uri); + return_val_if_fail (uri != NULL, NULL); return &uri->token; } @@ -390,8 +387,8 @@ p11_kit_uri_get_token_info (P11KitUri *uri) int p11_kit_uri_match_token_info (P11KitUri *uri, CK_TOKEN_INFO_PTR token_info) { - assert (uri); - assert (token_info); + return_val_if_fail (uri != NULL, 0); + return_val_if_fail (token_info != NULL, 0); if (uri->unrecognized) return 0; @@ -425,7 +422,7 @@ p11_kit_uri_get_attribute (P11KitUri *uri, CK_ATTRIBUTE_TYPE attr_type) { CK_ULONG i; - assert (uri); + return_val_if_fail (uri != NULL, NULL); for (i = 0; i < uri->n_attributes; i++) { if (uri->attributes[i].type == attr_type) @@ -479,8 +476,8 @@ p11_kit_uri_set_attribute (P11KitUri *uri, CK_ATTRIBUTE_PTR attr) CK_ATTRIBUTE copy; CK_ULONG i; - assert (uri); - assert (attr); + return_val_if_fail (uri != NULL, P11_KIT_URI_UNEXPECTED); + return_val_if_fail (uri != NULL, P11_KIT_URI_UNEXPECTED); /* Make sure the attribute type is valid */ for (i = 0; i < NUM_ATTRIBUTE_TYPES; i++) { @@ -495,8 +492,7 @@ p11_kit_uri_set_attribute (P11KitUri *uri, CK_ATTRIBUTE_PTR attr) /* Duplicate the value */ if (attr->pValue && attr->ulValueLen && attr->ulValueLen != (CK_ULONG)-1) { copy.pValue = malloc (attr->ulValueLen); - if (!copy.pValue) - return P11_KIT_URI_NO_MEMORY; + return_val_if_fail (copy.pValue != NULL, P11_KIT_URI_UNEXPECTED); memcpy (copy.pValue, attr->pValue, attr->ulValueLen); } @@ -523,7 +519,7 @@ p11_kit_uri_clear_attribute (P11KitUri *uri, CK_ATTRIBUTE_TYPE attr_type) CK_ATTRIBUTE_PTR last; CK_ULONG i; - assert (uri); + return_val_if_fail (uri != NULL, P11_KIT_URI_UNEXPECTED); /* Make sure the attribute type is valid */ for (i = 0; i < NUM_ATTRIBUTE_TYPES; i++) { @@ -574,8 +570,8 @@ p11_kit_uri_clear_attribute (P11KitUri *uri, CK_ATTRIBUTE_TYPE attr_type) CK_ATTRIBUTE_PTR p11_kit_uri_get_attributes (P11KitUri *uri, CK_ULONG_PTR n_attrs) { - assert (uri); - assert (n_attrs); + return_val_if_fail (uri != NULL, NULL); + return_val_if_fail (n_attrs != NULL, NULL); *n_attrs = uri->n_attributes; return uri->attributes; @@ -588,7 +584,7 @@ p11_kit_uri_set_attributes (P11KitUri *uri, CK_ATTRIBUTE_PTR attrs, CK_ULONG i; int ret; - assert (uri); + return_val_if_fail (uri != NULL, P11_KIT_URI_UNEXPECTED); p11_kit_uri_clear_attributes (uri); @@ -606,7 +602,7 @@ p11_kit_uri_clear_attributes (P11KitUri *uri) { CK_ULONG i; - assert (uri); + return_if_fail (uri != NULL); for (i = 0; i < uri->n_attributes; i++) free (uri->attributes[i].pValue); @@ -653,8 +649,8 @@ p11_kit_uri_match_attributes (P11KitUri *uri, CK_ATTRIBUTE_PTR attrs, CK_ULONG j; CK_ULONG i; - assert (uri); - assert (attrs || !n_attrs); + return_val_if_fail (uri != NULL, 0); + return_val_if_fail (attrs != NULL || n_attrs == 0, 0); if (uri->unrecognized) return 0; @@ -686,7 +682,7 @@ p11_kit_uri_match_attributes (P11KitUri *uri, CK_ATTRIBUTE_PTR attrs, void p11_kit_uri_set_unrecognized (P11KitUri *uri, int unrecognized) { - assert (uri); + return_if_fail (uri != NULL); uri->unrecognized = unrecognized; } @@ -705,7 +701,7 @@ p11_kit_uri_set_unrecognized (P11KitUri *uri, int unrecognized) int p11_kit_uri_any_unrecognized (P11KitUri *uri) { - assert (uri); + return_val_if_fail (uri != NULL, 1); return uri->unrecognized; } @@ -721,7 +717,7 @@ p11_kit_uri_any_unrecognized (P11KitUri *uri) const char* p11_kit_uri_get_pin_source (P11KitUri *uri) { - assert (uri); + return_val_if_fail (uri != NULL, NULL); return uri->pin_source; } @@ -734,6 +730,7 @@ p11_kit_uri_get_pin_source (P11KitUri *uri) const char* p11_kit_uri_get_pinfile (P11KitUri *uri) { + return_val_if_fail (uri != NULL, NULL); return p11_kit_uri_get_pin_source (uri); } @@ -748,9 +745,9 @@ p11_kit_uri_get_pinfile (P11KitUri *uri) void p11_kit_uri_set_pin_source (P11KitUri *uri, const char *pin_source) { - assert (uri); + return_if_fail (uri != NULL); free (uri->pin_source); - uri->pin_source = strdup (pin_source); + uri->pin_source = pin_source ? strdup (pin_source) : NULL; } /** @@ -763,6 +760,7 @@ p11_kit_uri_set_pin_source (P11KitUri *uri, const char *pin_source) void p11_kit_uri_set_pinfile (P11KitUri *uri, const char *pinfile) { + return_if_fail (uri != NULL); p11_kit_uri_set_pin_source (uri, pinfile); } @@ -782,8 +780,7 @@ p11_kit_uri_new (void) P11KitUri *uri; uri = calloc (1, sizeof (P11KitUri)); - if (!uri) - return NULL; + return_val_if_fail (uri != NULL, NULL); /* So that it matches anything */ uri->module.libraryVersion.major = (CK_BYTE)-1; @@ -806,9 +803,8 @@ format_raw_string (char **string, size_t *length, int *is_first, namelen = strlen (name); vallen = strlen (value); - *string = _p11_realloc (*string, *length + namelen + vallen + 3); - if (!*string) - return 0; + *string = realloc (*string, *length + namelen + vallen + 3); + return_val_if_fail (*string != NULL, 0); if (!*is_first) (*string)[(*length)++] = ';'; @@ -832,8 +828,7 @@ format_encode_string (char **string, size_t *length, int *is_first, int ret; encoded = url_encode (value, value + n_value, NULL); - if (!encoded) - return 0; + return_val_if_fail (encoded != NULL, 0); ret = format_raw_string (string, length, is_first, name, encoded); free (encoded); @@ -945,9 +940,11 @@ p11_kit_uri_format (P11KitUri *uri, P11KitUriType uri_type, char **string) size_t length = 0; int is_first = 1; + return_val_if_fail (uri != NULL, P11_KIT_URI_UNEXPECTED); + return_val_if_fail (string != NULL, P11_KIT_URI_UNEXPECTED); + result = malloc (128); - if (!result) - return P11_KIT_URI_NO_MEMORY; + return_val_if_fail (result != NULL, P11_KIT_URI_UNEXPECTED); length = P11_KIT_URI_SCHEME_LEN; memcpy (result, P11_KIT_URI_SCHEME, length); @@ -961,16 +958,14 @@ p11_kit_uri_format (P11KitUri *uri, P11KitUriType uri_type, char **string) !format_struct_string (&result, &length, &is_first, "library-manufacturer", uri->module.manufacturerID, sizeof (uri->module.manufacturerID))) { - free (result); - return P11_KIT_URI_NO_MEMORY; + return_val_if_reached (P11_KIT_URI_UNEXPECTED); } } if ((uri_type & P11_KIT_URI_FOR_MODULE_WITH_VERSION) == P11_KIT_URI_FOR_MODULE_WITH_VERSION) { if (!format_struct_version (&result, &length, &is_first, "library-version", &uri->module.libraryVersion)) { - free (result); - return P11_KIT_URI_NO_MEMORY; + return_val_if_reached (P11_KIT_URI_UNEXPECTED); } } @@ -987,8 +982,7 @@ p11_kit_uri_format (P11KitUri *uri, P11KitUriType uri_type, char **string) !format_struct_string (&result, &length, &is_first, "token", uri->token.label, sizeof (uri->token.label))) { - free (result); - return P11_KIT_URI_NO_MEMORY; + return_val_if_reached (P11_KIT_URI_UNEXPECTED); } } @@ -997,21 +991,21 @@ p11_kit_uri_format (P11KitUri *uri, P11KitUriType uri_type, char **string) p11_kit_uri_get_attribute (uri, CKA_ID)) || !format_attribute_string (&result, &length, &is_first, "object", p11_kit_uri_get_attribute (uri, CKA_LABEL))) { - free (result); - return P11_KIT_URI_NO_MEMORY; + return_val_if_reached (P11_KIT_URI_UNEXPECTED); } if (!format_attribute_class (&result, &length, &is_first, "object-type", p11_kit_uri_get_attribute (uri, CKA_CLASS))) { - free (result); - return P11_KIT_URI_NO_MEMORY; + return_val_if_reached (P11_KIT_URI_UNEXPECTED); } } if (uri->pin_source) { - format_encode_string (&result, &length, &is_first, "pin-source", - (const unsigned char*)uri->pin_source, - strlen (uri->pin_source)); + if (!format_encode_string (&result, &length, &is_first, "pin-source", + (const unsigned char*)uri->pin_source, + strlen (uri->pin_source))) { + return_val_if_reached (P11_KIT_URI_UNEXPECTED); + } } *string = result; @@ -1061,8 +1055,8 @@ parse_class_attribute (const char *name, const char *start, const char *end, return 0; value = key_decode (start, end); - if (value == NULL) - return P11_KIT_URI_NO_MEMORY; + return_val_if_fail (value != NULL, P11_KIT_URI_UNEXPECTED); + if (strcmp (value, "cert") == 0) klass = CKO_CERTIFICATE; else if (strcmp (value, "public") == 0) @@ -1084,8 +1078,7 @@ parse_class_attribute (const char *name, const char *start, const char *end, free (value); attr.pValue = malloc (sizeof (klass)); - if (attr.pValue == NULL) - return P11_KIT_URI_NO_MEMORY; + return_val_if_fail (attr.pValue != NULL, P11_KIT_URI_UNEXPECTED); memcpy (attr.pValue, &klass, sizeof (klass)); attr.ulValueLen = sizeof (klass); @@ -1336,8 +1329,7 @@ p11_kit_uri_parse (const char *string, P11KitUriType uri_type, return P11_KIT_URI_BAD_SYNTAX; key = key_decode (string, epos); - if (key == NULL) - return P11_KIT_URI_NO_MEMORY; + return_val_if_fail (key != NULL, P11_KIT_URI_UNEXPECTED); epos++; ret = 0; @@ -1407,8 +1399,8 @@ p11_kit_uri_message (int code) switch (code) { case P11_KIT_URI_OK: return NULL; - case P11_KIT_URI_NO_MEMORY: - return "Out of memory"; + case P11_KIT_URI_UNEXPECTED: + return "Unexpected or internal system error"; case P11_KIT_URI_BAD_SCHEME: return "URI scheme must be 'pkcs11:'"; case P11_KIT_URI_BAD_ENCODING: diff --git a/p11-kit/uri.h b/p11-kit/uri.h index 98a233d..7e03349 100644 --- a/p11-kit/uri.h +++ b/p11-kit/uri.h @@ -46,7 +46,7 @@ extern "C" { typedef enum { P11_KIT_URI_OK = 0, - P11_KIT_URI_NO_MEMORY = -1, + P11_KIT_URI_UNEXPECTED = -1, P11_KIT_URI_BAD_SCHEME = -2, P11_KIT_URI_BAD_ENCODING = -3, P11_KIT_URI_BAD_SYNTAX = -4, @@ -54,6 +54,8 @@ typedef enum { P11_KIT_URI_NOT_FOUND = -6, } P11KitUriResult; +#define P11_KIT_URI_NO_MEMORY P11_KIT_URI_UNEXPECTED + typedef enum { P11_KIT_URI_FOR_OBJECT = (1 << 1), P11_KIT_URI_FOR_TOKEN = (1 << 2), diff --git a/p11-kit/util.c b/p11-kit/util.c index 89ecc40..f8ec1e6 100644 --- a/p11-kit/util.c +++ b/p11-kit/util.c @@ -74,15 +74,6 @@ pthread_once_t _p11_once; static int print_messages = 1; -void* -_p11_realloc (void *memory, size_t length) -{ - void *allocated = realloc (memory, length); - if (!allocated) - free (memory); - return allocated; -} - /** * p11_kit_space_strlen: * @string: Pointer to string block diff --git a/p11-kit/util.h b/p11-kit/util.h index aa7ed19..2da4db6 100644 --- a/p11-kit/util.h +++ b/p11-kit/util.h @@ -41,8 +41,6 @@ #include -void* _p11_realloc (void *memory, size_t length); - /* ----------------------------------------------------------------------------- * WIN32 */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 28aa496..1f6083f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -4,7 +4,8 @@ INCLUDES = \ -I$(top_srcdir)/common \ -I$(top_srcdir)/p11-kit \ -I$(srcdir)/cutest \ - -DSRCDIR=\"$(srcdir)\" \ + -DSRCDIR=\"$(abs_srcdir)\" \ + -DBUILDDIR=\"$(abs_builddir)\" \ -DP11_KIT_FUTURE_UNSTABLE_API LDADD = \ diff --git a/tests/test-init.c b/tests/test-init.c index be970aa..8128db6 100644 --- a/tests/test-init.c +++ b/tests/test-init.c @@ -152,7 +152,8 @@ mock_C_Finalize__threaded_race (CK_VOID_PTR reserved) _p11_mutex_unlock (&race_mutex); _p11_sleep_ms (100); - return CKR_OK;} + return CKR_OK; +} static void * initialization_thread (void *data) @@ -223,6 +224,69 @@ test_threaded_initialization (CuTest *tc) CuAssertIntEquals (tc, 1, finalization_count); } +static CK_RV +mock_C_Initialize__test_mutexes (CK_VOID_PTR args) +{ + CK_C_INITIALIZE_ARGS_PTR init_args; + void *mutex = NULL; + CK_RV rv; + + assert (args != NULL); + init_args = args; + + rv = (init_args->CreateMutex) (&mutex); + assert (rv == CKR_OK); + + rv = (init_args->LockMutex) (mutex); + assert (rv == CKR_OK); + + rv = (init_args->UnlockMutex) (mutex); + assert (rv == CKR_OK); + + rv = (init_args->DestroyMutex) (mutex); + assert (rv == CKR_OK); + + return CKR_OK; +} + +static void +test_mutexes (CuTest *tc) +{ + CK_RV rv; + + /* Build up our own function list */ + memcpy (&module, &mock_module_no_slots, sizeof (CK_FUNCTION_LIST)); + module.C_Initialize = mock_C_Initialize__test_mutexes; + + rv = p11_kit_initialize_module (&module); + CuAssertTrue (tc, rv == CKR_OK); + + rv = p11_kit_finalize_module (&module); + CuAssertTrue (tc, rv == CKR_OK); +} + +static void +test_load_and_initialize (CuTest *tc) +{ + CK_FUNCTION_LIST_PTR module; + CK_INFO info; + CK_RV rv; + int ret; + + rv = p11_kit_load_initialize_module (BUILDDIR "/.libs/mock-one.so", &module); + CuAssertTrue (tc, rv == CKR_OK); + CuAssertTrue (tc, module != NULL); + + rv = (module->C_GetInfo) (&info); + CuAssertTrue (tc, rv == CKR_OK); + + ret = memcmp (info.manufacturerID, "MOCK MANUFACTURER ", 32); + CuAssertTrue (tc, ret == 0); + + rv = p11_kit_finalize_module (module); + CuAssertTrue (tc, ret == CKR_OK); +} + int main (void) { @@ -240,6 +304,8 @@ main (void) SUITE_ADD_TEST (suite, test_recursive_initialization); SUITE_ADD_TEST (suite, test_threaded_initialization); + SUITE_ADD_TEST (suite, test_mutexes); + SUITE_ADD_TEST (suite, test_load_and_initialize); CuSuiteRun (suite); CuSuiteSummary (suite, output); diff --git a/tests/uri-test.c b/tests/uri-test.c index 2012c0f..16480f6 100644 --- a/tests/uri-test.c +++ b/tests/uri-test.c @@ -1120,7 +1120,15 @@ test_uri_pin_source (CuTest *tc) uri = p11_kit_uri_new (); CuAssertPtrNotNull (tc, uri); - p11_kit_uri_set_pin_source (uri, "|my-pin-file"); + p11_kit_uri_set_pin_source (uri, "|my-pin-source"); + + pin_source = p11_kit_uri_get_pin_source (uri); + CuAssertStrEquals (tc, "|my-pin-source", pin_source); + + pin_source = p11_kit_uri_get_pinfile (uri); + CuAssertStrEquals (tc, "|my-pin-source", pin_source); + + p11_kit_uri_set_pinfile (uri, "|my-pin-file"); pin_source = p11_kit_uri_get_pin_source (uri); CuAssertStrEquals (tc, "|my-pin-file", pin_source); @@ -1149,7 +1157,7 @@ static void test_uri_message (CuTest *tc) { CuAssertTrue (tc, p11_kit_uri_message (P11_KIT_URI_OK) == NULL); - CuAssertPtrNotNull (tc, p11_kit_uri_message (P11_KIT_URI_NO_MEMORY)); + CuAssertPtrNotNull (tc, p11_kit_uri_message (P11_KIT_URI_UNEXPECTED)); CuAssertPtrNotNull (tc, p11_kit_uri_message (-555555)); } -- cgit v1.2.1