Re: hash_rehash allocation failure

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Eric Blake on 6/18/2009 5:22 PM:
> This patch gets things to the state where I'm happy that the memory 
> allocation 
> failure recovery works correctly.  But as-is, this patch uses a one-pass 
> algorithm, so my test case means the failure recovery can require more memory 
> than the condition that put us into the failure situation in the first place. 
>  
> I have not yet had time to code up a two-pass algorithm, although I claim 
> that 
> it should solve the memory pressure.

Yep, a slight modification to use a one-pass initial attempt and a
two-pass cleanup survived everything I've thrown at it.  Here's the
current state of the patch.  Also available at

http://repo.or.cz/w/gnulib/ericb.git?a=shortlog;h=refs/heads/hash
$ git pull git://repo.or.cz/gnulib/ericb.git hash

along with the changes I made for testing it (although the second commit
is not worth checking in to savannah).

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAko7EnYACgkQ84KuGfSFAYDMlwCeIdd4zzY8U8vc8ZUXdBnwC3uz
NVEAn0WuHS7oL4de57tUq4WuZWfgf8KV
=k/zh
-----END PGP SIGNATURE-----

>From c2a2b4b3acc7a2153fc68738eaba7ac9e628bf41 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Thu, 18 Jun 2009 22:11:48 -0600
Subject: [PATCH] hash: avoid memory leak on allocation failure

* lib/hash.c: (hash_rehash): Avoid memory leak on allocation
failure.  Factor repeated algorithm...
(transfer_entries): ...into new helper routine.
(hash_delete): React to hash_rehash return value.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog  |    6 ++
 lib/hash.c |  219 ++++++++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 161 insertions(+), 64 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 70de83e..21032e5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2009-06-18  Eric Blake  <address@hidden>

+       hash: avoid memory leak on allocation failure
+       * lib/hash.c: (hash_rehash): Avoid memory leak on allocation
+       failure.  Factor repeated algorithm...
+       (transfer_entries): ...into new helper routine.
+       (hash_delete): React to hash_rehash return value.
+
        hash: make rotation more obvious
        * modules/hash (Depends-on): Add bitrotate and stdint.
        * lib/bitrotate.h (rotl_sz, rotr_sz): New functions.
diff --git a/lib/hash.c b/lib/hash.c
index a81eec7..17f87c3 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -836,6 +836,93 @@ hash_find_entry (Hash_table *table, const void *entry,
   return NULL;
 }

