sssd/SOURCES/0022-sss_ptr_hash-internal-refactoring.patch

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