util: Fix a hash table collision bug

In util-hash.c there was some behavior that is unexpected and likely
incorrect. To see this behavior, create a hash table 32 entries wide
and use the default hash function. Then add a short string “abc”,
observe the string is stored properly. Now remove a string “iln”, and
observe string “abc” is no longer in the table.

This is because the hash function is not properly handling collisions in
some edge cases.

Includes new unit test:

- UT verifies that the hash function generates a collision for
  the selected test data. This must be true for the bug to be present.
  Then UT demonstrates the bug by adding two items to the hash table
  that collide, and then removing one of them 2x. The bug is that the
  other value is removed as well.

Bug #7828 --> https://redmine.openinfosecfoundation.org/issues/7828

Signed-off-by: Charlie Vigue <charlie.vigue@openvpn.com>

diff --git a/src/util-hash.c b/src/util-hash.c
index bdfc1c6f74ff516e3e39438913abd9a684142006..da9b6413ed881e99bdfe44ae66f791f1bf33af18 100644 (file)
--- a/src/util-hash.c
+++ b/src/util-hash.c
@@ -75,22 +75,38 @@ error:
     return NULL;
 }
 
-void HashTableFree(HashTable *ht)
+/**
+ *  \brief Free a HashTableBucket and return the next bucket
+ *  \param ht Pointer to the HashTable
+ *  \param htb Pointer to the HashTableBucket to free
+ *  \return HashTableBucket* Pointer to the next HashTableBucket or NULL
+ */
+static HashTableBucket *HashTableBucketFree(HashTable *ht, HashTableBucket *htb)
 {
-    uint32_t i = 0;
+    HashTableBucket *next_hashbucket = htb->next;
+    if (ht->Free != NULL)
+        ht->Free(htb->data);
+    SCFree(htb);
+    return next_hashbucket;
+}
 
+/**
+ *  \brief Free a HashTable and all its contents
+ *  \details This function will free all the buckets and the array of buckets.
+ *  \note If the Free function is set, it will be called for each data item in the hash table.
+ *  \param ht Pointer to the HashTable to free
+ *  \return void
+ */
+void HashTableFree(HashTable *ht)
+{
     if (ht == NULL)
         return;
 
     /* free the buckets */
-    for (i = 0; i < ht->array_size; i++) {
+    for (uint32_t i = 0; i < ht->array_size; i++) {
         HashTableBucket *hashbucket = ht->array[i];
         while (hashbucket != NULL) {
-            HashTableBucket *next_hashbucket = hashbucket->next;
-            if (ht->Free != NULL)
-                ht->Free(hashbucket->data);
-            SCFree(hashbucket);
-            hashbucket = next_hashbucket;
+            hashbucket = HashTableBucketFree(ht, hashbucket);
         }
     }
 
@@ -138,44 +154,27 @@ error:
         SCFree(hb);
     return -1;
 }
-
+/**
+ *  \brief Remove an item from the hash table
+ *  \details This function will search for the item in the hash table and remove it if found
+ *  \note If the Free function is set, it will be called for the data item being removed.
+ *  \param ht Pointer to the HashTable
+ *  \param data Pointer to the data to remove
+ *  \param datalen Length of the data to remove
+ *  \return int 0 on success, -1 if the item was not found or an error occurred
+ */
 int HashTableRemove(HashTable *ht, void *data, uint16_t datalen)
 {
     uint32_t hash = ht->Hash(ht, data, datalen);
 
-    if (ht->array[hash] == NULL) {
-        return -1;
-    }
-
-    if (ht->array[hash]->next == NULL) {
-        if (ht->Free != NULL)
-            ht->Free(ht->array[hash]->data);
-        SCFree(ht->array[hash]);
-        ht->array[hash] = NULL;
-        return 0;
-    }
-
-    HashTableBucket *hashbucket = ht->array[hash], *prev_hashbucket = NULL;
-    do {
-        if (ht->Compare(hashbucket->data,hashbucket->size,data,datalen) == 1) {
-            if (prev_hashbucket == NULL) {
-                /* root bucket */
-                ht->array[hash] = hashbucket->next;
-            } else {
-                /* child bucket */
-                prev_hashbucket->next = hashbucket->next;
-            }
-
-            /* remove this */
-            if (ht->Free != NULL)
-                ht->Free(hashbucket->data);
-            SCFree(hashbucket);
+    HashTableBucket **hashbucket = &(ht->array[hash]);
+    while (*hashbucket != NULL) {
+        if (ht->Compare((*hashbucket)->data, (*hashbucket)->size, data, datalen)) {
+            *hashbucket = HashTableBucketFree(ht, *hashbucket);
             return 0;
         }
-
-        prev_hashbucket = hashbucket;
-        hashbucket = hashbucket->next;
-    } while (hashbucket != NULL);
+        hashbucket = &((*hashbucket)->next);
+    }
 
     return -1;
 }
@@ -427,6 +426,39 @@ end:
     if (ht != NULL) HashTableFree(ht);
     return result;
 }
+
+static int HashTableTestCollisionBug(void)
+{
+    HashTable *ht = HashTableInit(32, HashTableGenericHash, NULL, NULL);
+    FAIL_IF_NULL(ht);
+
+    FAIL_IF_NOT(HashTableGenericHash(ht, (void *)"abc", 3) ==
+                HashTableGenericHash(ht, (void *)"iln", 3));
+
+    // Add two strings that collide in the same bucket
+    FAIL_IF_NOT(HashTableAdd(ht, (char *)"abc", 3) == 0);
+    FAIL_IF_NOT(HashTableAdd(ht, (char *)"iln", 3) == 0);
+
+    // Verify both keys are present
+    FAIL_IF_NULL(HashTableLookup(ht, (char *)"abc", 3));
+    FAIL_IF_NULL(HashTableLookup(ht, (char *)"iln", 3));
+
+    // Remove first key once
+    FAIL_IF_NOT(HashTableRemove(ht, (char *)"abc", 3) == 0);
+
+    // Verify first key is gone, second key remains
+    FAIL_IF_NOT_NULL(HashTableLookup(ht, (char *)"abc", 3));
+    FAIL_IF_NULL(HashTableLookup(ht, (char *)"iln", 3));
+
+    // Remove first key again (should not affect "iln")
+    FAIL_IF(HashTableRemove(ht, (char *)"abc", 3) == 0);
+
+    // Verify second key is still present (correct behavior)
+    FAIL_IF_NULL(HashTableLookup(ht, (char *)"iln", 3));
+
+    HashTableFree(ht);
+    PASS;
+}
 #endif
 
 void HashTableRegisterTests(void)
@@ -444,6 +476,6 @@ void HashTableRegisterTests(void)
 
     UtRegisterTest("HashTableTestFull01", HashTableTestFull01);
     UtRegisterTest("HashTableTestFull02", HashTableTestFull02);
+    UtRegisterTest("HashTableTestCollisionBug", HashTableTestCollisionBug);
 #endif
 }
-