Fixed a nasty bug in HashDictionary that could cause heap corruption after removing a value. Added a test case to catch that bug. A few tweaks to Hash.
authorJens Alfke <jens@mooseyard.com>
Thu Sep 24 10:28:50 2009 -0700 (2009-09-24)
changeset 38e3ae153e2c9
parent 2 851de24ecb61
child 4 715d6147ba3a
Fixed a nasty bug in HashDictionary that could cause heap corruption after removing a value. Added a test case to catch that bug. A few tweaks to Hash.
include/Dictionary.h
src/Dictionary.cpp
src/Hash.cpp
src/Hash.h
test/Dictionary_test.cpp
     1.1 --- a/include/Dictionary.h	Sun Sep 20 21:25:47 2009 -0700
     1.2 +++ b/include/Dictionary.h	Thu Sep 24 10:28:50 2009 -0700
     1.3 @@ -24,6 +24,7 @@
     1.4          explicit Key (const Blob &b)                    :Blob(b), hash(computeHash(b)) { }
     1.5          Key (const Blob &b, HashCode h)                 :Blob(b), hash(h) { }
     1.6          Key (const char *str)                           :Blob(str), hash(computeHash(*this)) { }
     1.7 +        Key (const void *bytes, uint32_t len)           :Blob(bytes,len), hash(computeHash(*this)) { }
     1.8          Key (const void *b, uint32_t len, HashCode h)   :Blob(b,len), hash(h) { }
     1.9  
    1.10          static HashCode computeHash (Blob);
     2.1 --- a/src/Dictionary.cpp	Sun Sep 20 21:25:47 2009 -0700
     2.2 +++ b/src/Dictionary.cpp	Thu Sep 24 10:28:50 2009 -0700
     2.3 @@ -186,9 +186,9 @@
     2.4      }
     2.5      
     2.6      bool HashDictionary::remove (Key key) {
     2.7 -        KeyAndValue *kv = (KeyAndValue*) _hash.get(key);
     2.8 +        KeyAndValue *kv = (KeyAndValue*) _hash.remove(key);
     2.9          if (kv) {
    2.10 -            free(kv);
    2.11 +            delete kv;
    2.12              return true;
    2.13          } else
    2.14              return false;
    2.15 @@ -196,7 +196,7 @@
    2.16      
    2.17      void HashDictionary::removeAll() {
    2.18          for (Hash::Iterator it(&_hash); it; ++it)
    2.19 -            free(it.value());
    2.20 +            delete (KeyAndValue*) it.value();
    2.21          _hash.removeAll();
    2.22      }
    2.23          
     3.1 --- a/src/Hash.cpp	Sun Sep 20 21:25:47 2009 -0700
     3.2 +++ b/src/Hash.cpp	Thu Sep 24 10:28:50 2009 -0700
     3.3 @@ -20,7 +20,7 @@
     3.4               _value(value) 
     3.5          { }
     3.6          
     3.7 -        Key key() const                             {return _key;}
     3.8 +        const Key& key() const                      {return _key;}
     3.9          HashCode hash() const                       {return _key.hash;}
    3.10          Hash::Value value() const                   {return _key.bytes ?_value :NULL;}
    3.11          void setValue (Hash::Value value)           {_value = value;}
    3.12 @@ -50,8 +50,7 @@
    3.13      }
    3.14      
    3.15      Hash::IndexEntry& Hash::_find (Key key) const {
    3.16 -        Key k = Key(key);
    3.17 -        IndexEntry newEntry(k);
    3.18 +        IndexEntry newEntry(key);
    3.19          int index = newEntry.hash() % _tableSize;
    3.20          IndexEntry *found = &_table[index];
    3.21          int probe = 0;
    3.22 @@ -78,7 +77,7 @@
    3.23      }
    3.24      
    3.25      void Hash::put(Key key, Hash::Value value) {
    3.26 -        IndexEntry newEntry = IndexEntry(Key(key),value);
    3.27 +        IndexEntry newEntry = IndexEntry(key,value);
    3.28          IndexEntry &found = _findForPut(newEntry);
    3.29          if (found.hasValue())
    3.30              found.setValue(value);
    3.31 @@ -90,15 +89,16 @@
    3.32          }
    3.33      }
    3.34      
    3.35 -    bool Hash::remove(Key key) {
    3.36 +    Hash::Value Hash::remove(Key key) {
    3.37          IndexEntry &entry = _find(key);
    3.38 -        if (!entry.value())
    3.39 -            return false;
    3.40 +        Value formerValue = entry.value();
    3.41 +        if (!formerValue)
    3.42 +            return NULL;
    3.43          entry.markDeleted();
    3.44          _count--;
    3.45          if (_count/(float)_tableSize < Dictionary::kMinLoadFactor)
    3.46              _resize(_tableSize/2);
    3.47 -        return true;
    3.48 +        return formerValue;
    3.49      }
    3.50          
    3.51      void Hash::removeAll() {
    3.52 @@ -109,7 +109,7 @@
    3.53      void Hash::dump() const {
    3.54          for (int index=0; index<_tableSize; index++)
    3.55              if (_table[index].hasValue()) {
    3.56 -                Blob key = _table[index].key();
    3.57 +                const Key &key = _table[index].key();
    3.58                  printf("%3i: %*s -> %p\n", 
    3.59                         index, (int)key.length,(const char*)key.bytes, _table[index].value());
    3.60              }
     4.1 --- a/src/Hash.h	Sun Sep 20 21:25:47 2009 -0700
     4.2 +++ b/src/Hash.h	Thu Sep 24 10:28:50 2009 -0700
     4.3 @@ -38,8 +38,8 @@
     4.4          /** Sets the given value for the given key, replacing any existing value. */
     4.5          void put(Key, Value);
     4.6          
     4.7 -        /** Removes the value with the given key. */
     4.8 -        bool remove(Key);
     4.9 +        /** Removes the value with the given key. Returns the former value, or NULL if not found. */
    4.10 +        Value remove(Key);
    4.11          
    4.12          /** Removes all values. */
    4.13          void removeAll();
     5.1 --- a/test/Dictionary_test.cpp	Sun Sep 20 21:25:47 2009 -0700
     5.2 +++ b/test/Dictionary_test.cpp	Thu Sep 24 10:28:50 2009 -0700
     5.3 @@ -32,6 +32,26 @@
     5.4      return *sDict;
     5.5  }
     5.6  
     5.7 +TEST(Dictionary,AddRemove) {
     5.8 +    HashDictionary dict;
     5.9 +    EXPECT_EQ(0, dict.count());
    5.10 +    dict.put("key 1", "value 1");
    5.11 +    EXPECT_EQ(1, dict.count());
    5.12 +    dict.put("key 2", "value 2");
    5.13 +    EXPECT_EQ(2, dict.count());
    5.14 +    dict.put("key 3", "value 3");
    5.15 +    EXPECT_TRUE(dict.get("key 1").equals("value 1"));
    5.16 +    EXPECT_TRUE(dict.get("key 2").equals("value 2"));
    5.17 +    EXPECT_TRUE(dict.get("key 3").equals("value 3"));
    5.18 +    EXPECT_TRUE(!dict.get("key 4"));
    5.19 +
    5.20 +    EXPECT_TRUE(dict.remove("key 2"));
    5.21 +    EXPECT_EQ(2, dict.count());
    5.22 +    EXPECT_TRUE(dict.get("key 1").equals("value 1"));
    5.23 +    EXPECT_TRUE(!dict.get("key 2"));
    5.24 +    EXPECT_TRUE(dict.get("key 3").equals("value 3"));
    5.25 +}
    5.26 +
    5.27  TEST(Dictionary,GetAll) {
    5.28      const Dictionary &dict = getDict();
    5.29      EXPECT_EQ( sNWords ,  dict.count() );