Merge lp:~stefan.goetz-deactivatedaccount/hipl/hash_table_comparisons into lp:hipl

[Stefan Götz]

Fixed hash table bug(s)

HIP's hash tables base on OpenSSL's lhash which requires two call-back functions: one for hashing table entries and one for comparing them in the case of hash collisions.

The current code consistently implemented the comparison functions by comparing hashes of the entries. This is incorrect as it does not meet the semantics required of the comparison function. The effect is that when two different table entries have the same hash, the hash table implementation is not able to distinguish them. In this case it can, for example, return the wrong entry during a lookup. Since some hash tables store HIs, this problem might be security sensitive.

With this change, all comparison functions are made to compare the hash input, i.e., the table entries themselves, instead of their hashes.

[Miika Komu (miika-iki)]

I'm glad that somebody did this last. The reason this has been working in the past is that the comparisons with pointers are based on rather long-lived structures such hadb entry.

While this looks ok, it needs some testing. Here's a simple way to test a subset of the functionality changed using two machines (opportunistic mode and registration):

Server:
1. hipd -k
2. hipconf add service rvs relay
3. hipconf get hi default

Client:
4. hipd -k
5. hipconf add server rvs <server-hit(see step 3)> <server-ip> 1234 # normal registration
6. hipconf rst all
7. hipconf add server rvs <server-ip> 1234 # opportunistic registration

P.S. I can do some testing later this week if needed.

review: Needs Information

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> P.S. I can do some testing later this week if needed.

Yes, since I don't have a test setup, I'd appreciate it if you or someone else
could test this.

Stefan

[René Hummen (rene-hummen)]

The patch looks ok. However, I suggest naming the parameters of the compare more expressively as to clarify that one parameter represents the (temporary and) partially filled entry containing only look-up information, whereas the other parameter is an entry of the hash table. Furthermore, mention that the hash of both entries already match when the compare function is used. I will do these changes to doxygen right after the merge.

I agree that the code needs to be tested before merging. Please, go ahead, Miika.

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Maybe another attempt at clarifying why these changes were necessary:

Let's assume, we have a hash table to hold elements of the type

struct entry {
  struct in6_addr hit;
  int foo;
  char bar;
};

and the hash is calculated only over the HIT (i.e., the HIT is the hash input). We initialize the hash table with a call-back function that takes a struct entry instance and hashes the HIT and a call-back function that compares two struct entry instances for equality.

Let's assume that we put two struct entry instances into the hash table. The entries have different HITs but they hash to the same hash value. Thus, both are stored in the hash table in the same hash bucket in a linked list.

Later, when when we want to look up an entry we create a 'struct entry' instance called 'needle'. We then copy the HIT we want to look up into it and pass it to the look-up function of the hash table. The hash table implementation then uses the hash call-back to hash the HIT and to find the correct hash bucket. It then iterates over the linked list of the hash bucket and compares every hash bucket entry with 'needle' to figure out, which one is equal to 'needle'.

The existing implementation would compare the hashes of the HITs in 'needle' and the hash bucket entries. Since 'needle' has, by algorithm, the same hash as all the hash bucket entries, the first entry from the hash bucket matches right away and the lookup always returns the first entry from the hash bucket, even if there are other entries in the hash bucket. This might, of course, mean that the returned hash table entry has the same hash as 'needle' but not the same HIT.

With the changes proposed here, the compare call-backs are changed to compare the actual hash input (e.g., the HITs) instead of their hashes so that hash collisions are handled correctly.

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> I will
> do these changes to doxygen right after the merge.

Wouldn't it make more sense to add these changes to the branch before merging it? Isn't the idea to keep working on the branch until everybody is happy with it and to merge it only then?

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> The reason this has been working in the
> past is that the comparisons with pointers are based on rather long-lived
> structures such hadb entry.

Actually, for the hash tables, the comparison call-back functions are not based on pointers but on hashes (the comparisons *are* pointer-based for the linked-list derivation of the hash table). Therefore, I believe there are two other reasons why this has been working in the past:

1) SHA1 is an excellent hash function and the hash table uses enough hash buckets compared to the entries stored in them by HIP, so that hash collisions were, indeed, quite unlikely, if they ever occurred. Without having really checked this, I assume that hipd never actually puts enough entries into any of these hash tables to create a lot of hash collisions.

2) This is a really hard bug to catch because it's so transient that you couldn't blame anyone for simply overlooking it :-)

[Miika Komu (miika-iki)]

> > The reason this has been working in the
> > past is that the comparisons with pointers are based on rather long-lived
> > structures such hadb entry.
>
> Actually, for the hash tables, the comparison call-back functions are not
> based on pointers but on hashes (the comparisons *are* pointer-based for the
> linked-list derivation of the hash table). Therefore, I believe there are two
> other reasons why this has been working in the past:

You're correct, though this function seems to actually compare pointers:

firewall/dlist.c:find_in_dlist()

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> You're correct, though this function seems to actually compare pointers:
>
> firewall/dlist.c:find_in_dlist()

What is the point of finding a pointer in a list if I already have that very
pointer at hand?

If that code does what I think it is doing, that list implementation is so
broken it just cannot do any useful work. So I hope that code does not do what I
think it is doing. Wait - I'm not sure that that would be a good thing either.

In any case, this is off-topic. I'll put it into a bug report.

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> You're correct, though this function seems to actually compare pointers:
>
> firewall/dlist.c:find_in_dlist()

Oops, I was a little too bug-happy there - that code is fine as long as it is
used the way it is.<eom>

[Miika Komu (miika-iki)]

Tested - changes affecting hipd are kosher. Next firewall..

[Miika Komu (miika-iki)]

Tested also firewall (hipfw -kld). It can still accept and drop connections properly. So the changes seem to work nicely.

review: Approve

[Stefan Götz (stefan.goetz-deactivatedaccount)]

Are there any remaining obstacles to merging this branch?

[René Hummen (rene-hummen)]

> > I will
> > do these changes to doxygen right after the merge.
>
> Wouldn't it make more sense to add these changes to the branch before merging
> it? Isn't the idea to keep working on the branch until everybody is happy with
> it and to merge it only then?

Are you willing to fix the doxygen? Otherwise, I need write permissions on your branch.

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> I suggest naming the parameters of the compare
> more expressively as to clarify that one parameter represents the (temporary
> and) partially filled entry containing only look-up information, whereas the
> other parameter is an entry of the hash table.

The information, which paramater is which, would unnecessarily expose an implementation detail of the hash table. I would also be worried that someone in the future might rely on this information and write strange comparison code that treats the two arguments differently.

> Furthermore, mention that the
> hash of both entries already match when the compare function is used.

Yes, I agree that that would me useful. I will make these changes, update the branch, and re-submit this MP.

Given the rich cultural copy&paste tradition in HIP, do you think that now that all comparison functions are fixed and the Doxygen comments are quite explicit about how to use them, we could do without renaming the function arguments? I don't like the idea very much for the reasons mentioned above.

review: Needs Fixing

[René Hummen (rene-hummen)]

> > I suggest naming the parameters of the compare
> > more expressively as to clarify that one parameter represents the (temporary
> > and) partially filled entry containing only look-up information, whereas the
> > other parameter is an entry of the hash table.
>
> The information, which paramater is which, would unnecessarily expose an
> implementation detail of the hash table. I would also be worried that someone
> in the future might rely on this information and write strange comparison code
> that treats the two arguments differently.

Agreed.

> > Furthermore, mention that the
> > hash of both entries already match when the compare function is used.
>
> Yes, I agree that that would me useful. I will make these changes, update the
> branch, and re-submit this MP.

Thanks.

> Given the rich cultural copy&paste tradition in HIP, do you think that now
> that all comparison functions are fixed and the Doxygen comments are quite
> explicit about how to use them, we could do without renaming the function
> arguments? I don't like the idea very much for the reasons mentioned above.

Yes.

[René Hummen (rene-hummen)]