+/* Internal helper, to move entries from SRC to DST.  Both tables must
+   share the same free entry list.  If SAFE, only move overflow
+   entries, saving bucket heads for later, so that no allocations will
+   occur.  Return false if the free entry list is exhausted and an
+   allocation fails.  */
+
+static bool
+transfer_entries (Hash_table *src, Hash_table *dst, bool safe)
+{
+  struct hash_entry *bucket;
+  struct hash_entry *cursor;
+  struct hash_entry *next;
+  for (bucket = src->bucket; bucket < src->bucket_limit; bucket++)
+    if (bucket->data)
+      {
+       void *data;
+       struct hash_entry *new_bucket;
+
+       /* Within each bucket, transfer overflow entries first and
+          then the bucket head, to minimize memory pressure.  After
+          all, the only time we might allocate is when moving the
+          bucket head, but moving overflow entries first may create
+          free entries that can be recycled by the time we finally
+          get to the bucket head.  */
+       for (cursor = bucket->next; cursor; cursor = next)
+         {
+           data = cursor->data;
+           new_bucket = (dst->bucket + dst->hasher (data, dst->n_buckets));
+
+           if (! (new_bucket < dst->bucket_limit))
+             abort ();
+
+           next = cursor->next;
+
+           if (new_bucket->data)
+             {
+               /* Merely relink an existing entry, when moving from a
+                  bucket overflow into a bucket overflow.  */
+               cursor->next = new_bucket->next;
+               new_bucket->next = cursor;
+             }
+           else
+             {
+               /* Free an existing entry, when moving from a bucket
+                  overflow into a bucket header.  */
+               new_bucket->data = data;
+               dst->n_buckets_used++;
+               free_entry (dst, cursor);
+             }
+         }
+       /* Now move the bucket head.  Be sure that if we fail due to
+          allocation failure that the src table is in a consistent
+          state.  */
+       data = bucket->data;
+       bucket->next = NULL;
+       if (safe)
+         continue;
+       new_bucket = (dst->bucket + dst->hasher (data, dst->n_buckets));
+
+       if (! (new_bucket < dst->bucket_limit))
+         abort ();
+
+       if (new_bucket->data)
+         {
+           /* Allocate or recycle an entry, when moving from a bucket
+              header into a bucket overflow.  */
+           struct hash_entry *new_entry = allocate_entry (dst);
+
+           if (new_entry == NULL)
+             return false;
+
+           new_entry->data = data;
+           new_entry->next = new_bucket->next;
+           new_bucket->next = new_entry;
+         }
+       else
+         {
+           /* Move from one bucket header to another.  */
+           new_bucket->data = data;
+           dst->n_buckets_used++;
+         }
+       bucket->data = NULL;
+       src->n_buckets_used--;
+      }
+  return true;
+}
+
 /* For an already existing hash table, change the number of buckets through
    specifying CANDIDATE.  The contents of the hash table are preserved.  The
    new number of buckets is automatically selected so as to _guarantee_ that
@@ -848,9 +935,6 @@ bool
 hash_rehash (Hash_table *table, size_t candidate)
 {
   Hash_table *new_table;
-  struct hash_entry *bucket;
-  struct hash_entry *cursor;
-  struct hash_entry *next;

   new_table = hash_initialize (candidate, table->tuning, table->hasher,
                               table->comparator, table->data_freer);
@@ -863,6 +947,20 @@ hash_rehash (Hash_table *table, size_t candidate)
       return true;
     }

+  /* In order for the transfer to successfully complete, we need
+     additional overflow entries when distinct buckets in the old
+     table collide into a common bucket in the new table.  The worst
+     case possible is a hasher that gives a good spread with the old
+     size, but returns a constant with the new size; if we were to
+     guarantee table->n_buckets_used-1 free entries in advance, then
+     the transfer would be guaranteed to not allocate memory.
+     However, for large tables, a guarantee of no further allocation
+     introduces a lot of extra memory pressure, all for an unlikely
+     corner case (most rehashes reduce, rather than increase, the
+     number of overflow entries needed).  So, we instead ensure that
+     the transfer process can be reversed if we hit a memory
+     allocation failure mid-transfer.  */
+
   /* Merely reuse the extra old space into the new table.  */
 #if USE_OBSTACK
   obstack_free (&new_table->entry_stack, NULL);
@@ -870,69 +968,44 @@ hash_rehash (Hash_table *table, size_t candidate)
 #endif
   new_table->free_entry_list = table->free_entry_list;

