rpms/sssd
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Resolves: upstream#4135 - util/sss_ptr_hash.c: potential double free in sss_ptr_hash_delete_cb()

Browse Source
This commit is contained in:
Michal Židek 2020-02-27 04:07:30 +01:00
parent 44805f5ff8
commit 3e2905a176
8 changed files with 927 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

43
0018-sbus_server-stylistic-rename.patch Normal file
View File

@ -0,0 +1,43 @@
From faa5dbf6f716bd4ac0a3020a28a1ee6fbf74654a Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikhono@redhat.com>
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 <pbrezina@redhat.com>
---
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

91
0019-sss_ptr_hash-don-t-keep-empty-sss_ptr_hash_delete_da.patch Normal file
View File

@ -0,0 +1,91 @@
From adc7730a4e1b9721c93863a1b283457e9c02a3c5 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikhono@redhat.com>
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 <pbrezina@redhat.com>
---
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

62
0020-sss_ptr_hash-sss_ptr_hash_delete-fix-optimization.patch Normal file
View File

@ -0,0 +1,62 @@
From d0eb88089b059bfe2da3bd1a3797b89d69119c29 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikhono@redhat.com>
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 <pbrezina@redhat.com>
---
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

35
0021-sss_ptr_hash-removed-redundant-check.patch Normal file
View File

@ -0,0 +1,35 @@
From 8cc2ce4e9060a71d441a377008fb2f567baa5d92 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikhono@redhat.com>
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 <pbrezina@redhat.com>
---
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

53
0022-sss_ptr_hash-fixed-memory-leak.patch Normal file
View File

@ -0,0 +1,53 @@
From 4bc0c2c7833dd643fc1137daf6519670c05c3736 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikhono@redhat.com>
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 <pbrezina@redhat.com>
---
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

366
0023-sss_ptr_hash-internal-refactoring.patch Normal file
View File

@ -0,0 +1,366 @@
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 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 <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

266
0024-TESTS-added-sss_ptr_hash-unit-test.patch Normal file
View File

@ -0,0 +1,266 @@
From 88b23bf50dd1c12413f3314639de2c3909bd9098 Mon Sep 17 00:00:00 2001
From: Alexey Tikhonov <atikhono@redhat.com>
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 <pbrezina@redhat.com>
---
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 <http://www.gnu.org/licenses/>.
+*/
+
+#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

11
sssd.spec
View File

@ -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 <mzidek@redhat.com> - 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 <mzidek@redhat.com> - 2.2.3-11
- Resolves: upstream#4118 - sssd requires timed sudoers ldap entries to be
specified up to the seconds