From 0bb1289252eec972ea26721a92adc7db47383f76 Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Fri, 24 Jan 2020 23:57:39 +0100 Subject: [PATCH 22/23] sss_ptr_hash: internal refactoring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sss_ptr_hash code was refactored: - got rid of a "spy" to make logic cleaner - table got destructor to wipe its content - described some usage limitation in the documentation And resolves: https://pagure.io/SSSD/sssd/issue/4135 Reviewed-by: Pavel Březina --- src/util/sss_ptr_hash.c | 183 +++++++++++++++++----------------------- src/util/sss_ptr_hash.h | 17 +++- 2 files changed, 91 insertions(+), 109 deletions(-) diff --git a/src/util/sss_ptr_hash.c b/src/util/sss_ptr_hash.c index 114b6edeb..6409236c7 100644 --- a/src/util/sss_ptr_hash.c +++ b/src/util/sss_ptr_hash.c @@ -39,67 +39,35 @@ static bool sss_ptr_hash_check_type(void *ptr, const char *type) return true; } +static int sss_ptr_hash_table_destructor(hash_table_t *table) +{ + sss_ptr_hash_delete_all(table, false); + return 0; +} + struct sss_ptr_hash_delete_data { hash_delete_callback *callback; void *pvt; }; struct sss_ptr_hash_value { - struct sss_ptr_hash_spy *spy; - void *ptr; -}; - -struct sss_ptr_hash_spy { - struct sss_ptr_hash_value *value; hash_table_t *table; const char *key; + void *payload; }; -static int -sss_ptr_hash_spy_destructor(struct sss_ptr_hash_spy *spy) -{ - spy->value->spy = NULL; - - /* This results in removing entry from hash table and freeing the value. */ - sss_ptr_hash_delete(spy->table, spy->key, false); - - return 0; -} - -static struct sss_ptr_hash_spy * -sss_ptr_hash_spy_create(TALLOC_CTX *mem_ctx, - hash_table_t *table, - const char *key, - struct sss_ptr_hash_value *value) -{ - struct sss_ptr_hash_spy *spy; - - spy = talloc_zero(mem_ctx, struct sss_ptr_hash_spy); - if (spy == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory!\n"); - return NULL; - } - - spy->key = talloc_strdup(spy, key); - if (spy->key == NULL) { - talloc_free(spy); - return NULL; - } - - spy->table = table; - spy->value = value; - talloc_set_destructor(spy, sss_ptr_hash_spy_destructor); - - return spy; -} - static int sss_ptr_hash_value_destructor(struct sss_ptr_hash_value *value) { - if (value->spy != NULL) { - /* Disable spy destructor and free it. */ - talloc_set_destructor(value->spy, NULL); - talloc_zfree(value->spy); + hash_key_t table_key; + + if (value->table && value->key) { + table_key.type = HASH_KEY_STRING; + table_key.str = discard_const_p(char, value->key); + if (hash_delete(value->table, &table_key) != HASH_SUCCESS) { + DEBUG(SSSDBG_CRIT_FAILURE, + "failed to delete entry with key '%s'\n", value->key); + } } return 0; @@ -112,18 +80,19 @@ sss_ptr_hash_value_create(hash_table_t *table, { struct sss_ptr_hash_value *value; - value = talloc_zero(table, struct sss_ptr_hash_value); + value = talloc_zero(talloc_ptr, struct sss_ptr_hash_value); if (value == NULL) { return NULL; } - value->spy = sss_ptr_hash_spy_create(talloc_ptr, table, key, value); - if (value->spy == NULL) { + value->key = talloc_strdup(value, key); + if (value->key == NULL) { talloc_free(value); return NULL; } - value->ptr = talloc_ptr; + value->table = table; + value->payload = talloc_ptr; talloc_set_destructor(value, sss_ptr_hash_value_destructor); return value; @@ -138,29 +107,31 @@ sss_ptr_hash_delete_cb(hash_entry_t *item, struct sss_ptr_hash_value *value; struct hash_entry_t callback_entry; + if (pvt == NULL) { + return; + } + value = talloc_get_type(item->value.ptr, struct sss_ptr_hash_value); if (value == NULL) { DEBUG(SSSDBG_CRIT_FAILURE, "Invalid value!\n"); return; } + /* Switch to the input value and call custom callback. */ + data = talloc_get_type(pvt, struct sss_ptr_hash_delete_data); + if (data == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "Invalid data!\n"); + return; + } + callback_entry.key = item->key; callback_entry.value.type = HASH_VALUE_PTR; - callback_entry.value.ptr = value->ptr; - - /* Free value, this also will disable spy */ - talloc_free(value); - - if (pvt != NULL) { - /* Switch to the input value and call custom callback. */ - data = talloc_get_type(pvt, struct sss_ptr_hash_delete_data); - if (data == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "Invalid data!\n"); - return; - } - - data->callback(&callback_entry, deltype, data->pvt); - } + callback_entry.value.ptr = value->payload; + /* Even if execution is already in the context of + * talloc_free(payload) -> talloc_free(value) -> ... + * there still might be legitimate reasons to execute callback. + */ + data->callback(&callback_entry, deltype, data->pvt); } hash_table_t *sss_ptr_hash_create(TALLOC_CTX *mem_ctx, @@ -194,6 +165,8 @@ hash_table_t *sss_ptr_hash_create(TALLOC_CTX *mem_ctx, talloc_steal(table, data); } + talloc_set_destructor(table, sss_ptr_hash_table_destructor); + return table; } @@ -282,15 +255,15 @@ void *_sss_ptr_hash_lookup(hash_table_t *table, struct sss_ptr_hash_value *value; value = sss_ptr_hash_lookup_internal(table, key); - if (value == NULL || value->ptr == NULL) { + if (value == NULL || value->payload == NULL) { return NULL; } - if (!sss_ptr_hash_check_type(value->ptr, type)) { + if (!sss_ptr_hash_check_type(value->payload, type)) { return NULL; } - return value->ptr; + return value->payload; } void *_sss_ptr_get_value(hash_value_t *table_value, @@ -311,11 +284,11 @@ void *_sss_ptr_get_value(hash_value_t *table_value, value = table_value->ptr; - if (!sss_ptr_hash_check_type(value->ptr, type)) { + if (!sss_ptr_hash_check_type(value->payload, type)) { return NULL; } - return value->ptr; + return value->payload; } void sss_ptr_hash_delete(hash_table_t *table, @@ -323,74 +296,70 @@ void sss_ptr_hash_delete(hash_table_t *table, bool free_value) { struct sss_ptr_hash_value *value; - hash_key_t table_key; - int hret; - void *payload; + void *payload = NULL; if (table == NULL || key == NULL) { return; } - if (free_value) { - value = sss_ptr_hash_lookup_internal(table, key); - if (value == NULL) { - free_value = false; - } else { - payload = value->ptr; - } - } - - table_key.type = HASH_KEY_STRING; - table_key.str = discard_const_p(char, key); - - /* Delete table entry. This will free value and spy in delete callback. */ - hret = hash_delete(table, &table_key); - if (hret != HASH_SUCCESS && hret != HASH_ERROR_KEY_NOT_FOUND) { - DEBUG(SSSDBG_CRIT_FAILURE, "Unable to remove key from table [%d]\n", - hret); + value = sss_ptr_hash_lookup_internal(table, key); + if (value == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, + "Unable to remove key '%s' from table\n", key); + return; } - /* Also free the original value if requested. */ if (free_value) { - talloc_free(payload); + payload = value->payload; } + talloc_free(value); /* this will call hash_delete() in value d-tor */ + + talloc_free(payload); /* it is safe to call talloc_free(NULL) */ + return; } void sss_ptr_hash_delete_all(hash_table_t *table, bool free_values) { + hash_value_t *content; struct sss_ptr_hash_value *value; - hash_value_t *values; + void *payload = NULL; unsigned long count; unsigned long i; int hret; - void *ptr; if (table == NULL) { return; } - hret = hash_values(table, &count, &values); + hret = hash_values(table, &count, &content); if (hret != HASH_SUCCESS) { DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get values [%d]\n", hret); return; } - for (i = 0; i < count; i++) { - value = values[i].ptr; - ptr = value->ptr; - - /* This will remove the entry from hash table and free value. */ - talloc_free(value->spy); - - if (free_values) { - /* Also free the original value. */ - talloc_free(ptr); + for (i = 0; i < count; ++i) { + if ((content[i].type == HASH_VALUE_PTR) && + sss_ptr_hash_check_type(content[i].ptr, + "struct sss_ptr_hash_value")) { + value = content[i].ptr; + if (free_values) { + payload = value->payload; + } + talloc_free(value); + if (free_values) { + talloc_free(payload); /* it's safe to call talloc_free(NULL) */ + } + } else { + DEBUG(SSSDBG_CRIT_FAILURE, + "Unexpected type of table content, skipping"); } } + talloc_free(content); + return; } diff --git a/src/util/sss_ptr_hash.h b/src/util/sss_ptr_hash.h index 56bb19a65..0889b171a 100644 --- a/src/util/sss_ptr_hash.h +++ b/src/util/sss_ptr_hash.h @@ -28,7 +28,19 @@ /** * Create a new hash table with string key and talloc pointer value with - * possible delete callback. + * possible custom delete callback @del_cb. + * Table will have destructor setup to wipe content. + * Never call hash_destroy(table) and hash_delete() explicitly but rather + * use talloc_free(table) and sss_ptr_hash_delete(). + * + * A notes about @del_cb: + * - this callback must never modify hash table (i.e. add/del entries); + * - this callback is triggered when value is either explicitly removed + * from the table or simply freed (latter leads to removal of an entry + * from the table); + * - this callback is also triggered for every entry when table is freed + * entirely. In this case (deltype == HASH_TABLE_DESTROY) any table + * lookups / iteration are forbidden as table might be already invalidated. */ hash_table_t *sss_ptr_hash_create(TALLOC_CTX *mem_ctx, hash_delete_callback *del_cb, @@ -41,7 +53,8 @@ hash_table_t *sss_ptr_hash_create(TALLOC_CTX *mem_ctx, * the value is overridden. Otherwise EEXIST error is returned. * * If talloc_ptr is freed the key and value are automatically - * removed from the hash table. + * removed from the hash table (del_cb that was set up during + * table creation is executed as a first step of this removal). * * @return EOK If the <@key, @talloc_ptr> pair was inserted. * @return EEXIST If @key already exists and @override is false. -- 2.20.1