-  for (bucket = table->bucket; bucket < table->bucket_limit; bucket++)
-    if (bucket->data)
-      for (cursor = bucket; cursor; cursor = next)
-       {
-         void *data = cursor->data;
-         struct hash_entry *new_bucket
-           = (new_table->bucket
-              + new_table->hasher (data, new_table->n_buckets));
-
-         if (! (new_bucket < new_table->bucket_limit))
-           abort ();
-
-         next = cursor->next;
-
-         if (new_bucket->data)
-           {
-             if (cursor == bucket)
-               {
-                 /* Allocate or recycle an entry, when moving from a bucket
-                    header into a bucket overflow.  */
-                 struct hash_entry *new_entry = allocate_entry (new_table);
-
-                 if (new_entry == NULL)
-                   return false;
+  if (transfer_entries (table, new_table, false))
+    {
+      /* Entries transferred successfully; tie up the loose ends.  */
+      free (table->bucket);
+      table->bucket = new_table->bucket;
+      table->bucket_limit = new_table->bucket_limit;
+      table->n_buckets = new_table->n_buckets;
+      table->n_buckets_used = new_table->n_buckets_used;
+      table->free_entry_list = new_table->free_entry_list;
+      /* table->n_entries already holds its value.  */
+#if USE_OBSTACK
+      table->entry_stack = new_table->entry_stack;
+#endif
+      free (new_table);

-                 new_entry->data = data;
-                 new_entry->next = new_bucket->next;
-                 new_bucket->next = new_entry;
-               }
-             else
-               {
-                 /* Merely relink an existing entry, when moving from a
-                    bucket overflow into a bucket overflow.  */
-                 cursor->next = new_bucket->next;
-                 new_bucket->next = cursor;
-               }
-           }
-         else
-           {
-             /* Free an existing entry, when moving from a bucket
-                overflow into a bucket header.  Also take care of the
-                simple case of moving from a bucket header into a bucket
-                header.  */
-             new_bucket->data = data;
-             new_table->n_buckets_used++;
-             if (cursor != bucket)
-               free_entry (new_table, cursor);
-           }
-       }
+      return true;
+    }

-  free (table->bucket);
-  table->bucket = new_table->bucket;
-  table->bucket_limit = new_table->bucket_limit;
-  table->n_buckets = new_table->n_buckets;
-  table->n_buckets_used = new_table->n_buckets_used;
+  /* We've allocated new_table, and possibly moved some entries, but
+     could not move the remaining entries.  We must undo the partial
+     move before returning failure.  The only way to get into this
+     situation is if new_table uses fewer buckets than the old table,
+     so we will reclaim some free entries as overflows in the new
+     table are put back into distinct buckets in the old table.
+
+     There are some pathological cases where a single pass through the
+     table requires more intermediate overflow entries than using two
+     passes.  Two passes give worse cache performance and takes
+     longer, but at this point, we're already out of memory, so slow
+     and safe is better than failure.  */
   table->free_entry_list = new_table->free_entry_list;
+  if (! (transfer_entries (new_table, table, true)
+        && transfer_entries (new_table, table, false)))
+    abort ();
   /* table->n_entries already holds its value.  */
-#if USE_OBSTACK
-  table->entry_stack = new_table->entry_stack;
-#endif
+  free (new_table->bucket);
   free (new_table);
-
-  return true;
+  return false;
 }

 /* If ENTRY matches an entry already in the hash table, return the pointer
@@ -1054,7 +1127,25 @@ hash_delete (Hash_table *table, const void *entry)
                 : (table->n_buckets * tuning->shrink_factor
                    * tuning->growth_threshold));

-             hash_rehash (table, candidate);
+             if (!hash_rehash (table, candidate))
+               {
+                 /* Failure to allocate memory in an attempt to
+                    shrink the table is not fatal.  But since memory
+                    is low, we can at least be kind and free any
+                    spare entries, rather than keeping them tied up
+                    in the free entry list.  */
+#if ! USE_OBSTACK
+                 struct hash_entry *cursor = table->free_entry_list;
+                 struct hash_entry *next;
+                 while (cursor)
+                   {
+                     next = cursor->next;
+                     free (cursor);
+                     cursor = next;
+                   }
+                 table->free_entry_list = NULL;
+#endif
+               }
            }
        }
     }
@@ -1069,9 +1160,9 @@ hash_delete (Hash_table *table, const void *entry)
 void
 hash_print (const Hash_table *table)
 {
-  struct hash_entry const *bucket;
+  struct hash_entry *bucket = (struct hash_entry *) table->bucket;

-  for (bucket = table->bucket; bucket < table->bucket_limit; bucket++)
+  for ( ; bucket < table->bucket_limit; bucket++)
     {
       struct hash_entry *cursor;

-- 
1.6.3.rc3.2.g4b51