From fa01d1d69ceeb4da685e6f39d11e9f5d05201589 Mon Sep 17 00:00:00 2001
Message-Id: <fa01d1d69ceeb4da685e6f39d11e9f5d05201589@dist-git>
From: Vincent Bernat <vincent@bernat.im>
Date: Wed, 23 May 2018 12:45:29 +0200
Subject: [PATCH] util: don't check for parallel iteration in hash-related
functions
RHEL-7.6: https://bugzilla.redhat.com/show_bug.cgi?id=1576464
RHEL-7.5.z: https://bugzilla.redhat.com/show_bug.cgi?id=1581364
This is the responsability of the caller to apply the correct lock
before using these functions. Moreover, the use of a simple boolean
was still racy: two threads may check the boolean and "lock" it
simultaneously.
Users of functions from src/util/virhash.c have to be checked for
correctness. Lookups and iteration should hold a RO
lock. Modifications should hold a RW lock.
Most important uses seem to be covered. Callers have now a greater
responsability, notably the ability to execute some operations while
iterating were reliably forbidden before are now accepted.
Signed-off-by: Vincent Bernat <vincent@bernat.im>
(cherry picked from commit 4d7384eb9ddef2008cb0cc165eb808f74bc83d6b)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Conflicts: src/util/virhash.c: Since
3e7db8d3e8538bcd5deb1b111fb1233fc18831aa isn't backported,
macro that is being removed has misaligned line breaks. It
needs to be removed regardless.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
---
src/util/virhash.c | 37 --------------------
tests/virhashtest.c | 83 ---------------------------------------------
2 files changed, 120 deletions(-)
diff --git a/src/util/virhash.c b/src/util/virhash.c
index 7fa2992f18..475c2b0281 100644
--- a/src/util/virhash.c
+++ b/src/util/virhash.c
@@ -41,12 +41,6 @@ VIR_LOG_INIT("util.hash");
/* #define DEBUG_GROW */
-#define virHashIterationError(ret) \
- do { \
- VIR_ERROR(_("Hash operation not allowed during iteration")); \
- return ret; \
- } while (0)
-
/*
* A single entry in the hash table
*/
@@ -66,10 +60,6 @@ struct _virHashTable {
uint32_t seed;
size_t size;
size_t nbElems;
- /* True iff we are iterating over hash entries. */
- bool iterating;
- /* Pointer to the current entry during iteration. */
- virHashEntryPtr current;
virHashDataFree dataFree;
virHashKeyCode keyCode;
virHashKeyEqual keyEqual;
@@ -339,9 +329,6 @@ virHashAddOrUpdateEntry(virHashTablePtr table, const void *name,
if ((table == NULL) || (name == NULL))
return -1;
- if (table->iterating)
- virHashIterationError(-1);
-
key = virHashComputeKey(table, name);
/* Check for duplicate entry */
@@ -551,9 +538,6 @@ virHashRemoveEntry(virHashTablePtr table, const void *name)
nextptr = table->table + virHashComputeKey(table, name);
for (entry = *nextptr; entry; entry = entry->next) {
if (table->keyEqual(entry->name, name)) {
- if (table->iterating && table->current != entry)
- virHashIterationError(-1);
-
if (table->dataFree)
table->dataFree(entry->payload, entry->name);
if (table->keyFree)
@@ -593,18 +577,11 @@ virHashForEach(virHashTablePtr table, virHashIterator iter, void *data)
if (table == NULL || iter == NULL)
return -1;
- if (table->iterating)
- virHashIterationError(-1);
-
- table->iterating = true;
- table->current = NULL;
for (i = 0; i < table->size; i++) {
virHashEntryPtr entry = table->table[i];
while (entry) {
virHashEntryPtr next = entry->next;
- table->current = entry;
ret = iter(entry->payload, entry->name, data);
- table->current = NULL;
if (ret < 0)
goto cleanup;
@@ -615,7 +592,6 @@ virHashForEach(virHashTablePtr table, virHashIterator iter, void *data)
ret = 0;
cleanup:
- table->iterating = false;
return ret;
}
@@ -643,11 +619,6 @@ virHashRemoveSet(virHashTablePtr table,
if (table == NULL || iter == NULL)
return -1;
- if (table->iterating)
- virHashIterationError(-1);
-
- table->iterating = true;
- table->current = NULL;
for (i = 0; i < table->size; i++) {
virHashEntryPtr *nextptr = table->table + i;
@@ -667,7 +638,6 @@ virHashRemoveSet(virHashTablePtr table,
}
}
}
- table->iterating = false;
return count;
}
@@ -723,23 +693,16 @@ void *virHashSearch(const virHashTable *ctable,
if (table == NULL || iter == NULL)
return NULL;
- if (table->iterating)
- virHashIterationError(NULL);
-
- table->iterating = true;
- table->current = NULL;
for (i = 0; i < table->size; i++) {
virHashEntryPtr entry;
for (entry = table->table[i]; entry; entry = entry->next) {
if (iter(entry->payload, entry->name, data)) {
- table->iterating = false;
if (name)
*name = table->keyCopy(entry->name);
return entry->payload;
}
}
}
- table->iterating = false;
return NULL;
}
diff --git a/tests/virhashtest.c b/tests/virhashtest.c
index 9407f98c4b..77d724c4c6 100644
--- a/tests/virhashtest.c
+++ b/tests/virhashtest.c
@@ -221,32 +221,6 @@ testHashRemoveForEachAll(void *payload ATTRIBUTE_UNUSED,
}
-const int testHashCountRemoveForEachForbidden = ARRAY_CARDINALITY(uuids);
-
-static int
-testHashRemoveForEachForbidden(void *payload ATTRIBUTE_UNUSED,
- const void *name,
- void *data)
-{
- virHashTablePtr hash = data;
- size_t i;
-
- for (i = 0; i < ARRAY_CARDINALITY(uuids_subset); i++) {
- if (STREQ(uuids_subset[i], name)) {
- int next = (i + 1) % ARRAY_CARDINALITY(uuids_subset);
-
- if (virHashRemoveEntry(hash, uuids_subset[next]) == 0) {
- VIR_TEST_VERBOSE(
- "\nentry \"%s\" should not be allowed to be removed",
- uuids_subset[next]);
- }
- break;
- }
- }
- return 0;
-}
-
-
static int
testHashRemoveForEach(const void *data)
{
@@ -303,61 +277,6 @@ testHashSteal(const void *data ATTRIBUTE_UNUSED)
}
-static int
-testHashIter(void *payload ATTRIBUTE_UNUSED,
- const void *name ATTRIBUTE_UNUSED,
- void *data ATTRIBUTE_UNUSED)
-{
- return 0;
-}
-
-static int
-testHashForEachIter(void *payload ATTRIBUTE_UNUSED,
- const void *name ATTRIBUTE_UNUSED,
- void *data)
-{
- virHashTablePtr hash = data;
-
- if (virHashAddEntry(hash, uuids_new[0], NULL) == 0)
- VIR_TEST_VERBOSE("\nadding entries in ForEach should be forbidden");
-
- if (virHashUpdateEntry(hash, uuids_new[0], NULL) == 0)
- VIR_TEST_VERBOSE("\nupdating entries in ForEach should be forbidden");
-
- if (virHashSteal(hash, uuids_new[0]) != NULL)
- VIR_TEST_VERBOSE("\nstealing entries in ForEach should be forbidden");
-
- if (virHashSteal(hash, uuids_new[0]) != NULL)
- VIR_TEST_VERBOSE("\nstealing entries in ForEach should be forbidden");
-
- if (virHashForEach(hash, testHashIter, NULL) >= 0)
- VIR_TEST_VERBOSE("\niterating through hash in ForEach"
- " should be forbidden");
- return 0;
-}
-
-static int
-testHashForEach(const void *data ATTRIBUTE_UNUSED)
-{
- virHashTablePtr hash;
- int ret = -1;
-
- if (!(hash = testHashInit(0)))
- return -1;
-
- if (virHashForEach(hash, testHashForEachIter, hash)) {
- VIR_TEST_VERBOSE("\nvirHashForEach didn't go through all entries");
- goto cleanup;
- }
-
- ret = 0;
-
- cleanup:
- virHashFree(hash);
- return ret;
-}
-
-
static int
testHashRemoveSetIter(const void *payload ATTRIBUTE_UNUSED,
const void *name,
@@ -628,9 +547,7 @@ mymain(void)
DO_TEST("Remove", Remove);
DO_TEST_DATA("Remove in ForEach", RemoveForEach, Some);
DO_TEST_DATA("Remove in ForEach", RemoveForEach, All);
- DO_TEST_DATA("Remove in ForEach", RemoveForEach, Forbidden);
DO_TEST("Steal", Steal);
- DO_TEST("Forbidden ops in ForEach", ForEach);
DO_TEST("RemoveSet", RemoveSet);
DO_TEST("Search", Search);
DO_TEST("GetItems", GetItems);
--
2.17.1