From 84145e212df846efc955dfd2c1916945a5c032e0 Mon Sep 17 00:00:00 2001 From: Charlie Vigue Date: Tue, 22 Jul 2025 08:55:45 +0000 Subject: [PATCH] util: Fix a hash table collision bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/util-hash.c | 114 +++++++++++++++++++++++++++++++----------------- 1 file changed, 73 insertions(+), 41 deletions(-) diff --git a/src/util-hash.c b/src/util-hash.c index bdfc1c6f74..da9b6413ed 100644 --- 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 } -