From 0bb1289252eec972ea26721a92adc7db47383f76 Mon Sep 17 00:00:00 2001

From: Alexey Tikhonov <atikhono@redhat.com>

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 <pbrezina@redhat.com>

---

 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