166 lines
6.2 KiB
Diff
166 lines
6.2 KiB
Diff
From f1db05d8839b39fd48471dcb29881c12ed27a434 Mon Sep 17 00:00:00 2001
|
|
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina@redhat.com>
|
|
Date: Thu, 29 Oct 2020 14:57:53 +0100
|
|
Subject: [PATCH 14/19] sss_ptr_hash: fix double free for circular dependencies
|
|
|
|
If the hash table delete callback deletes the stored item,
|
|
we can end up in double free in case when we try to override
|
|
an existing item (hash_enter(key) where key already exists).
|
|
|
|
```c
|
|
static void delete_cb(hash_entry_t *item,
|
|
hash_destroy_enum deltype,
|
|
void *pvt)
|
|
{
|
|
talloc_free(item->value.ptr);
|
|
}
|
|
|
|
hash_enter(key);
|
|
hash_enter(key);
|
|
```
|
|
|
|
The doble free it self is fine, since it is done via talloc destructor
|
|
and talloc can cope with that. However, the hash table fails to store
|
|
the new entry because hash_delete is called twice.
|
|
|
|
```
|
|
_sss_ptr_hash_add -> hash_enter -> hash_delete(old) -> delete_cb -> sss_ptr_hash_value_destructor -> hash_delete
|
|
```
|
|
---
|
|
src/tests/cmocka/test_sss_ptr_hash.c | 39 ++++++++++++++++++++++++++++
|
|
src/tests/cmocka/test_utils.c | 3 +++
|
|
src/tests/cmocka/test_utils.h | 1 +
|
|
src/util/sss_ptr_hash.c | 20 ++++++++++++++
|
|
4 files changed, 63 insertions(+)
|
|
|
|
diff --git a/src/tests/cmocka/test_sss_ptr_hash.c b/src/tests/cmocka/test_sss_ptr_hash.c
|
|
index 1458238f537970d0ecde80bd36830b28970ca364..31cf8b705367498822094f8811b393c1b35e12bc 100644
|
|
--- a/src/tests/cmocka/test_sss_ptr_hash.c
|
|
+++ b/src/tests/cmocka/test_sss_ptr_hash.c
|
|
@@ -91,6 +91,45 @@ void test_sss_ptr_hash_with_free_cb(void **state)
|
|
assert_int_equal(free_counter, MAX_ENTRIES_AMOUNT*2);
|
|
}
|
|
|
|
+void test_sss_ptr_hash_overwrite_with_free_cb(void **state)
|
|
+{
|
|
+ hash_table_t *table;
|
|
+ int free_counter = 0;
|
|
+ unsigned long count;
|
|
+ char *payload;
|
|
+ char *value;
|
|
+ errno_t ret;
|
|
+
|
|
+ table = sss_ptr_hash_create(global_talloc_context,
|
|
+ free_payload_cb,
|
|
+ &free_counter);
|
|
+ assert_non_null(table);
|
|
+
|
|
+ payload = talloc_strdup(table, "test_value1");
|
|
+ assert_non_null(payload);
|
|
+ talloc_set_name_const(payload, "char");
|
|
+ ret = sss_ptr_hash_add_or_override(table, "test", payload, char);
|
|
+ assert_int_equal(ret, 0);
|
|
+ count = hash_count(table);
|
|
+ assert_int_equal(count, 1);
|
|
+ value = sss_ptr_hash_lookup(table, "test", char);
|
|
+ assert_ptr_equal(value, payload);
|
|
+
|
|
+
|
|
+ payload = talloc_strdup(table, "test_value2");
|
|
+ assert_non_null(payload);
|
|
+ talloc_set_name_const(payload, "char");
|
|
+ ret = sss_ptr_hash_add_or_override(table, "test", payload, char);
|
|
+ assert_int_equal(ret, 0);
|
|
+ count = hash_count(table);
|
|
+ assert_int_equal(count, 1);
|
|
+ value = sss_ptr_hash_lookup(table, "test", char);
|
|
+ assert_ptr_equal(value, payload);
|
|
+
|
|
+ talloc_free(table);
|
|
+ assert_int_equal(free_counter, 2);
|
|
+}
|
|
+
|
|
struct table_wrapper
|
|
{
|
|
hash_table_t **table;
|
|
diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c
|
|
index d77a972c1bc93638085c3d49131247fefb333d56..d258622fb50e849a3efabb123960db410eb399e1 100644
|
|
--- a/src/tests/cmocka/test_utils.c
|
|
+++ b/src/tests/cmocka/test_utils.c
|
|
@@ -2144,6 +2144,9 @@ int main(int argc, const char *argv[])
|
|
cmocka_unit_test_setup_teardown(test_sss_ptr_hash_with_free_cb,
|
|
setup_leak_tests,
|
|
teardown_leak_tests),
|
|
+ cmocka_unit_test_setup_teardown(test_sss_ptr_hash_overwrite_with_free_cb,
|
|
+ setup_leak_tests,
|
|
+ teardown_leak_tests),
|
|
cmocka_unit_test_setup_teardown(test_sss_ptr_hash_with_lookup_cb,
|
|
setup_leak_tests,
|
|
teardown_leak_tests),
|
|
diff --git a/src/tests/cmocka/test_utils.h b/src/tests/cmocka/test_utils.h
|
|
index 44b9479f965ee830ea0937c0fd89b87e35796598..458bcb750569c1f5f346917f29aa8b5500891988 100644
|
|
--- a/src/tests/cmocka/test_utils.h
|
|
+++ b/src/tests/cmocka/test_utils.h
|
|
@@ -35,6 +35,7 @@ void test_concatenate_string_array(void **state);
|
|
|
|
/* from src/tests/cmocka/test_sss_ptr_hash.c */
|
|
void test_sss_ptr_hash_with_free_cb(void **state);
|
|
+void test_sss_ptr_hash_overwrite_with_free_cb(void **state);
|
|
void test_sss_ptr_hash_with_lookup_cb(void **state);
|
|
void test_sss_ptr_hash_without_cb(void **state);
|
|
|
|
diff --git a/src/util/sss_ptr_hash.c b/src/util/sss_ptr_hash.c
|
|
index 6409236c782bac729ec51502019c04c83bce7cab..e3805dac4052b587d395b7163f5c45e1ba0aa6dc 100644
|
|
--- a/src/util/sss_ptr_hash.c
|
|
+++ b/src/util/sss_ptr_hash.c
|
|
@@ -54,6 +54,7 @@ struct sss_ptr_hash_value {
|
|
hash_table_t *table;
|
|
const char *key;
|
|
void *payload;
|
|
+ bool delete_in_progress;
|
|
};
|
|
|
|
static int
|
|
@@ -61,12 +62,22 @@ sss_ptr_hash_value_destructor(struct sss_ptr_hash_value *value)
|
|
{
|
|
hash_key_t table_key;
|
|
|
|
+ /* Do not call hash_delete() if we got here from hash delete callback when
|
|
+ * the callback calls talloc_free(payload) which frees the value. This
|
|
+ * should not happen since talloc will avoid circular free but let's be
|
|
+ * over protective here. */
|
|
+ if (value->delete_in_progress) {
|
|
+ return 0;
|
|
+ }
|
|
+
|
|
+ value->delete_in_progress = true;
|
|
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);
|
|
+ value->delete_in_progress = false;
|
|
}
|
|
}
|
|
|
|
@@ -127,6 +138,15 @@ sss_ptr_hash_delete_cb(hash_entry_t *item,
|
|
callback_entry.key = item->key;
|
|
callback_entry.value.type = HASH_VALUE_PTR;
|
|
callback_entry.value.ptr = value->payload;
|
|
+
|
|
+ /* Delete the value in case this callback has been called directly
|
|
+ * from dhash (overwriting existing entry) instead of hash_delete()
|
|
+ * in value's destructor. */
|
|
+ if (!value->delete_in_progress) {
|
|
+ talloc_set_destructor(value, NULL);
|
|
+ talloc_free(value);
|
|
+ }
|
|
+
|
|
/* Even if execution is already in the context of
|
|
* talloc_free(payload) -> talloc_free(value) -> ...
|
|
* there still might be legitimate reasons to execute callback.
|
|
--
|
|
2.25.4
|
|
|