> Given the rich cultural copy&paste tradition in HIP, do you think that now
> that all comparison functions are fixed [...]

I'm actually not sure about that. A grep for HIP_HASHTABLE revealed other locations where it is used:

./firewall/cache_port.c-59-
./firewall/cache_port.c:60:static HIP_HASHTABLE *firewall_port_cache_db = NULL;
./firewall/cache_port.c-61-

-> compare function in line 254 (not modified)

./lib/core/state.h-393- // Has struct hip_peer_addr_list_item s
./lib/core/state.h:394: HIP_HASHTABLE *peer_addresses_old;
./lib/core/state.h-395-
--
./modules/update/hipd/update.c-87- */
./modules/update/hipd/update.c:88: HIP_HASHTABLE *addresses_to_send_echo_request;
./modules/update/hipd/update.c-89-

-> both actually use the (unorthodox) linked list implementation leading to the cmp function in lib/core/hashtable.c:73 (not modified)

[Stefan Götz (stefan.goetz-deactivatedaccount)]

>> now that all comparison functions are fixed [...]
>
> I'm actually not sure about that. A grep for HIP_HASHTABLE revealed other locations where it is used:

You're right, I missed a few comparison functions :-( I'll take care of those
along with the requested documentation changes. I'll also have a look at that
list implementation and how it is used.

Stefan

[Stefan Götz (stefan.goetz-deactivatedaccount)]

The requested changes have been made to this branch.

I also looked at how the hash table is used as a linked list, as suggested by Rene. In the instances he indicated I could not find any problems. It's certainly not the most elegant nor the most efficient list implementation (the complexity of iterating the list is > O(n)), but it works.

review: Abstain

[Miika Komu (miika-iki)]

review: Approve

