From 3e2905a17655e15f4cc6047d848b81a27dec204a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Thu, 27 Feb 2020 04:07:30 +0100 Subject: [PATCH] Resolves: upstream#4135 - util/sss_ptr_hash.c: potential double free in `sss_ptr_hash_delete_cb()` --- 0018-sbus_server-stylistic-rename.patch | 43 ++ ...-t-keep-empty-sss_ptr_hash_delete_da.patch | 91 +++++ ...sss_ptr_hash_delete-fix-optimization.patch | 62 +++ ...sss_ptr_hash-removed-redundant-check.patch | 35 ++ 0022-sss_ptr_hash-fixed-memory-leak.patch | 53 +++ 0023-sss_ptr_hash-internal-refactoring.patch | 366 ++++++++++++++++++ 0024-TESTS-added-sss_ptr_hash-unit-test.patch | 266 +++++++++++++ sssd.spec | 11 + 8 files changed, 927 insertions(+) create mode 100644 0018-sbus_server-stylistic-rename.patch create mode 100644 0019-sss_ptr_hash-don-t-keep-empty-sss_ptr_hash_delete_da.patch create mode 100644 0020-sss_ptr_hash-sss_ptr_hash_delete-fix-optimization.patch create mode 100644 0021-sss_ptr_hash-removed-redundant-check.patch create mode 100644 0022-sss_ptr_hash-fixed-memory-leak.patch create mode 100644 0023-sss_ptr_hash-internal-refactoring.patch create mode 100644 0024-TESTS-added-sss_ptr_hash-unit-test.patch diff --git a/0018-sbus_server-stylistic-rename.patch b/0018-sbus_server-stylistic-rename.patch new file mode 100644 index 0000000..0d96af5 --- /dev/null +++ b/0018-sbus_server-stylistic-rename.patch @@ -0,0 +1,43 @@ +From faa5dbf6f716bd4ac0a3020a28a1ee6fbf74654a Mon Sep 17 00:00:00 2001 +From: Alexey Tikhonov +Date: Thu, 23 Jan 2020 17:22:28 +0100 +Subject: [PATCH 18/24] sbus_server: stylistic rename +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Renamed sbus_server_name_remove_from_table() to +sbus_server_name_remove_from_table_cb() to keep naming consistent +with other functions used as `hash_delete_callback` argument of +sss_ptr_hash_create() + +Reviewed-by: Pavel Březina +--- + src/sbus/server/sbus_server.c | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/sbus/server/sbus_server.c b/src/sbus/server/sbus_server.c +index 5405dae56..2b9327051 100644 +--- a/src/sbus/server/sbus_server.c ++++ b/src/sbus/server/sbus_server.c +@@ -584,7 +584,7 @@ sbus_server_name_lost(struct sbus_server *server, + } + + static void +-sbus_server_name_remove_from_table(hash_entry_t *item, ++sbus_server_name_remove_from_table_cb(hash_entry_t *item, + hash_destroy_enum type, + void *pvt) + { +@@ -676,7 +676,7 @@ sbus_server_create(TALLOC_CTX *mem_ctx, + } + + sbus_server->names = sss_ptr_hash_create(sbus_server, +- sbus_server_name_remove_from_table, sbus_server); ++ sbus_server_name_remove_from_table_cb, sbus_server); + if (sbus_server->names == NULL) { + ret = ENOMEM; + goto done; +-- +2.20.1 + diff --git a/0019-sss_ptr_hash-don-t-keep-empty-sss_ptr_hash_delete_da.patch b/0019-sss_ptr_hash-don-t-keep-empty-sss_ptr_hash_delete_da.patch new file mode 100644 index 0000000..2b4cae9 --- /dev/null +++ b/0019-sss_ptr_hash-don-t-keep-empty-sss_ptr_hash_delete_da.patch @@ -0,0 +1,91 @@ +From adc7730a4e1b9721c93863a1b283457e9c02a3c5 Mon Sep 17 00:00:00 2001 +From: Alexey Tikhonov +Date: Thu, 23 Jan 2020 17:55:24 +0100 +Subject: [PATCH 19/24] sss_ptr_hash: don't keep empty sss_ptr_hash_delete_data +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +There is no need to allocate memory for `sss_ptr_hash_delete_data` +if table user doesn't provide custom delete callback. + +Reviewed-by: Pavel Březina +--- + src/util/sss_ptr_hash.c | 36 ++++++++++++++++++++---------------- + 1 file changed, 20 insertions(+), 16 deletions(-) + +diff --git a/src/util/sss_ptr_hash.c b/src/util/sss_ptr_hash.c +index 8f9762cb9..f8addec1e 100644 +--- a/src/util/sss_ptr_hash.c ++++ b/src/util/sss_ptr_hash.c +@@ -138,12 +138,6 @@ sss_ptr_hash_delete_cb(hash_entry_t *item, + struct sss_ptr_hash_value *value; + struct hash_entry_t callback_entry; + +- data = talloc_get_type(pvt, struct sss_ptr_hash_delete_data); +- if (data == NULL) { +- DEBUG(SSSDBG_CRIT_FAILURE, "Invalid data!\n"); +- return; +- } +- + value = talloc_get_type(item->value.ptr, struct sss_ptr_hash_value); + if (value == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "Invalid value!\n"); +@@ -157,8 +151,14 @@ sss_ptr_hash_delete_cb(hash_entry_t *item, + /* Free value, this also will disable spy */ + talloc_free(value); + +- /* Switch to the input value and call custom callback. */ +- if (data->callback != NULL) { ++ 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); + } + } +@@ -167,17 +167,19 @@ hash_table_t *sss_ptr_hash_create(TALLOC_CTX *mem_ctx, + hash_delete_callback *del_cb, + void *del_cb_pvt) + { +- struct sss_ptr_hash_delete_data *data; ++ struct sss_ptr_hash_delete_data *data = NULL; + hash_table_t *table; + errno_t ret; + +- data = talloc_zero(NULL, struct sss_ptr_hash_delete_data); +- if (data == NULL) { +- return NULL; +- } ++ if (del_cb != NULL) { ++ data = talloc_zero(NULL, struct sss_ptr_hash_delete_data); ++ if (data == NULL) { ++ return NULL; ++ } + +- data->callback = del_cb; +- data->pvt = del_cb_pvt; ++ data->callback = del_cb; ++ data->pvt = del_cb_pvt; ++ } + + ret = sss_hash_create_ex(mem_ctx, 10, &table, 0, 0, 0, 0, + sss_ptr_hash_delete_cb, data); +@@ -188,7 +190,9 @@ hash_table_t *sss_ptr_hash_create(TALLOC_CTX *mem_ctx, + return NULL; + } + +- talloc_steal(table, data); ++ if (data != NULL) { ++ talloc_steal(table, data); ++ } + + return table; + } +-- +2.20.1 + diff --git a/0020-sss_ptr_hash-sss_ptr_hash_delete-fix-optimization.patch b/0020-sss_ptr_hash-sss_ptr_hash_delete-fix-optimization.patch new file mode 100644 index 0000000..8ba4b16 --- /dev/null +++ b/0020-sss_ptr_hash-sss_ptr_hash_delete-fix-optimization.patch @@ -0,0 +1,62 @@ +From d0eb88089b059bfe2da3bd1a3797b89d69119c29 Mon Sep 17 00:00:00 2001 +From: Alexey Tikhonov +Date: Thu, 23 Jan 2020 19:00:27 +0100 +Subject: [PATCH 20/24] sss_ptr_hash: sss_ptr_hash_delete fix/optimization +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + + - no reason to skip hash_delete() just because sss_ptr_hash_lookup_internal() +failed + - avoid excessive lookup if it is not required to free payload + +Reviewed-by: Pavel Březina +--- + src/util/sss_ptr_hash.c | 17 +++++++++-------- + 1 file changed, 9 insertions(+), 8 deletions(-) + +diff --git a/src/util/sss_ptr_hash.c b/src/util/sss_ptr_hash.c +index f8addec1e..7326244e6 100644 +--- a/src/util/sss_ptr_hash.c ++++ b/src/util/sss_ptr_hash.c +@@ -331,20 +331,21 @@ void sss_ptr_hash_delete(hash_table_t *table, + struct sss_ptr_hash_value *value; + hash_key_t table_key; + int hret; +- void *ptr; ++ void *payload; + + if (table == NULL || key == NULL) { + return; + } + +- value = sss_ptr_hash_lookup_internal(table, key); +- if (value == NULL) { +- /* Value not found. */ +- return; ++ if (free_value) { ++ value = sss_ptr_hash_lookup_internal(table, key); ++ if (value == NULL) { ++ free_value = false; ++ } else { ++ payload = value->ptr; ++ } + } + +- ptr = value->ptr; +- + table_key.type = HASH_KEY_STRING; + table_key.str = discard_const_p(char, key); + +@@ -357,7 +358,7 @@ void sss_ptr_hash_delete(hash_table_t *table, + + /* Also free the original value if requested. */ + if (free_value) { +- talloc_free(ptr); ++ talloc_free(payload); + } + + return; +-- +2.20.1 + diff --git a/0021-sss_ptr_hash-removed-redundant-check.patch b/0021-sss_ptr_hash-removed-redundant-check.patch new file mode 100644 index 0000000..ac841f0 --- /dev/null +++ b/0021-sss_ptr_hash-removed-redundant-check.patch @@ -0,0 +1,35 @@ +From 8cc2ce4e9060a71d441a377008fb2f567baa5d92 Mon Sep 17 00:00:00 2001 +From: Alexey Tikhonov +Date: Thu, 23 Jan 2020 20:07:41 +0100 +Subject: [PATCH 21/24] sss_ptr_hash: removed redundant check +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +`sss_ptr_hash_check_type()` call would take care of this case. + +Reviewed-by: Pavel Březina +--- + src/util/sss_ptr_hash.c | 6 ------ + 1 file changed, 6 deletions(-) + +diff --git a/src/util/sss_ptr_hash.c b/src/util/sss_ptr_hash.c +index 7326244e6..bf111a613 100644 +--- a/src/util/sss_ptr_hash.c ++++ b/src/util/sss_ptr_hash.c +@@ -268,12 +268,6 @@ sss_ptr_hash_lookup_internal(hash_table_t *table, + return NULL; + } + +- /* This may happen if we are in delete callback +- * and we try to search the hash table. */ +- if (table_value.ptr == NULL) { +- return NULL; +- } +- + if (!sss_ptr_hash_check_type(table_value.ptr, "struct sss_ptr_hash_value")) { + return NULL; + } +-- +2.20.1 + diff --git a/0022-sss_ptr_hash-fixed-memory-leak.patch b/0022-sss_ptr_hash-fixed-memory-leak.patch new file mode 100644 index 0000000..04f8da3 --- /dev/null +++ b/0022-sss_ptr_hash-fixed-memory-leak.patch @@ -0,0 +1,53 @@ +From 4bc0c2c7833dd643fc1137daf6519670c05c3736 Mon Sep 17 00:00:00 2001 +From: Alexey Tikhonov +Date: Thu, 23 Jan 2020 21:11:16 +0100 +Subject: [PATCH 22/24] sss_ptr_hash: fixed memory leak +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +In case `override` check was failed in _sss_ptr_hash_add() +`value` was leaking. +Fixed to do `override` check before value allocation. + +Reviewed-by: Pavel Březina +--- + src/util/sss_ptr_hash.c | 14 +++++++------- + 1 file changed, 7 insertions(+), 7 deletions(-) + +diff --git a/src/util/sss_ptr_hash.c b/src/util/sss_ptr_hash.c +index bf111a613..114b6edeb 100644 +--- a/src/util/sss_ptr_hash.c ++++ b/src/util/sss_ptr_hash.c +@@ -217,21 +217,21 @@ errno_t _sss_ptr_hash_add(hash_table_t *table, + return ERR_INVALID_DATA_TYPE; + } + ++ table_key.type = HASH_KEY_STRING; ++ table_key.str = discard_const_p(char, key); ++ ++ if (override == false && hash_has_key(table, &table_key)) { ++ return EEXIST; ++ } ++ + value = sss_ptr_hash_value_create(table, key, talloc_ptr); + if (value == NULL) { + return ENOMEM; + } + +- table_key.type = HASH_KEY_STRING; +- table_key.str = discard_const_p(char, key); +- + table_value.type = HASH_VALUE_PTR; + table_value.ptr = value; + +- if (override == false && hash_has_key(table, &table_key)) { +- return EEXIST; +- } +- + hret = hash_enter(table, &table_key, &table_value); + if (hret != HASH_SUCCESS) { + DEBUG(SSSDBG_CRIT_FAILURE, "Unable to add key %s!\n", key); +-- +2.20.1 + diff --git a/0023-sss_ptr_hash-internal-refactoring.patch b/0023-sss_ptr_hash-internal-refactoring.patch new file mode 100644 index 0000000..25a58ac --- /dev/null +++ b/0023-sss_ptr_hash-internal-refactoring.patch @@ -0,0 +1,366 @@ +From 0bb1289252eec972ea26721a92adc7db47383f76 Mon Sep 17 00:00:00 2001 +From: Alexey Tikhonov +Date: Fri, 24 Jan 2020 23:57:39 +0100 +Subject: [PATCH 23/24] 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 + diff --git a/0024-TESTS-added-sss_ptr_hash-unit-test.patch b/0024-TESTS-added-sss_ptr_hash-unit-test.patch new file mode 100644 index 0000000..0fd4f63 --- /dev/null +++ b/0024-TESTS-added-sss_ptr_hash-unit-test.patch @@ -0,0 +1,266 @@ +From 88b23bf50dd1c12413f3314639de2c3909bd9098 Mon Sep 17 00:00:00 2001 +From: Alexey Tikhonov +Date: Tue, 28 Jan 2020 19:26:08 +0100 +Subject: [PATCH 24/24] TESTS: added sss_ptr_hash unit test +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Reviewed-by: Pavel Březina +--- + Makefile.am | 1 + + src/tests/cmocka/test_sss_ptr_hash.c | 193 +++++++++++++++++++++++++++ + src/tests/cmocka/test_utils.c | 9 ++ + src/tests/cmocka/test_utils.h | 6 + + 4 files changed, 209 insertions(+) + create mode 100644 src/tests/cmocka/test_sss_ptr_hash.c + +diff --git a/Makefile.am b/Makefile.am +index 57ba51356..c991f2aa0 100644 +--- a/Makefile.am ++++ b/Makefile.am +@@ -3054,6 +3054,7 @@ test_ipa_idmap_LDADD = \ + test_utils_SOURCES = \ + src/tests/cmocka/test_utils.c \ + src/tests/cmocka/test_string_utils.c \ ++ src/tests/cmocka/test_sss_ptr_hash.c \ + src/p11_child/p11_child_common_utils.c \ + $(NULL) + if BUILD_SSH +diff --git a/src/tests/cmocka/test_sss_ptr_hash.c b/src/tests/cmocka/test_sss_ptr_hash.c +new file mode 100644 +index 000000000..1458238f5 +--- /dev/null ++++ b/src/tests/cmocka/test_sss_ptr_hash.c +@@ -0,0 +1,193 @@ ++/* ++ Copyright (C) 2020 Red Hat ++ ++ This program is free software; you can redistribute it and/or modify ++ it under the terms of the GNU General Public License as published by ++ the Free Software Foundation; either version 3 of the License, or ++ (at your option) any later version. ++ ++ This program is distributed in the hope that it will be useful, ++ but WITHOUT ANY WARRANTY; without even the implied warranty of ++ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the ++ GNU General Public License for more details. ++ ++ You should have received a copy of the GNU General Public License ++ along with this program. If not, see . ++*/ ++ ++#include "tests/cmocka/common_mock.h" ++#include "util/sss_ptr_hash.h" ++ ++static const int MAX_ENTRIES_AMOUNT = 5; ++ ++static void populate_table(hash_table_t *table, int **payloads) ++{ ++ char key[2] = {'z', 0}; ++ ++ for (int i = 0; i < MAX_ENTRIES_AMOUNT; ++i) { ++ payloads[i] = talloc_zero(global_talloc_context, int); ++ assert_non_null(payloads[i]); ++ *payloads[i] = i; ++ key[0] = '0'+(char)i; ++ assert_int_equal(sss_ptr_hash_add(table, key, payloads[i], int), 0); ++ } ++ ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT); ++} ++ ++static void free_payload_cb(hash_entry_t *item, hash_destroy_enum type, void *pvt) ++{ ++ int *counter; ++ ++ assert_non_null(item); ++ assert_non_null(item->value.ptr); ++ talloc_zfree(item->value.ptr); ++ ++ assert_non_null(pvt); ++ counter = (int *)pvt; ++ (*counter)++; ++} ++ ++void test_sss_ptr_hash_with_free_cb(void **state) ++{ ++ hash_table_t *table; ++ int free_counter = 0; ++ int *payloads[MAX_ENTRIES_AMOUNT]; ++ ++ table = sss_ptr_hash_create(global_talloc_context, ++ free_payload_cb, ++ &free_counter); ++ assert_non_null(table); ++ ++ populate_table(table, payloads); ++ ++ /* check explicit removal from the hash */ ++ sss_ptr_hash_delete(table, "1", false); ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT-1); ++ assert_int_equal(free_counter, 1); ++ ++ /* check implicit removal triggered by payload deletion */ ++ talloc_free(payloads[3]); ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT-2); ++ assert_int_equal(free_counter, 2); ++ ++ /* try to remove non existent entry */ ++ sss_ptr_hash_delete(table, "q", false); ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT-2); ++ assert_int_equal(free_counter, 2); ++ ++ /* clear all */ ++ sss_ptr_hash_delete_all(table, false); ++ assert_int_equal((int)hash_count(table), 0); ++ assert_int_equal(free_counter, MAX_ENTRIES_AMOUNT); ++ ++ /* check that table is still operable */ ++ populate_table(table, payloads); ++ sss_ptr_hash_delete(table, "2", false); ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT-1); ++ assert_int_equal(free_counter, MAX_ENTRIES_AMOUNT+1); ++ ++ talloc_free(table); ++ assert_int_equal(free_counter, MAX_ENTRIES_AMOUNT*2); ++} ++ ++struct table_wrapper ++{ ++ hash_table_t **table; ++}; ++ ++static void lookup_cb(hash_entry_t *item, hash_destroy_enum type, void *pvt) ++{ ++ hash_table_t *table; ++ hash_key_t *keys; ++ unsigned long count; ++ int *value = NULL; ++ int sum = 0; ++ ++ assert_non_null(pvt); ++ table = *((struct table_wrapper *)pvt)->table; ++ assert_non_null(table); ++ ++ if (type == HASH_TABLE_DESTROY) { ++ /* table is being destroyed */ ++ return; ++ } ++ ++ assert_int_equal(hash_keys(table, &count, &keys), HASH_SUCCESS); ++ for (unsigned int i = 0; i < count; ++i) { ++ assert_int_equal(keys[i].type, HASH_KEY_STRING); ++ value = sss_ptr_hash_lookup(table, keys[i].c_str, int); ++ assert_non_null(value); ++ sum += *value; ++ } ++ DEBUG(SSSDBG_TRACE_ALL, "sum of all values = %d\n", sum); ++ talloc_free(keys); ++} ++ ++/* main difference with `test_sss_ptr_hash_with_free_cb()` ++ * is that table cb here doesn't delete payload so ++ * this is requested via `free_value(s)` arg ++ */ ++void test_sss_ptr_hash_with_lookup_cb(void **state) ++{ ++ hash_table_t *table; ++ struct table_wrapper wrapper; ++ int *payloads[MAX_ENTRIES_AMOUNT]; ++ ++ wrapper.table = &table; ++ table = sss_ptr_hash_create(global_talloc_context, ++ lookup_cb, ++ &wrapper); ++ assert_non_null(table); ++ ++ populate_table(table, payloads); ++ ++ /* check explicit removal from the hash */ ++ sss_ptr_hash_delete(table, "2", true); ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT-1); ++ ++ /* check implicit removal triggered by payload deletion */ ++ talloc_free(payloads[0]); ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT-2); ++ ++ /* clear all */ ++ sss_ptr_hash_delete_all(table, true); ++ assert_int_equal((int)hash_count(table), 0); ++ /* teardown function shall verify there are no leaks ++ * on global_talloc_context and so that payloads[] were freed ++ */ ++ ++ /* check that table is still operable */ ++ populate_table(table, payloads); ++ ++ talloc_free(table); ++ /* d-tor triggers hash_destroy() but since cb here doesn free payload ++ * this should be done manually ++ */ ++ for (int i = 0; i < MAX_ENTRIES_AMOUNT; ++i) { ++ talloc_free(payloads[i]); ++ } ++} ++ ++/* Just smoke test to verify that absence of cb doesn't break anything */ ++void test_sss_ptr_hash_without_cb(void **state) ++{ ++ hash_table_t *table; ++ int *payloads[MAX_ENTRIES_AMOUNT]; ++ ++ table = sss_ptr_hash_create(global_talloc_context, NULL, NULL); ++ assert_non_null(table); ++ ++ populate_table(table, payloads); ++ ++ sss_ptr_hash_delete(table, "4", true); ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT-1); ++ ++ talloc_free(payloads[1]); ++ assert_int_equal((int)hash_count(table), MAX_ENTRIES_AMOUNT-2); ++ ++ sss_ptr_hash_delete_all(table, true); ++ assert_int_equal((int)hash_count(table), 0); ++ ++ talloc_free(table); ++} +diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c +index 666f32903..c5eda4dd2 100644 +--- a/src/tests/cmocka/test_utils.c ++++ b/src/tests/cmocka/test_utils.c +@@ -2055,6 +2055,15 @@ int main(int argc, const char *argv[]) + cmocka_unit_test_setup_teardown(test_sss_get_domain_mappings_content, + setup_dom_list_with_subdomains, + teardown_dom_list), ++ 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_with_lookup_cb, ++ setup_leak_tests, ++ teardown_leak_tests), ++ cmocka_unit_test_setup_teardown(test_sss_ptr_hash_without_cb, ++ setup_leak_tests, ++ teardown_leak_tests), + }; + + /* Set debug level to invalid value so we can decide if -d 0 was used. */ +diff --git a/src/tests/cmocka/test_utils.h b/src/tests/cmocka/test_utils.h +index e93e0da25..44b9479f9 100644 +--- a/src/tests/cmocka/test_utils.h ++++ b/src/tests/cmocka/test_utils.h +@@ -33,4 +33,10 @@ void test_guid_blob_to_string_buf(void **state); + void test_get_last_x_chars(void **state); + 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_with_lookup_cb(void **state); ++void test_sss_ptr_hash_without_cb(void **state); ++ ++ + #endif /* __TESTS__CMOCKA__TEST_UTILS_H__ */ +-- +2.20.1 + diff --git a/sssd.spec b/sssd.spec index f3d80c1..731ab91 100644 --- a/sssd.spec +++ b/sssd.spec @@ -60,6 +60,13 @@ Patch0014: 0014-ldap-add-new-option-ldap_sasl_maxssf.patch Patch0015: 0015-ad-set-min-and-max-ssf-for-ldaps.patch Patch0016: 0016-BE_REFRESH-Do-not-try-to-refresh-domains-from-other-.patch Patch0017: 0017-sysdb_sudo-Enable-LDAP-time-format-compatibility.patch +Patch0018: 0018-sbus_server-stylistic-rename.patch +Patch0019: 0019-sss_ptr_hash-don-t-keep-empty-sss_ptr_hash_delete_da.patch +Patch0020: 0020-sss_ptr_hash-sss_ptr_hash_delete-fix-optimization.patch +Patch0021: 0021-sss_ptr_hash-removed-redundant-check.patch +Patch0022: 0022-sss_ptr_hash-fixed-memory-leak.patch +Patch0023: 0023-sss_ptr_hash-internal-refactoring.patch +Patch0024: 0024-TESTS-added-sss_ptr_hash-unit-test.patch ### Downstream only patches ### Patch0502: 0502-SYSTEMD-Use-capabilities.patch @@ -1089,6 +1096,10 @@ fi %{_libdir}/%{name}/modules/libwbclient.so %changelog +* Wed Feb 26 2020 Michal Židek - 2.2.3-12 +- Resolves: upstream#4135 - util/sss_ptr_hash.c: potential double free in + `sss_ptr_hash_delete_cb()` + * Wed Feb 26 2020 Michal Židek - 2.2.3-11 - Resolves: upstream#4118 - sssd requires timed sudoers ldap entries to be specified up to the seconds