Windows: Hash table improvements

1) The current implementation was not entirely thread-safe. The hash
   table was read outside of the mutex, which in some cases could lead
   to improper hashing.
2) The hash creation function accepted a size parameter that was
   hard-coded. Eliminating this saves time because we can ensure that
   the pre-defined size is a prime number instead of requiring code to
   calculate a prime number.

Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>

diff --git a/libusb/os/windows_nt_common.c b/libusb/os/windows_nt_common.c
index d28cb53..8dcbd9b 100644 (file)
--- a/libusb/os/windows_nt_common.c
+++ b/libusb/os/windows_nt_common.c
@@ -115,7 +115,7 @@ const char *windows_error_str(DWORD retval)
    [Aho,Sethi,Ullman] Compilers: Principles, Techniques and Tools, 1986
    [Knuth]            The Art of Computer Programming, part 3 (6.4)  */
 
-#define HTAB_SIZE 1021
+#define HTAB_SIZE 1021UL       // *MUST* be a prime number!!
 
 typedef struct htab_entry {
        unsigned long used;
@@ -123,29 +123,14 @@ typedef struct htab_entry {
 } htab_entry;
 
 static htab_entry *htab_table = NULL;
-static usbi_mutex_t htab_write_mutex = NULL;
-static unsigned long htab_size, htab_filled;
-
-/* For the used double hash method the table size has to be a prime. To
-   correct the user given table size we need a prime test.  This trivial
-   algorithm is adequate because the code is called only during init and
-   the number is likely to be small  */
-static int isprime(unsigned long number)
-{
-       // no even number will be passed
-       unsigned int divider = 3;
-
-       while((divider * divider < number) && (number % divider != 0))
-               divider += 2;
-
-       return (number % divider != 0);
-}
+static usbi_mutex_t htab_mutex = NULL;
+static unsigned long htab_filled;
 
 /* Before using the hash table we must allocate memory for it.
    We allocate one element more as the found prime number says.
    This is done for more effective indexing as explained in the
    comment for the hash function.  */
-static bool htab_create(struct libusb_context *ctx, unsigned long nel)
+static bool htab_create(struct libusb_context *ctx)
 {
        if (htab_table != NULL) {
                usbi_err(ctx, "hash table already allocated");
@@ -153,19 +138,13 @@ static bool htab_create(struct libusb_context *ctx, unsigned long nel)
        }
 
        // Create a mutex
-       usbi_mutex_init(&htab_write_mutex);
+       usbi_mutex_init(&htab_mutex);
 
-       // Change nel to the first prime number not smaller as nel.
-       nel |= 1;
-       while (!isprime(nel))
-               nel += 2;
-
-       htab_size = nel;
-       usbi_dbg("using %lu entries hash table", nel);
+       usbi_dbg("using %lu entries hash table", HTAB_SIZE);
        htab_filled = 0;
 
        // allocate memory and zero out.
-       htab_table = calloc(htab_size + 1, sizeof(htab_entry));
+       htab_table = calloc(HTAB_SIZE + 1, sizeof(htab_entry));
        if (htab_table == NULL) {
                usbi_err(ctx, "could not allocate space for hash table");
                return false;
@@ -182,13 +161,12 @@ static void htab_destroy(void)
        if (htab_table == NULL)
                return;
 
-       for (i = 0; i < htab_size; i++) {
-               if (htab_table[i].used)
-                       safe_free(htab_table[i].str);
-       }
+       for (i = 0; i < HTAB_SIZE; i++)
+               free(htab_table[i].str);
 
-       usbi_mutex_destroy(&htab_write_mutex);
        safe_free(htab_table);
+
+       usbi_mutex_destroy(&htab_mutex);
 }
 
 /* This is the search function. It uses double hashing with open addressing.
@@ -217,26 +195,29 @@ unsigned long htab_hash(const char *str)
                ++r;
 
        // compute table hash: simply take the modulus
-       hval = r % htab_size;
+       hval = r % HTAB_SIZE;
        if (hval == 0)
                ++hval;
 
        // Try the first index
        idx = hval;
 
+       // Mutually exclusive access (R/W lock would be better)
+       usbi_mutex_lock(&htab_mutex);
+
        if (htab_table[idx].used) {
-               if ((htab_table[idx].used == hval) && (safe_strcmp(str, htab_table[idx].str) == 0))
-                       return idx; // existing hash
+               if ((htab_table[idx].used == hval) && (strcmp(str, htab_table[idx].str) == 0))
+                       goto out_unlock; // existing hash
 
                usbi_dbg("hash collision ('%s' vs '%s')", str, htab_table[idx].str);
 
                // Second hash function, as suggested in [Knuth]
-               hval2 = 1 + hval % (htab_size - 2);
+               hval2 = 1 + hval % (HTAB_SIZE - 2);
 
                do {
                        // Because size is prime this guarantees to step through all available indexes
                        if (idx <= hval2)
-                               idx = htab_size + idx - hval2;
+                               idx = HTAB_SIZE + idx - hval2;
                        else
                                idx -= hval2;
 
@@ -245,35 +226,32 @@ unsigned long htab_hash(const char *str)
                                break;
 
                        // If entry is found use it.
-                       if ((htab_table[idx].used == hval) && (safe_strcmp(str, htab_table[idx].str) == 0))
-                               return idx;
+                       if ((htab_table[idx].used == hval) && (strcmp(str, htab_table[idx].str) == 0))
+                               goto out_unlock;
                } while (htab_table[idx].used);
        }
 
        // Not found => New entry
 
        // If the table is full return an error
-       if (htab_filled >= htab_size) {
-               usbi_err(NULL, "hash table is full (%d entries)", htab_size);
-               return 0;
+       if (htab_filled >= HTAB_SIZE) {
+               usbi_err(NULL, "hash table is full (%lu entries)", HTAB_SIZE);
+               idx = 0;
+               goto out_unlock;
        }
 
-       // Concurrent threads might be storing the same entry at the same time
-       // (eg. "simultaneous" enums from different threads) => use a mutex
-       usbi_mutex_lock(&htab_write_mutex);
-       // Just free any previously allocated string (which should be the same as
-       // new one). The possibility of concurrent threads storing a collision
-       // string (same hash, different string) at the same time is extremely low
-       safe_free(htab_table[idx].str);
-       htab_table[idx].used = hval;
        htab_table[idx].str = _strdup(str);
        if (htab_table[idx].str == NULL) {
                usbi_err(NULL, "could not duplicate string for hash table");
-               usbi_mutex_unlock(&htab_write_mutex);
-               return 0;
+               idx = 0;
+               goto out_unlock;
        }
+
+       htab_table[idx].used = hval;
        ++htab_filled;
-       usbi_mutex_unlock(&htab_write_mutex);
+
+out_unlock:
+       usbi_mutex_unlock(&htab_mutex);
 
        return idx;
 }
@@ -595,7 +573,7 @@ int windows_common_init(struct libusb_context *ctx)
        if (!windows_init_clock(ctx))
                goto error_roll_back;
 
-       if (!htab_create(ctx, HTAB_SIZE))
+       if (!htab_create(ctx))
                goto error_roll_back;
 
        return LIBUSB_SUCCESS;

diff --git a/libusb/version_nano.h b/libusb/version_nano.h
index c84def4..963fb81 100644 (file)
--- a/libusb/version_nano.h
+++ b/libusb/version_nano.h
@@ -1 +1 @@
-#define LIBUSB_NANO 11171
+#define LIBUSB_NANO 11172