=== modified file 'firewall/cache.c'
--- firewall/cache.c	2010-09-25 18:30:26 +0000
+++ firewall/cache.c	2010-09-26 15:58:41 +0000
@@ -272,16 +272,23 @@
 }
 
 /**
- * Compare two HITs
- *
- * @param ptr1: pointer to a HIT
- * @param ptr2: pointer to a HIT
- *
- * @return zero if hashes are identical, or one otherwise
+ * Compare two cache table entries to resolve hash collisions.
+ *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
+ * @param ptr1: pointer to a fw_cache_hl_t
+ * @param ptr2: pointer to a fw_cache_hl_t
+ *
+ * @return zero if the peer HITs in both table entries are identical, a non-zero value otherwise.
  */
 static int hip_firewall_match_hit_peer(const void *ptr1, const void *ptr2)
 {
-    return hip_firewall_hash_hit_peer(ptr1) != hip_firewall_hash_hit_peer(ptr2);
+    // stg: can one assume that there will always be at most one fw_cache_hl_t object per hit_peer?
+    // If so, one could compare the pointers instead because if the hit_peers are the same, the pointers would be the same, and comparing the pointers is faster.
+    const struct in6_addr *peer1 = &((const fw_cache_hl_t *) ptr1)->hit_peer;
+    const struct in6_addr *peer2 = &((const fw_cache_hl_t *) ptr2)->hit_peer;
+    return memcmp(peer1, peer2, sizeof(*peer1));
 }
 
 /**

=== modified file 'firewall/cache_port.c'
--- firewall/cache_port.c	2010-09-25 18:30:26 +0000
+++ firewall/cache_port.c	2010-09-26 15:58:41 +0000
@@ -246,14 +246,17 @@
 /**
  * Compare two keys for the hashtable
  *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
  * @param ptr1 pointer to the first key
  * @param ptr2 pointer to the second key
  *
- * @return 0 if hashes identical, otherwise 1
+ * @return 0 if keys identical, otherwise != 0
  */
 static int hip_firewall_match_port_cache_key(const void *ptr1, const void *ptr2)
 {
-    return hip_firewall_port_hash_key(ptr1) != hip_firewall_port_hash_key(ptr2);
+    return strncmp((const char *)ptr1, (const char *)ptr2, FIREWALL_PORT_CACHE_KEY_LENGTH);
 }
 
 /**

=== modified file 'firewall/user_ipsec_sadb.c'
--- firewall/user_ipsec_sadb.c	2010-09-25 18:30:26 +0000
+++ firewall/user_ipsec_sadb.c	2010-09-26 15:58:41 +0000
@@ -127,6 +127,9 @@
 /**
  * compares the hashes of 2 SA entries to check if they are the same
  *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
  * @param sa_entry1     first SA entry to be compared with
  * @param sa_entry2     second SA entry to be compared with
  * @return              1 if different entries, else 0
@@ -134,22 +137,16 @@
 static int hip_sa_entries_cmp(const hip_sa_entry_t *sa_entry1,
                               const hip_sa_entry_t *sa_entry2)
 {
-    int err             = 0;
-    unsigned long hash1 = 0;
-    unsigned long hash2 = 0;
+    int result = 0;
 
     // values have to be present
     HIP_ASSERT(sa_entry1 && sa_entry2);
 
-    HIP_IFEL(!(hash1 = hip_sa_entry_hash(sa_entry1)), -1,
-             "failed to hash sa entry\n");
-    HIP_IFEL(!(hash2 = hip_sa_entry_hash(sa_entry2)), -1,
-             "failed to hash sa entry\n");
-
-    err = (hash1 != hash2);
-
-out_err:
-    return err;
+    result = memcmp(&sa_entry1->inner_src_addr, &sa_entry2->inner_src_addr, sizeof(sa_entry1->inner_src_addr));
+    if (result != 0) {
+        return result;
+    }
+    return memcmp(&sa_entry1->inner_dst_addr, &sa_entry2->inner_dst_addr, sizeof(sa_entry1->inner_dst_addr));
 }
 
 /**
@@ -191,6 +188,9 @@
 /**
  * compares the hashes of 2 link entries to check if they are the same
  *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
  * @param link_entry1   first link entry to be compared with
  * @param link_entry2   second link entry to be compared with
  * @return              1 if different entries, else 0
@@ -198,23 +198,17 @@
 static int hip_link_entries_cmp(const hip_link_entry_t *link_entry1,
                                 const hip_link_entry_t *link_entry2)
 {
-    int err             = 0;
-    unsigned long hash1 = 0;
-    unsigned long hash2 = 0;
+    int result = 0;
 
     // values have to be present
     HIP_ASSERT(link_entry1 != NULL && link_entry1->spi != 0);
     HIP_ASSERT(link_entry2 != NULL && link_entry2->spi != 0);
 
-    HIP_IFEL(!(hash1 = hip_link_entry_hash(link_entry1)), -1,
-             "failed to hash link entry\n");
-    HIP_IFEL(!(hash2 = hip_link_entry_hash(link_entry2)), -1,
-             "failed to hash link entry\n");
-
-    err = (hash1 != hash2);
-
-out_err:
-    return err;
+    result = link_entry1->spi - link_entry2->spi;
+    if (result != 0) {
+        return result;
+    }
+    return memcmp(&link_entry1->dst_addr, &link_entry2->dst_addr, sizeof(link_entry1->dst_addr));
 }
 
 /**

=== modified file 'hipd/hadb.c'
--- hipd/hadb.c	2010-09-25 18:30:26 +0000
+++ hipd/hadb.c	2010-09-26 15:58:41 +0000
@@ -147,18 +147,26 @@
  * a comparison function for the hash table algorithm to distinguish
  * two HAs from each other
  *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
  * @param ha1 a HA to compare for equality
  * @param ha2 a HA to compare for equality
  * @return zero if the HAs match or non-zero otherwise
  */
 static int hip_ha_cmp(const hip_ha_t *ha1, const hip_ha_t *ha2)
 {
-    if (ha1 == NULL || &(ha1->hit_our) == NULL || &(ha1->hit_peer) == NULL ||
-        ha2 == NULL || &(ha2->hit_our) == NULL || &(ha2->hit_peer) == NULL) {
+    int result_hit_our = 0;
+
+    if (ha1 == NULL || ha2 == NULL) {
         return 1;
     }
 
-    return hip_ha_LHASH_HASH(ha1) != hip_ha_LHASH_HASH(ha2);
+    result_hit_our = memcmp(&ha1->hit_our, &ha2->hit_our, sizeof(ha1->hit_our));
+    if (result_hit_our != 0) {
+        return result_hit_our;
+    }
+    return memcmp(&ha1->hit_peer, &ha2->hit_peer, sizeof(ha1->hit_peer));
 }
 
 /** A callback wrapper of the prototype required by @c lh_new(). */
@@ -182,7 +190,10 @@
 }
 
 /**
- * test if two peer addresses match
+ * test if two peer addresses match to detect and avoid hash collisions in the peer address list.
+ *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
  *
  * @param ptr1 a pointer to a hip_peer_addr_list_item
  * @param ptr2 a pointer to a hip_peer_addr_list_item
@@ -190,7 +201,9 @@
  */
 static int hip_match_peer_addr(const void *ptr1, const void *ptr2)
 {
-    return hip_hash_peer_addr(ptr1) != hip_hash_peer_addr(ptr2);
+    const struct in6_addr *addr1 = &((const struct hip_peer_addr_list_item *) ptr1)->address;
+    const struct in6_addr *addr2 = &((const struct hip_peer_addr_list_item *) ptr2)->address;
+    return memcmp(addr1, addr2, sizeof(*addr1));
 }
 
 /* PRIMITIVES */

=== modified file 'hipd/hidb.c'
--- hipd/hidb.c	2010-09-25 18:30:26 +0000
+++ hipd/hidb.c	2010-09-26 15:58:41 +0000
@@ -187,13 +187,18 @@
 /**
  * matching function required by hashtable/linked list implementation
  *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
  * @param ptr1 a pointer to hip_host_id_entry
  * @param ptr2 a pointer to hip_host_id_entry
  * @return zero on match or non-zero on unmatch
  */
 int hip_hidb_match(const void *ptr1, const void *ptr2)
 {
-    return hip_hidb_hash(ptr1) != hip_hidb_hash(ptr2);
+    const hip_hit_t *hit1 = &((const struct hip_host_id_entry *) ptr1)->lhi.hit;
+    const hip_hit_t *hit2 = &((const struct hip_host_id_entry *) ptr2)->lhi.hit;
+    return memcmp(hit1, hit2, sizeof(*hit1));
 }
 
 /**

=== modified file 'hipd/hiprelay.c'
--- hipd/hiprelay.c	2010-09-25 18:30:26 +0000
+++ hipd/hiprelay.c	2010-09-26 15:58:41 +0000
@@ -314,18 +314,20 @@
 /**
  * relay hash table comparison function
  *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
  * @param rec1 a hip_relrec_t structure
  * @param rec2 a hip_relrec_t structure
  * @return zero if the structures are equal or one otherwise
  */
 static int hip_relht_cmp(const hip_relrec_t *rec1, const hip_relrec_t *rec2)
 {
-    if (rec1 == NULL || &(rec1->hit_r) == NULL ||
-        rec2 == NULL || &(rec2->hit_r) == NULL) {
+    if (rec1 == NULL || rec2 == NULL) {
         return 1;
     }
 
-    return hip_relht_hash(rec1) != hip_relht_hash(rec2);
+    return memcmp(&rec1->hit_r, &rec2->hit_r, sizeof(rec1->hit_r));
 }
 
 /** A callback wrapper of the prototype required by @c lh_new(). */
@@ -585,8 +587,10 @@
 static IMPLEMENT_LHASH_HASH_FN(hip_relwl, const hip_hit_t)
 
 /**
- * The compare function of the @c hiprelay_wl hashtable. Compares the hash
- * values calculated from parameter @c hit1 and @c hit2.
+ * The compare function of the @c hiprelay_wl hashtable.
+ *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
  *
  * @param hit1 a pointer to a HIT.
  * @param hit2 a pointer to a HIT.
@@ -598,7 +602,7 @@
         return 1;
     }
 
-    return hip_relwl_hash(hit1) != hip_relwl_hash(hit2);
+    return memcmp(hit1, hit2, sizeof(*hit1));
 }
 
 /** A callback wrapper of the prototype required by @c lh_new(). */

=== modified file 'hipd/netdev.c'
--- hipd/netdev.c	2010-09-25 18:30:26 +0000
+++ hipd/netdev.c	2010-09-26 15:58:41 +0000
@@ -178,22 +178,29 @@
     const struct netdev_address *na = (const struct netdev_address *) ptr;
     uint8_t hash[HIP_AH_SHA_LEN];
 
+    /* stg: TODO A cryptographically secure hash is unnecessarily expensive for a hash table. Something cheaper, like an XOR, would do just fine. */
     hip_build_digest(HIP_DIGEST_SHA1, &na->addr,
                      sizeof(struct sockaddr_storage), hash);
 
+    /* stg: TODO Returning only a long is all the more reason to move to a cheaper hash function. */
     return *((unsigned long *) hash);
 }
 
 /**
  * equality function for the addresses hash table
  *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
  * @param ptr1 a pointer to a netdev_address structure
  * @param ptr2 a pointer to a netdev_address structure
  * @return 0 if the given pointers match or 1 otherwise
  */
 static int hip_netdev_match(const void *ptr1, const void *ptr2)
 {
-    return hip_netdev_hash(ptr1) != hip_netdev_hash(ptr2);
+    const struct netdev_address *na1 = (const struct netdev_address *) ptr1;
+    const struct netdev_address *na2 = (const struct netdev_address *) ptr2;
+    return memcmp(&na1->addr, &na2->addr, sizeof(na1->addr));
 }
 
 /**

=== modified file 'hipd/oppdb.c'
--- hipd/oppdb.c	2010-09-25 18:30:26 +0000
+++ hipd/oppdb.c	2010-09-26 15:58:41 +0000
@@ -121,13 +121,18 @@
 /**
  * matching function for the hashtable implementation
  *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
+ *
  * @param ptr1 a pointer to a hip_opp_block_t structure
  * @param ptr2 a pointer to a hip_opp_block_t structure
  * @return zero on match or non-zero otherwise
  */
 static int hip_oppdb_match_hit(const void *ptr1, const void *ptr2)
 {
-    return hip_hash_hit(ptr1) != hip_hash_hit(ptr2);
+    const hip_opp_block_t *b1 = (const hip_opp_block_t *) ptr1;
+    const hip_opp_block_t *b2 = (const hip_opp_block_t *) ptr2;
+    return memcmp(&b1->peer_phit, &b2->peer_phit, sizeof(hip_hit_t) + sizeof(struct sockaddr_in6));
 }
 
 /**

=== modified file 'hipd/oppipdb.c'
--- hipd/oppipdb.c	2010-09-25 18:30:26 +0000
+++ hipd/oppipdb.c	2010-09-26 15:58:41 +0000
@@ -67,16 +67,19 @@
 }
 
 /**
- * Compares two ip addresses using their hashes
+ * Compares two ip addresses.
+ *
+ * Note that when this function is called, the hashes of the two hash table entries provided as arguments are known to be equal.
+ * The point of this function is to allow the hash table to determine whether the entries (or rather the part used to calculate the hash) themselves are equal or whether they are different and this is just a hash collision.
  *
  * @param ptr1: pointer to the first ip address to compare
  * @param ptr2: pointer to the second ip address to compare
  *
- * @return 0 if the ip hashes are identical, 1 if they are different
+ * @return 0 if the ips are identical, 1 if they are different
  */
 static int hip_oppipdb_match_ip(const void *ptr1, const void *ptr2)
 {
-    return hip_oppipdb_hash_ip(ptr1) != hip_oppipdb_hash_ip(ptr2);
+    return memcmp(ptr1, ptr2, sizeof(hip_oppip_t));
 }
 
 /**